preflight: refuse parent-level index builds on partitioned tables

[Kiran01bm]

Summary

Partitioned parent tables (pg_class.relkind = 'p') are now detected at preflight, and any routed plan that builds an index on a parent is refused with a typed verdict before anything executes. Leaf partitions and supported in-place parent ALTERs are unaffected.

What

• Preflight captures the target's relkind; new preflight.CheckPartitionSupport refuses index-building steps (explicit CREATE INDEX [CONCURRENTLY] or ALTER shapes whose online substitution contains CIC) on partitioned parents via typed *UnsupportedPartitionedParentError.
• New verdict reason unsupported-partitioned-parent with the same JSON/exit-code contract as existing refusals.
• Integration tests on a real partitioned fixture: parent CIC and parent ADD CONSTRAINT UNIQUE refuse (and nothing is created); the same changes on a leaf partition execute; parent ADD COLUMN executes.
• docs/engine-role.md notes the limitation.

Why

PostgreSQL cannot run CREATE INDEX CONCURRENTLY on a partitioned parent (SQLSTATE 0A000, verified live), so today those plans die mid-change at runtime. Failing closed at plan time keeps the honesty contract: the engine refuses what it cannot do safely, and orchestrators can surface the refusal as a blocked check. The partition-aware sequence (CREATE INDEX ON ONLY → per-partition CIC → ATTACH PARTITION) is deferred for the time being, not simulated.

Before
┌──────┐   ┌─────────────────────┐   ┌──────────┐   ┌─────────────────────────────┐
│ plan │──▶│ preflight           │──▶│ executor │──▶│ CIC on partitioned parent   │
│      │   │ (size, privileges)  │   │          │   │ fails mid-change (0A000),   │
└──────┘   └─────────────────────┘   └──────────┘   │ INVALID index left behind   │
                                                    └─────────────────────────────┘

After
┌──────┐   ┌─────────────────────────────────┐
│ plan │──▶│ preflight                       │
│      │   │ size, privileges,               │──▶ relkind='p' + BuildsIndex()?
└──────┘   │ partition support               │    ┌──────────────────────────────────┐
           └─────────────────────────────────┘    │ typed refusal                    │
                       │ otherwise                │ unsupported-partitioned-parent   │
                       ▼                          │ nothing executed                 │
                   executor (unchanged for        └──────────────────────────────────┘
                   leaf partitions & plain
                   parent ALTERs)

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 4c64880, built and run against live PostgreSQL 16.10, 17.11 and 18.6.

Verdict: the refusal is correctly built and the version boundary is exactly right — one shape gets through it. I went at this from the direction most likely to be wrong: the set of things PostgreSQL refuses on a partitioned parent is larger than "index builds", and the serverMajor < 18 constant is the kind of number that is usually off by one. I ran the whole shape matrix on all three majors rather than trusting either. The version gate is right to the version. The shape set has one hole, and it is the shape adjacent to the one being fixed.

The matrix I built the review on — every candidate shape, three majors, live

                                               PG16.10                    PG17.11                    PG18.6
CHECK ... NOT VALID                            OK                         OK                         OK
FK ... NOT VALID                               ERROR 0A000                ERROR 0A000                OK
FK (validated)                                 OK                         OK                         OK
CREATE UNIQUE INDEX (non-concurrent)           OK                         OK                         OK
ADD CONSTRAINT PK USING INDEX                  ERROR 0A000                ERROR 0A000                ERROR 0A000
ADD COLUMN c int UNIQUE                        ERROR (partition key)      ERROR (partition key)      ERROR (partition key)
ADD CONSTRAINT UNIQUE incl. partition key      OK                         OK                         OK
CREATE INDEX CONCURRENTLY                      ERROR 0A000                ERROR 0A000                ERROR 0A000
REINDEX TABLE CONCURRENTLY                     OK                         OK                         OK
ADD COLUMN plain (control)                     OK                         OK                         OK

Three of these directly confirm decisions in the PR. CHECK … NOT VALID works everywhere, so scoping the NOT VALID rule to ConstraintForeignKey is right, not an omission. REINDEX … CONCURRENTLY works on a parent, and buildsIndex is correctly not set for KindReindex, so no false refusal. And the FK row flips exactly at 18 — serverMajor < 18 is right to the version.

Findings

1. ADD CONSTRAINT … USING INDEX is the other 0A000 shape, and admission lets it through. It builds no index, so BuildsIndex() is false by design (constraintBuildsIndex returns false the moment Indexname is set) — but PostgreSQL refuses it on a partitioned parent on every supported major, not just before 18. Unlike the FK case there is no version where it works.

The consequence is the one this PR exists to prevent, on the surface an orchestrator gates on:

$ pg-sprite migrate --alter "ALTER TABLE public.p ADD CONSTRAINT p_pk2 PRIMARY KEY USING INDEX ix_p_id" --dry-run --json
disposition: execute | reason: (none)
  stmt disposition: execute | exec_sql: ['ALTER TABLE public.p ADD CONSTRAINT p_pk2 PRIMARY KEY USING INDEX ix_p_id']

Plan time says execute. Apply time does not:

$ pg-sprite migrate --alter "ALTER TABLE public.p ADD CONSTRAINT p_pk PRIMARY KEY USING INDEX ix_p_id"
failed (execution-failed)
  table:     public.p
  statement: ALTER TABLE public.p ADD CONSTRAINT p_pk PRIMARY KEY USING INDEX ix_p_id
  detail:    sequence step 1 of 1 failed; no earlier steps had committed
  failed at: step 1: ALTER TABLE public.p ADD CONSTRAINT p_pk PRIMARY KEY USING INDEX ix_p_id
pg-sprite: error: ... ERROR: ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables (SQLSTATE 0A000)
exit=1

Compare the shape one row up in the matrix, which the PR does catch: refused (unsupported-partitioned-parent), exit 2. Same server, same target, same SQLSTATE class — and the two land on opposite sides of the refused-vs-failed seam that the whole verdict contract is built around. Nothing is left behind (it is a single statement and fails atomically), so this is not a mid-change-wreckage bug; it is a typing bug, and typing is the product here.

The substituted path is already safe by luck of ordering: the planner's rewrite for ADD CONSTRAINT UNIQUE is CREATE UNIQUE INDEX CONCURRENTLY followed by ADD CONSTRAINT … USING INDEX, and the loop refuses on the first step. It is the submitted form, routed native, that walks through.

Fix is small and fits the existing shape: BuildsIndex() is the wrong predicate because the statement genuinely builds nothing. A third PartitionRefusalCause keyed on OpAddConstraint with a non-empty index name says what is actually true — adopting an existing index into a constraint is unsupported on a partitioned parent — and it needs no version gate.

Repro test (pkg/preflight, fails at head on all five majors)

func TestRefusesPartitionedParentAdoptsExistingIndex(t *testing.T) {
	// ADD CONSTRAINT ... USING INDEX builds no index, so BuildsIndex() is
	// false — but PostgreSQL refuses the shape on a partitioned parent on
	// every supported version (0A000), so admission must not let it through.
	for _, major := range []int{14, 15, 16, 17, 18} {
		cause, err := preflight.RefusesPartitionedParent(major,
			[]string{`ALTER TABLE public.p ADD CONSTRAINT p_pk PRIMARY KEY USING INDEX ix_p_id`})
		require.NoError(t, err)
		require.NotEmpty(t, cause, "PG%d: adopting an index into a constraint is unsupported on a partitioned parent", major)
	}
}

--- FAIL: TestRefusesPartitionedParentAdoptsExistingIndex (0.43s)
    Error:    Should NOT be empty, but was
    Messages: PG14: adopting an index into a constraint is unsupported on a partitioned parent

2. The refused plan report withdraws the sanctioned copy of the sequence and keeps the advisory copy — and on this target every step of it is impossible. RefuseUnsupportedPartitionedParent clears Backend, ExecSQL and Execution, which is exactly right for anything that would run the plan. Decisions[].safer_sql is untouched:

{
  "disposition": "refuse",
  "reason": "unsupported-partitioned-parent",
  "statements": [{
    "sql": "ALTER TABLE public.p ADD CONSTRAINT u2 UNIQUE (id, n)",
    "route": "native",
    "disposition": "refuse",
    "decisions": [{
      "reason": "safer-idiom",
      "safer_sql": [
        "CREATE UNIQUE INDEX CONCURRENTLY \"u2\" ON \"public\".\"p\" (\"id\", \"n\")",
        "ALTER TABLE \"public\".\"p\" ADD CONSTRAINT \"u2\" UNIQUE USING INDEX \"u2\""
      ],
      "safer_sql_execution": "autocommit-each-step"
    }]
  }]
}

Both of those steps fail with 0A000 on this table — row 8 and row 5 of the matrix. #13 established that safer_sql is advice a human or an agent is expected to act on, and the advice here is not merely un-caveated, it is known-impossible against the target the report names. An agent that reads disposition: refuse and then reaches for the suggestion — the obvious next move, and the one the field exists to enable — gets two guaranteed failures.

route: native also survives on the refusal, so a consumer branching on route before disposition sees a native plan.

I'd blank safer_sql/safer_sql_execution on refused statements alongside exec_sql, or (better, since the decision's classification is still true and worth showing) attach the refusal to the decision so the advice carries its own contradiction rather than being silently withdrawn from one field and left in another.

3. (nit) A vanished target reaches the caller as raw pgx.ErrNoRows. sequenceTargetFacts matches pg_class by to_regclass(...); if the table is dropped between preflight and executor admission the WHERE matches nothing and QueryRow(...).Scan returns pgx.ErrNoRows, wrapped as admit sequence target public.p: no rows in result set. Fail-closed, so nothing unsafe — but CheckTable maps the identical condition to the typed ErrTableNotFound two files away, and this is the executor-admission path where an orchestrator most needs to tell "target gone" from "catalog query broke".

Action items

1. (Finding 1) Refuse ADD CONSTRAINT … USING INDEX on a partitioned parent — a PartitionRefusalCause keyed on the constraint's index name, no version gate.
2. (Finding 2) Withdraw or annotate safer_sql on statements refused by partition admission.
3. (optional) (Finding 3) Map pgx.ErrNoRows in sequenceTargetFacts to ErrTableNotFound.

Verified (tried to break, couldn't)

Attacked the version constant first, since a wrong boundary would silently refuse working changes on PG18 or let a broken one through on 17 — it is right to the version, confirmed live on both sides. Checked whether restricting the NOT VALID rule to foreign keys leaves a CHECK … NOT VALID hole: it does not, that shape works on every major. Checked REINDEX … CONCURRENTLY, the obvious false-positive candidate, since it is an index operation on a parent that PostgreSQL supports — buildsIndex is correctly not set for KindReindex, so it is not refused. Probed ADD COLUMN … UNIQUE, the shape that fell through the classifier in #7: alterBuildsIndex now walks AddColumn constraints, and the partition-key-inclusive form (ADD CONSTRAINT UNIQUE (id, n), which PostgreSQL accepts) is correctly refused rather than being missed. Confirmed --force does not bypass the refusal — the CHANGELOG claims this and it holds, which matters because --force's help text promises the run is "still preflighted". Confirmed a leaf partition is unaffected end to end: CREATE INDEX CONCURRENTLY on p1 executes natively, and a plain blocking CREATE INDEX on the leaf still gets the CIC substitution. Checked the refusal ordering in execute() — partition admission now runs before the privilege tier check, so a partitioned parent is refused for the reason that will still be true after the operator fixes their grants, which is the right order. Verified the executor's independent re-check in RunSequence is a genuine second gate rather than a restatement: it re-reads relkind and the server version from the database instead of trusting PreflightedTable, so a library consumer that skips the CLI still cannot start the sequence. Confirmed the report mutation happens before plan.Fingerprint in both call sites, so a refused plan does not fingerprint as its executable twin.

The two things I'd single out: the shape/version matrix underlying RefusesPartitionedParent is right everywhere I could check it except one row, and wiring the check into diffplan.Plan and runDryRun rather than only into apply is precisely the plan-time gap I raised on #31 — that is what makes this a blocked check instead of a failed apply.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Second pass on the same head (4c64880), through the two lenses Armand asks for: ease of OSS adoption and SchemaBot integration. Correctness findings are in the separate comment above.

Lens 1 — OSS adoption ease

The CHANGELOG contradicts itself two entries apart. The new entry says partitioned-parent index builds "now refuse"; the entry immediately below it says "A plain (blocking) CREATE INDEX now succeeds instead of refusing." Both are true, and their scopes are disjoint, but nothing on the page says so. Verified both live:

$ pg-sprite migrate --alter "CREATE INDEX ix_p_blocking ON public.p(n)"     # parent
refused (unsupported-partitioned-parent)

$ pg-sprite migrate --alter "CREATE INDEX ix_p1_blocking ON public.p1(n)"   # leaf
executed natively — the submitted form blocks; pg-sprite ran the safer native sequence instead

A reader going top-down learns a promise and then sees it withdrawn without being told the withdrawal is scoped to parents. One clause on the new entry ("leaf partitions keep the substitution behaviour below") fixes it.

The refusal detail leads with a sentence that is false for one of the two things it refuses. UnsupportedPartitionedParentError.Error() opens with "PostgreSQL cannot build parent-level indexes concurrently" — true for the CIC case, and the honest lead for it. But the same string is returned for a submitted plain CREATE INDEX, which PostgreSQL can do on a parent (I verified: it succeeds). For that case the true reason is the clause buried at the end: pg-sprite refuses to run it under ACCESS EXCLUSIVE. That distinction is the difference between "your database can't" and "we won't" — the second invites a maintenance-window conversation, the first ends it. PartitionRefusalCause already exists as a type; this wants a third value and a second message rather than one string covering both. A user who reads only the first clause will conclude PostgreSQL is the blocker and stop looking.

The limitation is documented on the privileges page. docs/engine-role.md is the page about what to GRANT; a partitioned-parent capability limit is not a role problem and an adopter asking "can pg-sprite handle my partitioned tables?" will not look there. docs/invariants.md has the rigorous statement (RF-6, well written), but that page reads as internal design rationale. There is no adopter-facing "what pg-sprite does and doesn't support yet" surface, and this PR is the second limitation that would belong on it.

The deferred flow has no follow-through for adopters. CREATE INDEX ON ONLY → per-partition CIC → ATTACH PARTITION is named in the error text, the docs and the PR body as deferred, but there is nothing in the repo an adopter can watch. For someone evaluating pg-sprite against a partitioned schema, "not yet" with no tracked issue is indistinguishable from "not planned" — and that is the difference between adopting now and walking away.

No escape hatch, and that should be stated as policy. I verified --force does not bypass this refusal, which is the right call for the concurrent case. But an operator who wants the blocking parent build inside a maintenance window now has no path through pg-sprite at all and must drop to psql — which is precisely the "engine gets bypassed once" outcome the project's honesty contract is trying to avoid. Whatever the answer, say it explicitly in the docs so it reads as a decision rather than an oversight.

Credit where it's due: the invariant is stated as a capability fact with an enforcement site (*Enforced:* preflight and sequence-executor admission), not as an implementation note, and the invariant table was updated to match rather than left to drift. The docs discipline in this repo remains better than most projects at this stage.

Lens 2 — SchemaBot integration

This closes the plan-time gap I raised on #31. The check runs in diffplan.Plan and runDryRun, not only in execute(). That is the whole difference between SchemaBot rendering a blocked check on the PR and SchemaBot starting an apply that dies at step 1 — exactly the reasoning in the PR body, and it is implemented on the surface that matters rather than only on the apply path. Same for the executor's independent re-read of relkind: SchemaBot is a library consumer, so a gate that only exists in internal/cli would not protect it.

plan.Report now carries two refusal vocabularies. The new top-level reason is a verdict.Reason, while per-operation causes in decisions[].reason are planner.Reason — one document, two enums, two owning packages, and pkg/plan now imports pkg/verdict. It is additive and omitempty, so format_version staying at 1 is defensible for wire compatibility. But SchemaBot branches on these strings, and docs/plan-report.md says only "currently unsupported-partitioned-parent" without saying whether the field is closed, who owns the vocabulary, or whether a consumer seeing an unknown value should fail closed. Given the engine's posture everywhere else, "unknown reason ⇒ treat as refused" is presumably the intent — worth stating.

Report-level reason cannot say which statement. RefuseUnsupportedPartitionedParent marks per-statement dispositions but hoists a single reason to the report. SchemaBot's PR comment renders per-statement rows, so a multi-statement plan with one refused statement has to infer the mapping by re-deriving the check — or show the reason against the whole table. A per-statement reason (or reusing decisions[].reason) would make the rendering direct.

Detail is safe to render verbatim, and that is worth keeping deliberate. UnsupportedPartitionedParentError.Error() returns a fixed English sentence with no interpolation — no identifiers, no server text, no caller-influenced content. That is materially different from PrivilegeError.Error() in #31, which interpolates catalog identifiers and which I flagged as needing clamping before it reaches PR markdown. If that property is intentional, an explicit note on the type would let consumers rely on it instead of re-deriving the risk per error type.

Refusal precedence changed, silently. execute() previously ran the privilege tier check before the size guard; it now runs size → partition → privilege. For an oversized table on an under-privileged role, the reported reason flips from insufficient-privileges to the size refusal. Both are still refusals so nothing unsafe follows, but SchemaBot surfaces the reason to a human who then goes and fixes that specific thing, and the ordering is not documented anywhere. Worth one line in the docs stating the precedence, since it is now a consumed contract.

Cost, for a planner that is remote from the database. CheckTable now runs twice on the apply path — run() calls dryRunFacts (which added a CheckTable) and then execute() calls it again — and once per diffplan.Plan invocation. Each call sums pg_total_relation_size across the whole pg_partition_tree, which is a per-relation-fork stat for every partition. SchemaBot loops Plan per table, so on a wide partitioned schema this multiplies as tables × partitions per plan, on a connection that is not local. The dry-run path only needs relkind; threading the already-computed PreflightedTable through, or using a relkind-only lookup where the size is not wanted, keeps the round-trip budget where it was.

Mapping note for the adapter: DispositionRefuse with reason: unsupported-partitioned-parent must land as a blocked check, never a failed apply — the same treatment as DispositionUnavailable from #20, and for the same reason: the engine is telling the truth about what it cannot do, and converting that into a failure spends an operator's incident attention on a decision that was already made correctly.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving on Armand's behalf. The two comments above are the adversarial correctness pass and the two-lens pass; the findings there are follow-ups, not fix-before-merge blockers — the USING INDEX shape fails atomically and left the target untouched in my repro, so this PR narrows the hole rather than opening one. Wiring the check into diffplan.Plan and the dry-run path (not just apply) is what makes it a blocked check instead of a failed apply, and the serverMajor < 18 boundary is right to the version — I verified it live on 16, 17 and 18.

Reviewed by Claude Code (claude-opus-5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp, Claude Opus 4.6)

All three correctness findings and all lens findings are addressed in the follow-up commit; one item (a public tracking issue for the partition-aware index flow) is being filed separately.

Correctness findings

#	Finding	Status	Explanation
1	ADD CONSTRAINT … USING INDEX passes admission, fails at apply	fixed	New typed cause parent-index-adoption, keyed on the constraint's index name, no version gate — your repro test is adopted verbatim, plus an end-to-end CLI test asserting exit-2 refusal and that no constraint was adopted.
2	Refused statements keep known-impossible safer_sql advice	fixed	RefuseUnsupportedPartitionedParent now blanks safer_sql/safer_sql_execution and attaches a per-statement reason, so the surviving route: native classification carries its own contradiction.
3	(nit) Vanished target surfaces as raw pgx.ErrNoRows	fixed	Executor admission maps it to the typed ErrTableNotFound / table-not-found code, with a drop-between-preflight-and-run regression test.

Lens findings

#	Finding	Status	Explanation
1	CHANGELOG contradicts itself on blocking CREATE INDEX	fixed	The parent entry now scopes itself and states leaf partitions retain the CIC substitution.
2	One refusal message is false for the blocking-build case	fixed	Causes split: parent-concurrent-index-build says PostgreSQL can't; parent-blocking-index-build says pg-sprite won't run it under ACCESS EXCLUSIVE.
3	Limitation documented on the privileges page	fixed	New adopter-facing docs/limitations.md (linked from the docs index); engine-role.md points there.
4	Deferred flow has nothing public to watch	deferred	A public tracking issue (#33) for the CREATE INDEX ON ONLY → per-partition CIC → ATTACH PARTITION flow is filed; the docs say "planned but not yet implemented" until it exists.
5	No escape hatch, unstated as policy	fixed	docs/limitations.md states it as a decision: --force does not bypass, and a maintenance-window blocking build runs outside pg-sprite.
6	Two refusal vocabularies, unknown-value posture undocumented	fixed	The plan contract now documents target-dependent refusal reasons as an open vocabulary owned by pkg/verdict; unknown value ⇒ fail closed.
7	Report-level reason can't say which statement	fixed	statements[].reason added to the contract and populated.
8	Render-verbatim safety of the error is implicit	fixed	Doc comment on UnsupportedPartitionedParentError states the fixed-English, no-interpolation property as deliberate.
9	Refusal precedence changed silently	fixed	Documented in the plan contract: size, then partition support, then privileges.
10	Dry-run/diff pay full partition-tree size cost; CheckTable runs twice	fixed	New single-query LookupTargetFacts (relkind + server major, one round trip) shared by dry-run, diff, and executor admission; the executor keeps its independent re-check.
11	Adapter mapping: refusal ⇒ blocked check, never failed apply	no action	Agreed — that is adapter-side behavior and will be honored in the SchemaBot adapter workstream.