schemadiff: plan a rebuild for an index left invalid by an unfinished build

[Kiran01bm]

schemadiff now reads pg_index.indisvalid into the index model and plans a live invalid index that matches the desired definition as a create-index change instead of "no change".

Why

A concurrent index build that dies after its catalog entry commits — a killed builder, a crashed orchestrator, a lost connection — leaves an index that carries the desired name and definition but is invalid: the planner never uses it and it serves no query. The diff compared indexes by name and definition only, so that leftover diffed to nothing and a re-planned convergence reported the table as already matching the desired state. An orchestrator that re-plans after a crash then settles the change as delivered while the index is invalid — a silent wrong success. The diff has to see validity to plan the rebuild.

What

• schemadiff.Index gains Invalid (from i.indisvalid; the zero value is the delivered state). The desired side, materialized on the scratch schema, is never invalid.
• On a plain table an invalid entry is an unfinished concurrent build — abandoned, or still running — and the diff treats it the same way on both sides. Diff emits the create for a live index that is absent, redefined, or invalid (indexNeedsCreate), and never emits a drop for an invalid entry whatever the desired file says about its name (indexNeedsDrop): a plain DROP INDEX blocks the table and cannot distinguish abandoned debris from a build in flight. A REINDEX … CONCURRENTLY _ccnew leftover therefore no longer wedges the table's plan behind the destructive gate.
• The planned create does not run as-is against the occupied name. A plain CREATE INDEX fails as a duplicate relation (42P07); the concurrent build path (BuildIndexConcurrently) refuses by proof with a typed failed outcome (invalid-index-abandoned for debris, invalid-index-build-in-flight for a running build) and never drops the occupant. Nothing routes from that refusal to a removal: the caller chooses — a library caller runs the same create through executor.RebuildAbandonedIndex, which proves the occupant abandoned by identity under the lock that excludes its builder (LK-5) and then builds; a CLI operator follows the invalid-index runbook. docs/limitations.md states the same.
• On a partitioned parent indisvalid = false means unattached partition indexes, not an unfinished build, so validity plays no part there; Diff computes partitioned once and passes it to the index helpers, so the carve-out sits at the call site. The parent's indexes compare by name and definition as before.
• Tests: unit tests on every diff shape (matching-invalid, redefined-invalid, removed-invalid, partitioned parent); an integration test that produces real debris (a unique concurrent build over duplicates), proves the plain create fails with 42P07, executes the plan's CONCURRENTLY form through RebuildAbandonedIndex, and asserts convergence; a second integration test over the same debris where the desired file redefines the index (plain where the leftover was unique), proving the create-only plan converges through the same path without touching the duplicate rows; and an integration test that parks a caller-owned build at indisready = true, indisvalid = false behind a repeatable-read snapshot and asserts create-only / no-drop, which pins indisvalid against indisready.
• docs/capabilities.md, docs/limitations.md (two desired-file-edit rows), docs/pull.md and the Render contract state the one round-trip exception: a table carrying an invalid index re-diffs to exactly its rebuild, and a freshly pulled baseline that re-diffs to a lone create-index is the signal to check pg_index.indisvalid. docs/invariants.md records the trade under OC-2 — the never-drop rule keeps the cleanup half of the invariant and gives up the "silently converged" half for a removed invalid index (empty plan) — and notes under LK-5 that the diff now emits a create it knows cannot run as-is and depends on LK-5's proof for the plan to complete. The demo/tour.sh assertions are unaffected.

Known gap

A desired file that removes an index whose live entry is invalid diffs to an empty plan: the diff will not drop it and has no other change to hang the observation on, so the leftover is invisible in diff --json. Likewise a Destructive: false create-index whose execution through RebuildAbandonedIndex renames and drops a relation carries no hint of that in the plan. A plan-level non-fatal advisory alongside []Change is the fix; it adds a field to the plan JSON contract and is tracked separately rather than folded in here.

Before / after

Same starting state in both: a live index with the desired name and definition, left invalid by an unfinished build. What changes is which columns the diff compares, and where the leftover is dealt with.

Before                                        After

┌──────────────────────┐                      ┌──────────────────────┐
│ live idx             │                      │ live idx             │
│ indisvalid = false   │                      │ indisvalid = false   │
└──────────┬───────────┘                      └──────────┬───────────┘
           │ compare: name, definition                   │ compare: name, definition, invalid
           ▼                                             ▼
┌──────────────────────┐                      ┌──────────────────────┐
│ matches desired      │                      │ differs: not valid   │
└──────────┬───────────┘                      └──────────┬───────────┘
           ▼                                             ▼
┌──────────────────────┐                      ┌──────────────────────┐
│ no change            │                      │ create-index         │
│ "delivered";         │                      │ (no drop)            │
│ index stays invalid  │                      └──────────┬───────────┘
└──────────────────────┘                                 │ execute
                                                         ▼
                                              ┌──────────────────────┐
                                              │ name occupied:       │
                                              │ build refuses by     │
                                              │ proof, drops nothing │
                                              └──────────┬───────────┘
                                                         │ caller chooses:
                                                         │ RebuildAbandonedIndex
                                                         │ (library) or runbook
                                                         ▼
                                              ┌──────────────────────┐
                                              │ occupant proven      │
                                              │ abandoned (LK-5),    │
                                              │ removed, rebuilt     │
                                              └──────────────────────┘

[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 (1 of 2) — 0e6c43a3 (9 files, +374/−15). This part is the diff behavior and the mutation results; part 2 covers invariants, the consent surface, and the missing shape.

Reading validity into the model is the right fix and the column choice is the crux of it. indisready turns true the moment the build's scan finishes, so a build parked in its final snapshot wait would read as delivered — the one wrong answer available here, and the change picks indisvalid, says why at introspect.go:287, and then proves it against a real parked build rather than a fixture. That test earns the change.

Mutation testing found no survivors, which is rare here:

isUnfinishedBuild → false (revert the whole behavior)   killed  unit
drop the partitioned-parent guard                       killed  unit
indexNeedsDrop loses its invalid early-return           killed  unit (redefined case)
indexNeedsCreate stops consulting validity              killed  unit
introspect reads NOT i.indisready instead of indisvalid killed  INTEGRATION

The last one is the important line. Nothing in the unit layer can tell the two columns apart; it dies only in TestDiffInFlightConcurrentBuildIsNeitherDeliveredNorDropped, on the assertion that a ready-but-unvalidated entry introspects as invalid. Baseline before mutating: go build ./... clean, go test -race ./pkg/schemadiff/... green in 30.5s.

One finding.

Sev	Where	What
med	diff.go:364	A table carrying an invalid index the desired file no longer names now diffs to zero changes. Before this change that entry produced a DROP INDEX with Destructive: true — blocking, but visible. The validity observation is now read, used to suppress the drop, and discarded
nit	diff.go:346, :363, :380	All three helpers take the whole live Model to read one field, and the partitioned-parent carve-out ends up two levels down inside a helper named for builds. Passing partitioned bool from Diff would put the carve-out at the call site where a reader is already thinking about parents

The removed-invalid case reports convergence

indexNeedsDrop returns false for any invalid entry, whatever desired says. Combined with the create loop skipping a name desired does not carry, a live invalid index absent from desired produces no statement at all, and the plan is empty. TestDiffInvalidIndexAbsentFromDesiredIsLeftAlone pins that as intended, and the in-flight integration test asserts the same thing for a live builder.

For the in-flight builder that is unarguably right — you must not drop an index from under a running build, and a plain DROP INDEX cannot tell that build from debris. The part worth separating is that "do not drop it" and "report the table as matching desired" are two different decisions, and only the first one needs to be true for safety. The table genuinely differs from the desired state: an extra relation exists under a name the desired file does not carry. And it is not free — PostgreSQL's own note on a failed concurrent build is that the leftover "will still consume update overhead", so an entry parked at indisready = true, indisvalid = false is maintained on every write while serving no read. An operator diffing that table is told there is nothing to do.

The awkwardness is that Change has nowhere to say it. Every value the diff can return is a statement to execute, so an observation that is deliberately not a statement has no channel — which is why I think this is worth raising now rather than after a plan format is depended on. Two shapes that would work:

• Report it. A plan-level advisory (or a typed non-fatal note alongside []Change) naming the invalid entry and pointing at the runbook. Keeps _ccnew from wedging the plan, and the operator learns the name is occupied.
• Remove it by proof. The mechanism that distinguishes debris from a live build already exists — the locked identity re-verification plus DROP INDEX CONCURRENTLY inside RebuildAbandonedIndex — it just is not reachable as a removal on its own. A "prove and drop" entry point would let the removed case converge the way the named case does.

Reporting is the cheap one and it is the one I would do first: the current behavior is defensible as a policy, but only if a caller can find out it happened.

What holds

The parked-build test is the strongest thing in the change. It builds the indisready = true, indisvalid = false window out of a repeatable-read snapshot that holds no lock, waits on a real catalog poll with a bounded require.Eventually, and registers the teardown before starting the build so a failing assertion still cancels it instead of leaking it into the pool close — with a time.After(time.Minute) guard that turns a hung build into a failure rather than a hang. Then it releases the snapshot and asserts the same desired state re-diffs to nothing, so the test covers both sides of the transition rather than just the parked state.

The _ccnew unwedging is a real improvement, and TestDiffInvalidIndexAbsentFromDesiredIsLeftAlone is careful about it: it puts a valid events_legacy_idx in the same position and asserts that one is still dropped, so the test pins the carve-out rather than "invalid entries are ignored".

The partitioned-parent carve-out is correct and tested in both directions — a matching invalid parent index is not rebuilt (the server never builds a partitioned index concurrently, so invalid means unattached partitions), and one desired drops is still dropped. Dropping the guard fails both halves.

The round-trip exception is proved, not asserted. TestDiffRebuildsInvalidIndex renders the live model, materializes it, and asserts the re-diff equals exactly the one create — so Render's updated contract and the new docs/pull.md paragraph are backed by the test rather than by prose. Reaching the invalid state through a real failed unique build over duplicate rows, and then confirming the planned create is rejected 42P07 before handing it to the recovery, is the right way to build that fixture.

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

[aparajon]

🤖 Adversarial correctness review (2 of 2) — invariants, the consent surface, and one shape with no integration coverage.

Invariants

OC-2 is the entry this change is really about, and it lands on both halves of it. OC-2 says a started build must not be silently marked succeeded and must not be cleaned up by a later change of intent. The named case is the first half, and fixing it is the whole point: a live entry with the desired name and definition that never finished building was diffing to nothing, so a re-plan after a crash reported the change delivered. That is exactly "silently marked succeeded", and the fix is right. Suppressing the drop is the second half, and also right — an in-flight build is started work and a DROP INDEX from under it is the cleanup OC-2 forbids.

Which is why the removed case is worth stating in the summary rather than left implicit: for a table whose desired file no longer names the entry, the change fixes the cleanup half and re-introduces the succeeded half, since an empty plan is how this tool says "delivered". Naming that as a deliberate, documented trade — and citing the invariant it sits under — is more reviewable than the current framing of "not the diff's to remove", which answers the cleanup question only. OC-1 reaches the same place from the other side (in-flight ambiguity must never become a passing status); OC-2 is the sharper citation because it already contemplates a reverted desired file.

LK-5 is cited correctly and needs no text change. The proof, the OID-derived quarantine name, the pre-/post-drop OID checks and the droppability predicate all still live in RebuildAbandonedIndex, so the entry's *Enforced:* line is still accurate. What is new is the reach: the diff now emits a create it knows cannot run as-is, and delegates the occupant to that proof. LK-5 was previously a property of an executor a caller could choose to use; it is now load-bearing for whether a plan can complete. Worth one sentence in the entry — the enforcement did not move, but the set of paths depending on it grew.

ST-8 is untouched — the create still rides the desired schema's proven order, and qualifiedDesired is unchanged.

The plan says non-destructive and the execution drops a relation

For the matching and redefined shapes the emitted change is a single create-index with Destructive: false, and executing it through the recovery quarantines and drops the occupying relation. That is safe — LK-5's proof is what makes it safe, and an invalid index discards no data and backs no query, so Destructive: false is the right value for the field as documented.

The gap is the operator's view. A caller gating on Destructive sees one non-destructive statement; nothing in the plan says a relation under that name will be renamed and dropped on the target. That was not reachable before, because a plan whose create hit an occupied name simply failed. It is reachable now, and it is reachable automatically for a caller that routes the create through the recovery. The plan is the last place a human sees this before it happens, so a line in the plan-report contract — or the advisory suggested in part 1, which would carry this case too — is worth more here than it looks.

Related, and worth aligning because two audiences read two documents: the PR body says "the concurrent build path — which proves the occupant by identity before removing it (RebuildAbandonedIndex, LK-5) — handles the leftover", which reads as though the plan self-heals. Nothing in this repo routes between BuildIndexConcurrently (which refuses with the invalid-index verdict) and RebuildAbandonedIndex (which recovers) — the caller picks, and docs/limitations.md says so precisely: a failed outcome carrying invalid-index-abandoned, cleared per the runbook, or the library caller running the create through the recovery. The docs are right; the summary overstates them.

The redefined shape converges, and nothing proves it

Both integration tests cover the matching shape — real debris, and an in-flight build. The redefined shape (desired changes the definition of a name whose live entry is invalid) has only the unit test, which asserts the plan is create-only. That leaves the interesting half unasserted: the recovery proves the occupant by name, table, validity and droppability, not by definition, so a redefinition over a leftover should converge on the create alone. Should — there was no test either way.

I checked rather than assuming, and it does converge. A failed CREATE UNIQUE INDEX CONCURRENTLY over duplicate rows, desired then asking for a plain non-unique index of the same name: the diff emits the one create, RebuildAbandonedIndex drops exactly the leftover, the rebuilt index is valid, and the re-diff is empty (--- PASS ... (1.66s)). Worth adding as a third integration case, both because it is the shape most likely to regress if identity ever starts consulting the definition, and because it is the one that proves "create alone" is sufficient rather than merely planned. It reuses the existing fixture almost verbatim:

// desired asks for a plain index under the name the failed UNIQUE build left invalid
require.NotEqual(t, live.Indexes[0].Def, want.Indexes[0].Def, "desired redefines the leftover")
changes, err := schemadiff.Diff(schema, live, want)
require.NoError(t, err)
require.Len(t, changes, 1, "create only, no drop")

concurrent, err := statement.Concurrently(changes[0].SQL)
require.NoError(t, err)
rep, err := executor.RebuildAbandonedIndex(t.Context(), pool, concurrent,
    executor.ConcurrentBudget{CallerOwned: true})
require.NoError(t, err, "the recovery must converge a redefinition over the leftover")
require.Len(t, rep.Dropped, 1)

Docs

Thorough, and the two docs/limitations.md rows are the right place for this — the rebuild row states the execution outcome and the remedy, and the removed-or-redefined row is honest that the diff shows no change for a removed index. The docs/pull.md paragraph is the one a reader hits first and it correctly frames the exception as "exactly one create-index", which matches what the integration test asserts. Two small things:

• The docs/capabilities.md cell is now a single ~120-word sentence carrying four clauses. The table around it uses short cells; this one is where a reader stops. The rebuild behavior and the never-drop rule are two facts and read better as two sentences.
• docs/pull.md tells the operator to "check pg_index.indisvalid for the table's indexes" before recording a baseline as verified. Since diff now knows this and shows the rebuild, the more useful instruction is the inverse: a pulled baseline that re-diffs to a lone create-index is the signal, and indisvalid is how you confirm which entry. Same facts, but it starts from what the tool already told them.

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

[aparajon]

🤖 Approving — reading indisvalid into the model is the right fix, and the parked-build integration test proves the column choice against a real indisready = true, indisvalid = false window rather than a fixture. Five mutations, no survivors.

One medium in the review comments: a table carrying an invalid index the desired file no longer names now diffs to zero changes, where before it produced a visible (destructive-gated) drop — so the validity observation is read, used, and discarded. Not dropping it is right; reporting the table as converged is the separable half, and OC-2 is the entry that covers both. Also worth adding: the redefined-invalid shape has no integration coverage — I verified it does converge through the recovery, so it is a missing test rather than a bug.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude) — pull/86, follow-up commit

All findings from both comments are answered: seven fixed in ec79a29, and the one design gap (a plan-level advisory for an invalid index the diff declines to drop) is deferred to block/pg-sprite# since it adds a field to the plan JSON contract.

#	Concern	Status	Explanation
C1-F1	A live invalid index the desired file no longer names diffs to zero changes; the validity observation is read, used to suppress the drop, then discarded — the operator is told there is nothing to do	deferred	Agreed the two decisions ("don't drop" and "report converged") are separable and only the first is needed for safety. The fix is the reporting shape proposed: a typed non-fatal advisory alongside []Change, surfaced in diff --json. That is a plan JSON contract change with a format_version step, so it ships as its own PR: block/pg-sprite#. The trade is now stated explicitly in the PR body ("Known gap") and under OC-2 in docs/invariants.md.
C2-F3	The plan says Destructive: false for the matching/redefined create, yet completing it through RebuildAbandonedIndex renames and drops a relation; nothing in the plan says so	deferred	Same advisory carries this case (one entry per invalid index observed, stating that completion goes through the proven-removal path); tracked in block/pg-sprite#. Destructive: false stays as documented — the field means data loss or a query-serving index removed, and an invalid entry is neither.
C2-F5	The redefined shape converges through the recovery, but only the create-only plan is unit-tested; nothing proves the recovery converges a redefinition over a leftover	fixed (in ec79a29)	TestDiffRebuildsRedefinedInvalidIndex reuses the failed-unique-build debris with desired asking for a plain index of the same name: asserts the definitions differ, one non-destructive create-index, RebuildAbandonedIndex drops exactly one entry without touching the duplicate rows, the rebuilt index is valid with the redefined definition, re-diff empty. 3/3 under scripts/test-flaky.sh.
C2-F4	PR body says the concurrent build path "handles the leftover", reading as if the plan self-heals; nothing routes from BuildIndexConcurrently's refusal to RebuildAbandonedIndex	fixed	PR body rewritten to match docs/limitations.md: the build refuses by proof with a typed failed outcome and drops nothing; the caller chooses RebuildAbandonedIndex (library) or the runbook (CLI). The after-diagram gains the refusal step and the caller's choice.
C2-F1	The removed case fixes OC-2's cleanup half and re-introduces its succeeded half (empty plan = "delivered"); should be named as a deliberate trade under the invariant, not framed as "not the diff's to remove"	fixed (in ec79a29)	New paragraph under OC-2 in docs/invariants.md states the trade, why it is preferred to a name-based drop or a fabricated change, where the leftover resurfaces (pull, runbook), and that OC-1 reaches the same conclusion from the uncertainty side.
C2-F2	LK-5's enforcement is unchanged but its reach grew: the diff now emits a create it knows cannot run as-is and depends on the proof for the plan to complete	fixed (in ec79a29)	Sentence added to the LK-5 entry: the diff leans on the proof without performing it; the plan completes only through RebuildAbandonedIndex or the runbook, since a plain CREATE INDEX fails as a duplicate relation and the build path refuses by proof rather than drop by name.
C2-F7	docs/pull.md tells the operator to go check pg_index.indisvalid; the more useful framing is that a pulled baseline re-diffing to a lone create-index is the signal	fixed (in ec79a29)	Inverted: the lone create-index on a fresh baseline is the signal — nothing else makes a just-exported file diff to a create; indisvalid confirms which entry; then the runbook, or wait for a running build.
C2-F6	docs/capabilities.md plain-tables cell is one ~120-word sentence carrying four clauses	fixed (in ec79a29)	Split into two sentences: the rebuild behaviour, and the never-drop rule with its reason.
C1-F2	indexNeedsCreate, indexNeedsDrop, isUnfinishedBuild take the whole live Model to read PartitionKey; the partitioned carve-out sits two levels down inside a helper named for builds	fixed (in ec79a29)	Diff computes partitioned := live.PartitionKey != "" once, with the carve-out's explanation at that site; the helpers take partitioned bool. Behaviour unchanged; the existing unit tests pin it.

The "What holds" section of part 1 and the ST-8 / docs confirmations in part 2 need no action.

Source: #86, review comments 5576879982 and 5576880610 at head 0e6c43a3.