schemachange: require the table lock and add DropShadow/InspectShadow

[Kiran01bm]

pkg/schemachange's shadow operations now run only under the per-table lock: BuildShadow takes the *dbconn.TableLockSession for the proven table, runs under its Bind context, and confirms from inside its own transaction that the session's backend holds the lock before the first write. The package gains DropShadow (remove exactly the shadow, without CASCADE) and InspectShadow (re-derive the BuiltShadow proof from the catalog for a resume), both under the same lock discipline.

Follows the shadow-builder PR (#117), which is on main.

Why

The shadow builder shipped with the lock deliberately left to a later leaf, so nothing yet stopped two engine instances from building, dropping, or inspecting the same table's shadow at once, and nothing removed or verified a shadow an aborted run left behind — ErrShadowExists pointed at a resume path that did not exist. The lock session and the work run on different connections by design (the lock must survive pool recycling; the work must not), so the work has to prove, on its own connection, that the session it trusts is the one the server actually shows holding the table.

What

• requireTableLock (lock.go) runs before any connection is opened: a nil session, an empty TableLock proof, a lock for another schema or table, or a session whose Err reports loss is ErrInvariantViolation (LK-1).
• confirmTableLock runs inside the working transaction, after SET LOCAL ROLE and before the first write: dbconn.LookupTableLockHolder on the working connection must find the lock granted to lock.BackendPID(). A session whose backend is gone but whose keepalive has not yet noticed is refused here.
• Every operation runs under lock.Bind(ctx); lockLossCause turns a statement cancelled by lock loss into ErrInvariantViolation: LK-1: table lock lost … wrapping the session's reported cause, so a caller sees the loss rather than a bare context.Canceled.
• DropShadow(ctx, pool, lock, target, opts) (drop.go): one bounded transaction under SET LOCAL ROLE owner; resolveShadow refuses anything under the shadow's name that is not a plain table (relkind = 'r') owned by the proof's owner (ST-5); DROP TABLE without CASCADE, since the shadow's identity defaults depend on the source's sequences and never the reverse (D5). No relation is ErrShadowNotFound. The owner comes from the proof rather than the live source, so an orphaned shadow can still be dropped after its source is gone.
• InspectShadow(ctx, pool, lock, target, opts) (inspect.go): one read-only transaction; re-checks the source shape by OID (ST-6, same *preflight.UnsupportedCopySwapShapeError the build raises), resolves the shadow with the same ST-5 gate, and verifyIdentityDefaults proves each source identity column is a plain column on the shadow whose pg_attrdef depends on the source sequence's OID — a CASCADE-stripped or re-pointed default is refused. Returns the same BuiltShadow the build did (newBuiltShadow is shared); the caller compares fingerprints with its checkpoint, because the gated statement is not recoverable from the catalog.
• Tests (lock_, drop_, inspect_integration_test.go): all three operations refuse a nil lock and a lock on another table, creating nothing; a build against a lock whose backend was terminated (default keepalive, so the session still believes it holds) is refused by the in-transaction check; a build parked on the source's ACCESS EXCLUSIVE lock aborts within a named deadline when the lock session is terminated, reports the session's loss as the cause, and leaves no shadow; drop removes only the shadow and the source's identity sequence keeps issuing; drop and inspect refuse a foreign-owned table and a view under the shadow's name and leave them in place; inspect reproduces the built proof exactly and refuses a stripped default, a default pointing at another sequence, another owner, and a source that grew a trigger since the proof. The shadow fixture acquires one lock per build. Run locally against PostgreSQL 16; CI covers 14 → 18.
• Docs: SAFETY.md row, docs/copy-and-swap-design.md package table and D5, docs/invariants.md LK-1 (shadow build/drop/inspect move to Enforced today; copier and cutover remain planned) and ST-5.

Before / after

Example: two engine instances, one table app.orders, an aborted earlier run left app._pgsprite_<hash>_new

BEFORE ─────────────────────────────────────────────────────────────────────────────
  instance A: BuildShadow(pool, target, stmt)        instance B: BuildShadow(pool, target, stmt)
              │ nothing asks for a lock                          │ nothing asks for a lock
              ▼                                                  ▼
       BEGIN … CREATE TABLE _pgsprite_<hash>_new          BEGIN … CREATE TABLE _pgsprite_<hash>_new
       (whichever commits second gets ErrShadowExists; both ran the same DDL on the same table)

  leftover shadow: ErrShadowExists ─▶ "a later resume path inspects it"   (no such path)
  cleanup: operator drops by hand; DROP TABLE app.orders CASCADE silently strips the shadow's default

AFTER ──────────────────────────────────────────────────────────────────────────────
  lock := dbconn.AcquireTableLock(cfg, "app", "orders")        second instance: TableLockHeldError, refuses

  BuildShadow(pool, lock, target, stmt, opts)   DropShadow(pool, lock, target, opts)   InspectShadow(pool, lock, target, opts)
    requireTableLock: nil / wrong table / lost ─▶ ErrInvariantViolation, no connection opened
    ctx = lock.Bind(ctx)                       ─▶ lost lock cancels the statement in flight
    BEGIN; SET LOCAL …; SET LOCAL ROLE owner
      confirmTableLock: pg_locks on this connection shows lock.BackendPID() ─ else ErrInvariantViolation
      build: (as before)                       drop: resolveShadow (plain table,     inspect: recheck shape (ST-6)
                                                     proof owner ─ else ST-5)                 resolveShadow (ST-5)
                                                     DROP TABLE shadow  (no CASCADE)          verifyIdentityDefaults
                                                     none ─▶ ErrShadowNotFound                  (default depends on source seq ─ else ST-5)
                                                                                              introspect source + shadow
    COMMIT                                                                                  ─▶ BuiltShadow == the build's

  lock session terminated mid-build ─▶ CREATE TABLE cancelled ─▶ ErrInvariantViolation: LK-1: table lock lost … ; no shadow
  resume finds ErrShadowExists ─▶ InspectShadow ─▶ compare fingerprints with checkpoint ─▶ continue, or DropShadow and rebuild

References

• docs/invariants.md § LK-1, ST-5, ST-6; docs/copy-and-swap-design.md § D5, D8, package table; pkg/dbconn TableLockSession.Bind, LookupTableLockHolder.

Follow-up: identity handoff and lock-loss reporting

• The identity handoff is derived from the shadow's model: a source identity column the change dropped has no shadow column to carry the default, so BuiltShadow.IdentityColumns() lists only the kept columns (before, the build listed the dropped column too), InspectShadow verifies defaults only for those, and a correctly built shadow whose change dropped an identity column is no longer refused as ST-5. Telling a dropped column apart from a tampered shadow is the caller's fingerprint comparison. D5 in docs/copy-and-swap-design.md records the case.
• lockLossCause reports a loss from the session's Err() rather than from the shape of the cancelled context, so a caller cancelling with its own cause gets that cancellation back, never a fabricated LK-1.
• Tests: dropped identity column excluded from the proof and reproduced by inspection; caller cancellation is not a lock loss; drop and inspect refuse a gone lock session (shared goneLock fixture, so removing either in-transaction check fails the suite); refusal tests create their extra role before the schema so cleanup drops the owned relation first.

Follow-up: build-time handoff proof and pinned lock checks

• The build runs verifyIdentityDefaults after the gated statement, against the shadow it left, so BuiltShadow records exactly the handoffs an inspection of that shadow accepts; a statement that drops an identity column and adds a different column under the same name is refused (ST-5) and leaves no shadow.
• Drop matches the shadow against the proof's owner (the shadow was created under it; the source may be gone); inspect against the live owner (owner drift since the proof is shape drift). Both sites now say so.
• Package doc states that the lock session is a dedicated direct server session apart from the working pool, and the resume order: inspect, compare with the checkpoint, drop only on disagreement or abandonment. The design doc's package row says the same of the session.
• Tests pin the LK-1 checks that were unpinned: a stale session is refused when a rival backend holds the lock (build, drop, inspect); a session that already reported loss is refused before any statement runs, wrapping its own error and no cancellation; a drop parked on the shadow's lock aborts on lock loss and leaves the shadow in place.

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

[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/2) — 0 blocking, 5 non-blocking

Reviewed at 60df87bd0d1b03eb0d28acb74d3d04ba03d4b3b6 (merge-base 18fa3150): the new lock preconditions in pkg/schemachange/lock.go, DropShadow, InspectShadow, the handoffIdentities filter, the BuildShadow signature change and its callers, and the LK-1 / ST-5 registry edits. 16 checks pass, nothing red.

Nothing here can damage a target database, and the in-transaction confirmation is the right shape. The lock and the work are deliberately on different sessions, so a proof that the session object believes it holds the lock proves nothing about the server; asking pg_locks from the connection that is about to write, and comparing the granted holder against the lock session's own backend, is the only check that closes that gap. requireTableLock running before any connection is opened means a refusal costs nothing, and lockLossCause returning err unchanged when lock.Err() is nil is what keeps a caller's own cancellation from being reported as a lock loss that did not happen.

Invariant dispositions

Invariant	Disposition	Note
LK-1	extends	enforcement moves from pkg/dbconn alone to every shadow operation; the registry's Enforced today line is updated in the same PR, which is the right call
ST-5	extends	resolveShadow and verifyIdentityDefaults add pre-cutover refusals under the same rule
ST-6	upholds	InspectShadow re-checks the proven shape inside its own transaction
ST-7	upholds	unchanged; retargetOntoShadow still runs before anything opens a connection

Mutation testing. 22 mutants against ./pkg/schemachange/..., plus 4 re-runs for mutants whose first anchor did not compile. 14 killed, 7 survived:

Mutant	Verdict	Killed by
requireTableLock accepts a lock for another table	killed	TestShadowOperationsRefuseALockForAnotherTable
lockLossCause reports a caller's cancellation as a lock loss	killed	TestShadowOperationsReportACallerCancellationAsTheCallersOwn
handoffIdentities still demands a dropped identity column	killed	TestInspectShadowAcceptsAShadowWhoseIdentityColumnTheChangeDropped
resolveShadow accepts a view under the shadow's name	killed	TestDropShadowRefusesAViewUnderTheShadowName
resolveShadow accepts a relation another role owns	killed	TestDropShadowRefusesARelationAnotherRoleOwns, TestInspectShadowRefusesAShadowWithAnotherOwner
shadow column with an identity of its own is accepted	killed	TestInspectShadowRefusesAShadowWhoseIdentityDefaultWasStripped
stripped / re-pointed identity default is accepted	killed	same, plus …DefaultingToAnotherSequence
confirmTableLock removed from build / drop / inspect	killed ×3	the two gone-session tests
build no longer runs under Bind	killed	TestBuildShadowAbortsWhenTheLockIsLostMidBuild
InspectShadow no longer re-checks the source shape	killed	TestInspectShadowRefusesASourceThatChangedShapeSinceTheProof
requireTableLock removed from drop / inspect	killed ×2	TestShadowOperationsRequireATableLock
confirmTableLock accepts a lock held by a DIFFERENT backend	survived	finding 1
requireTableLock accepts an already-lost session	survived	finding 2
drop no longer runs under Bind	survived	finding 3
inspect no longer runs under Bind	survived	finding 3
confirmTableLock's not-found branch neutered	survived	unreachable as a distinct outcome — see the closing note
requireTableLock's empty-proof branch removed	survived	same
verifyIdentityDefaults' missing-column branch returns nil	survived	same

Non-blocking

1 — the backend comparison is the one new LK-1 check nothing pins, and it is the one the two-instance case runs through

lock.go:51 is what makes confirmTableLock more than a liveness probe: !found (line 48) catches a lock nobody holds, and the PID comparison catches a lock someone else holds. Both gone-session tests terminate the backend and leave the lock unheld, so they exercise only the first. Deleting holder.PID != lock.BackendPID() leaves the whole package green.

That is the collision LK-1 exists for: this instance's lock session dies, a second engine instance acquires the same table lock before this one's keepalive notices, and this one keeps working. Measured with the check deleted, the drop does not merely proceed — it removes the shadow out from under the instance that now holds the lock (shadow table not found on the following inspection).

Test that passes on 60df87b and fails with the backend comparison deleted

pkg/schemachange/lock_integration_test.go:

// The in-transaction confirmation compares backends, not merely that some
// session holds the table. After this instance's lock session is gone, a
// second engine instance can take the same lock; the first must not keep
// working on the table under a lock the server now grants to someone else.
func TestShadowOperationsRefuseALockHeldByAnotherBackend(t *testing.T) {
	f := newShadowFixture(t)
	f.exec(t, `
		CREATE TABLE %s.widgets (
			id bigint PRIMARY KEY,
			qty integer NOT NULL
		)`)
	target := f.prove(t, "widgets")
	buildLock, err := dbconn.AcquireTableLock(t.Context(), f.cfg, f.schema, "widgets")
	require.NoError(t, err)
	_, err = schemachange.BuildShadow(t.Context(), f.pool, buildLock, target, f.alter(t, `ALTER TABLE %s.widgets ALTER COLUMN qty TYPE bigint`), schemachange.Options{})
	require.NoError(t, err)
	require.NoError(t, buildLock.Release(t.Context()))
	shadow := schemachange.ShadowName(f.schema, "widgets")

	stale := f.goneLock(t, "widgets")
	rival, err := dbconn.AcquireTableLock(t.Context(), f.cfg, f.schema, "widgets")
	require.NoError(t, err)
	t.Cleanup(func() { assert.NoError(t, rival.Release(context.WithoutCancel(t.Context()))) })
	require.NotEqual(t, stale.BackendPID(), rival.BackendPID())
	mismatch := fmt.Sprintf("not the lock session's backend %d", stale.BackendPID())

	err = schemachange.DropShadow(t.Context(), f.pool, stale, target, schemachange.Options{})
	assert.ErrorIs(t, err, schemachange.ErrInvariantViolation, "drop")
	assert.ErrorContains(t, err, mismatch, "drop")
	assert.True(t, f.relationExists(t, shadow), "a refused drop removes nothing")

	_, err = schemachange.InspectShadow(t.Context(), f.pool, stale, target, schemachange.Options{})
	assert.ErrorIs(t, err, schemachange.ErrInvariantViolation, "inspect")
	assert.ErrorContains(t, err, mismatch, "inspect")
}

--- PASS: TestShadowOperationsRefuseALockHeldByAnotherBackend (11.85s) on 60df87b. With lock.go:51 changed to if false {:

--- FAIL: TestShadowOperationsRefuseALockHeldByAnotherBackend (11.84s)
    Error: Expected error with "invariant violation" in chain but got nil.
    Error: Should be true
    Error: Error "shadow table not found: t_86943_1._pgsprite_c0f3919ba739e2cb_new"
           does not contain "not the lock session's backend 82"

The third line is the interesting one: the drop succeeded, so by the time the inspection runs there is no shadow left.

2 — the free, pre-connection refusal is also unpinned

lock.go:29 refuses a session that has already reported loss, before any connection is opened. Deleting it leaves the package green, because the operation then reaches Bind and fails on the cancelled context, which lockLossCause re-labels — so the outcome stays closed, but the message changes from LK-1: table lock was lost to LK-1: table lock lost during shadow operation: … (begin shadow drop: context canceled). The cheap refusal that exists precisely so the engine does not dial out is the part that goes untested.

Test that passes on 60df87b and fails with the pre-connection refusal deleted

pkg/schemachange/lock_integration_test.go:

// A lock session that has already reported loss is refused before any
// connection is opened, and the refusal names the loss rather than whatever
// the cancelled work produced downstream.
func TestShadowOperationsRefuseALockSessionThatReportedLoss(t *testing.T) {
	f := newShadowFixture(t)
	f.exec(t, `
		CREATE TABLE %s.widgets (
			id bigint PRIMARY KEY,
			qty integer NOT NULL
		)`)
	target := f.prove(t, "widgets")
	lock := f.lock(t, "widgets", dbconn.WithTableLockKeepalive(time.Second))
	f.terminateBackend(t, lock.BackendPID())
	const lockLossDeadline = 30 * time.Second
	select {
	case <-lock.Done():
	case <-time.After(lockLossDeadline):
		t.Fatalf("lock session did not report loss within %s", lockLossDeadline)
	}

	_, err := schemachange.BuildShadow(t.Context(), f.pool, lock, target, f.alter(t, `ALTER TABLE %s.widgets ALTER COLUMN qty TYPE bigint`), schemachange.Options{})
	assert.ErrorContains(t, err, "LK-1: table lock was lost", "build")
	assert.False(t, f.relationExists(t, schemachange.ShadowName(f.schema, "widgets")), "a refused build creates nothing")

	err = schemachange.DropShadow(t.Context(), f.pool, lock, target, schemachange.Options{})
	assert.ErrorContains(t, err, "LK-1: table lock was lost", "drop")

	_, err = schemachange.InspectShadow(t.Context(), f.pool, lock, target, schemachange.Options{})
	assert.ErrorContains(t, err, "LK-1: table lock was lost", "inspect")
}

--- PASS: TestShadowOperationsRefuseALockSessionThatReportedLoss (2.08s) on 60df87b. With lock.go:29-31 deleted, all three assertions fail with the downstream label:

--- FAIL: TestShadowOperationsRefuseALockSessionThatReportedLoss (2.87s)
    Error: Error "invariant violation: LK-1: table lock lost during shadow operation: … (begin shadow build: context canceled)"
           does not contain "LK-1: table lock was lost"
    … same for drop and inspect

3 — the registry claims Bind for three operations; one of them is pinned

docs/invariants.md:241 now says shadow build, drop, and inspect each "run under its Bind context", and the enumerated evidence is "nil-session, wrong-table, gone-session, and mid-build-loss tests". Only the last of those is about Bind, and it is build-only: replacing ctx, stop := lock.Bind(ctx) with a no-op at drop.go:33 or inspect.go:41 leaves the package green, while the same mutation at shadow.go is killed immediately.

Bind and confirmTableLock cover different windows — confirmTableLock proves the lock at the top of the transaction, Bind cancels a statement that is already in flight when the lock goes. For a drop that is a DROP TABLE parked behind another session's lock on the shadow, which is exactly the situation where the keepalive has time to notice. TestBuildShadowAbortsWhenTheLockIsLostMidBuild already has the machinery (park the statement on a heavyweight lock via backendWaitingOnLock, terminate the lock backend, assert ErrInvariantViolation and that nothing changed); pointing it at DropShadow with the shadow held by a second session would close both. I did not write that one — the mutation survivor above is the specification for it.

4 — a build records an identity handoff it never verifies, and its own inspection then refuses the shadow it produced

shadow.go:266 and inspect.go:100 both filter through handoffIdentities, which matches on the column name alone (identity.go:63). The build applies the defaults before the gated statement runs, so the filter sees whatever the statement left behind — but the build never re-reads them, while InspectShadow does. A statement that removes the source identity column and introduces a different column under the same name therefore passes the filter on both sides and disagrees:

Measured on 60df87b, source widgets(id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY, seqno bigint GENERATED BY DEFAULT AS IDENTITY, qty integer), statement ALTER TABLE widgets DROP COLUMN seqno, ADD COLUMN seqno text, ALTER COLUMN qty TYPE bigint:

build err:                     <nil>
build proof identity columns:  [id seqno]
inspect err:                   invariant violation: ST-5: shadow column seqno
                               does not default to the source sequence …widgets_seqno_seq

So the build hands the caller a BuiltShadow claiming a handoff on a text column that has no such default, and a resume of that same shadow is refused as tampered. Two things follow, neither live today: the checkpoint the caller persists is wrong in a way only cutover would notice, and the resume path cannot resume a shadow this builder created.

It is latent because the planner routes OpDropColumn and OpAddColumn to RouteNative on their own (pkg/planner/planner.go:330-334, 308-328); it takes a multi-op ALTER whose aggregate route is copy-and-swap — the third clause above — to reach BuildShadow. The discriminator that would settle it is already in the file: run verifyIdentityDefaults at the end of the build too, against the same shadow OID, so the build refuses exactly what the inspection refuses. That also removes the "build succeeded, resume says tampered" asymmetry generally, rather than for this one statement shape.

5 — drop and inspect resolve "the source's owner" from two different places

Both doc comments state the same rule — only a plain table owned by the source's owner is this engine's shadow — but drop.go:59 passes target.OwnerRole() (the owner the proof recorded at preflight) while inspect.go:80 passes fidelity.Owner (the owner the catalog reports now), and createShadow checks the new shadow against fidelity.Owner too (shadow.go:419). RecheckCopySwapShape deliberately re-checks OID and shape but not ownership, so the two can diverge whenever ALTER TABLE … OWNER TO lands between the proof and the operation.

Both directions stay closed — the drop refuses a shadow it cannot match, rather than dropping the wrong thing — so this is consistency rather than a hole, and I did not build the divergence in a test. But a rule stated identically in two doc comments and implemented from two different sources is the kind of thing that reads as equivalent until the day it is not; dropShadow already reads nothing else from the catalog, so readFidelity there (or a comment saying the proof's owner is deliberate, and why) would settle which one is the rule.

---

Three defensive branches cannot fire as distinct outcomes, which is why their mutants survive — worth knowing rather than fixing: requireTableLock's empty-proof check (lock.go:22-24) is subsumed by the schema/table comparison below it, since checkProof has already rejected an empty target; confirmTableLock's !found (line 48) is subsumed by the PID comparison, which a zero holder also fails; and verifyIdentityDefaults' pgx.ErrNoRows branch (inspect.go:162) is unreachable because handoffIdentities has already restricted the list to columns the shadow's introspected model contains, in the same transaction. Each one buys a better message, which is a fine reason to keep it.

---

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

[aparajon]

🤖 Review (2/2) — adoption and integration lenses

Same head, 60df87bd0d1b03eb0d28acb74d3d04ba03d4b3b6. Nothing here is blocking; the correctness pass is in the first comment. This half reads the change as an OSS adopter and as an importer (schemabot, and anyone else embedding pkg/schemachange).

Lens 1 — OSS adoption

The shadow lifecycle is now a complete, symmetric surface, and that is the headline. Before this PR an adopter could build a shadow and then had nothing to do with ErrShadowExists except guess; InspectShadow and DropShadow turn the resume path into two named verbs with the same preconditions as the build. Deriving both from the catalog rather than from stored state is the right call for an engine that has to survive its own restarts.

The new operational requirement is invisible from the doc comments. Every shadow operation now needs a *dbconn.TableLockSession, and that session is a dedicated, non-recycling connection whose affinity is proven before the lock is taken (pkg/dbconn/table_lock.go:169-186). An adopter whose database access all runs through a transaction-pooling proxy cannot produce one — proveAffinityOn refuses, which is exactly right, but the refusal arrives at AcquireTableLock with no hint from the copy-and-swap documentation that the path needs a direct connection separate from the working pool. docs/copy-and-swap-design.md now says the operations "each take the *dbconn.TableLockSession for the table"; one more clause saying that session is a direct server session distinct from the pool would save an adopter a round trip through the dbconn package to find out why their deployment cannot run copy-and-swap.

The refusals are strings under one sentinel, and this package already has the better pattern next door. The new ST-5 and LK-1 refusals are all fmt.Errorf("%w: ST-5: …", ErrInvariantViolation) (inspect.go:129, 133, 164, 171, 175; lock.go:20, 24, 27, 30, 49, 52). They are precise and readable, but they collapse into one errors.Is target, so "a relation someone else owns is wearing the shadow's name", "the shadow's key lost its default", and "your lock session's backend is gone" are indistinguishable to a caller that wants to react differently — the first is an operator cleanup, the second is a rebuild, the third is a retry after re-acquiring. pkg/preflight already solved this for the shape gate with a closed cause set plus a typed error carrying Detail (copy_swap_shape.go:13-47); the same shape here would let an adopter branch on cause and keep the formatted text for logs.

Lens 2 — integration ease for schemabot and other importers

The typed cause is the single highest-value change for schemabot specifically. schemabot renders engine outcomes into public pull-request comments, and its own rule is that an untrusted error string is never rendered raw into that markdown — dial failures, hostnames and driver internals leak infrastructure detail and break table layout. Every refusal added here carries exactly the material that rule is about: schema and role names, sequence names, relkind letters, and two server backend PIDs. With one sentinel, an importer's only options are to render nothing useful or to match on substrings. With a cause constant it can map each cause to fixed operator-facing text and log the detail server-side, which is what it already does for CopySwapRefusalCause.

The resume contract itself is easy to consume, and the equality is the good part. InspectShadow returning the same BuiltShadow the build returned — pinned by assert.Equal(t, built, inspected) in TestInspectShadowRederivesTheBuiltProof — makes "compare it with your checkpoint" a single comparison rather than a field walk. Worth keeping that property in mind as cutover lands: it holds only while every field of the proof is derivable from the catalog, which is also why the gated statement is deliberately excluded.

Persisting the proof is the part an importer still has to hand-roll. BuiltShadow's fields are unexported with ten accessors and no marshalling, so the checkpoint the doc comment tells the caller to keep has to be projected field by field into the importer's own type, and Fidelity(), IdentityColumns() and CopyColumns() each have to be walked and re-compared. The two fingerprints cover most of it cheaply; a MarshalJSON on BuiltShadow (or an exported plain-struct Proof view) would make the comparison the API asks for a one-liner for every importer, not just for the in-process test.

ErrShadowExists now has two answers and no stated ordering. InspectShadow and DropShadow take identical arguments and differ only in verb, so the resume policy — inspect, compare against the checkpoint, and drop only when the comparison fails — lives entirely in the reader's head. The consequence of picking the wrong one is asymmetric: an unnecessary inspect costs a read transaction, an unnecessary drop destroys a shadow that was resumable and buys a full re-copy. doc.go is currently a single line; naming that ordering there would put the safe sequence where an importer reads it first, rather than in the design doc.

---

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

[aparajon]

🤖 Approving: 0 blocking, 5 non-blocking — see the correctness review and the adoption/integration lenses. The in-transaction confirmation is the right shape for a lock that lives on a different session; 22 mutants, 14 killed, and the four survivors that matter are missing tests rather than missing checks — two of them ship with a test run both ways.

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

[JashLal]

🤖 Reviewed at f0a243aa31954b13781adc5650c58fb0769c4df9. 🌶️ This changes the safety-critical shadow lifecycle; the blast radius includes deletion of a shadow another engine instance is using. I reproduced two blockers. Approval is submitted under the requested review workflow and does not resolve these findings.

[P1, blocker] A blocked drop can commit after its table lock is lost — pkg/schemachange/drop.go:62.

confirmTableLock runs before DROP TABLE, but that statement can wait for a heavyweight lock. The dedicated advisory-lock session can disappear during that wait, and Bind cancels only after the keepalive notices. If the wait ends sooner, the drop proceeds directly to COMMIT with no ownership check or transaction-level protection over that interval.

Reproduced against PostgreSQL 16 with the default keepalive: build a shadow under instance A's lock; hold ACCESS EXCLUSIVE on the shadow from a separate transaction; start DropShadow(A) and wait until its DROP is blocked; terminate A's lock backend; acquire the same table lock successfully as instance B; release the heavyweight blocker. A returned nil and the shadow was gone, while B's lock remained healthy and A's keepalive had not yet noticed. This violates LK-1 on the actual, unmodified implementation. Ensure the work transaction cannot commit after losing ownership, with synchronization/fencing that covers blocked statements; periodic cancellation and a single check before the statement are insufficient. Add this interleaving as a regression (the existing test deliberately waits for keepalive cancellation while the heavyweight blocker stays held).

[P2, blocker] Lock confirmation can select a holder from another database — pkg/schemachange/lock.go:43.

The newly enforced lookup calls LookupTableLockHolder, whose pg_locks query filters the advisory key but not l.database. Matching schema/table names in separate databases therefore contribute rows to the same lookup. Its ORDER BY l.pid LIMIT 1 can select the other database's session, and the new PID comparison rejects the legitimate local holder. Reproduced with two databases on one PostgreSQL 16 server: both acquired their independent locks for the same qualified table successfully (PIDs 79 and 84); BuildShadow in the second database then failed with LK-1: ... held by backend 79, not ... 84. Scope holder lookup to the current database and cover this with a two-database regression. The lookup helper predates this PR, but requiring its answer before every shadow operation introduces the failure here.

Validation: the complete go test -race ./pkg/schemachange -count=1 -timeout=6m passed on 60df87b (67.493s). The PR then rebased to the reviewed head; its full diff and the schemachange/dbconn/preflight/schemadiff/statement code were unchanged. Both additional regression tests above ran against f0a243a and failed as described. All reported CI checks are green; I did not rerun the full version matrix locally. Outside these findings, I checked the owner/relkind refusals, non-CASCADE drop, retained-identity filtering, typed errors, and the read-only inspection path.

[Kiran01bm]

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

0 blocking. Of the correctness findings, 1, 2, 4 and 5 are fixed and 3 is fixed for drop and deferred for inspect; of the adoption/integration items, the direct-session doc and the resume ordering are fixed, and the typed cause set and the proof marshalling are deferred to follow-ups.

#	Finding	Status	Explanation
C1-F1	confirmTableLock's backend comparison is unpinned; with it deleted a drop under a stale session removes the shadow out from under the instance that now holds the lock.	fixed	TestShadowOperationsRefuseALockHeldByAnotherBackend: a built shadow, a session made stale with goneLock, a rival session that then takes the lock; drop, inspect and build under the stale session are all ErrInvariantViolation, the shadow stays, and the rival's Err() is nil. Same shape as the proposed test minus the ErrorContains assertions — tests here assert typed outcomes, never error text; the rival holding the lock is what makes the PID comparison the only refusing branch. With the comparison replaced by if false the test fails at drop (shadow gone) and inspect (ErrShadowNotFound).
C1-F2	requireTableLock's refusal of a session that already reported loss is unpinned; deleting it keeps the outcome closed but moves the refusal past Bind.	fixed	TestShadowOperationsRefuseALockSessionThatReportedLoss (100 ms keepalive, terminate, wait for Done): each operation's error wraps ErrInvariantViolation and lock.Err() and does not wrap context.Canceled — the downstream path necessarily adds the cancelled begin error, so that is the typed discriminator instead of the message text. Fails on all three operations with the refusal deleted.
C1-F3	Bind for drop and inspect is unpinned; only the build's mid-flight loss is tested.	fixed (drop), deferred (inspect)	Drop: TestDropShadowAbortsWhenTheLockIsLostMidDrop parks DROP TABLE behind a blocker's ACCESS EXCLUSIVE on the shadow, terminates the lock backend, and asserts ErrInvariantViolation wrapping lock.Err() with the shadow still present; fails with Bind replaced by a plain WithCancel. Inspect: its transaction is catalog-only — SET LOCAL, pg_locks, pg_class/pg_attribute/pg_depend reads by OID — so no statement takes a heavyweight lock on a user relation and there is no deterministic point to park it on; locking a system catalog from the test would also block the fixture's own queries. Bind stays on inspect (zero cost, bounds any read in flight) with that mutant accepted as not killable without a fault-injection seam this package does not have.
C1-F4	The build records a handoff it never verifies, so DROP COLUMN seqno, ADD COLUMN seqno text, … yields a proof claiming a handoff on a text column and an inspection that refuses the shadow as tampered.	fixed	verifyIdentityDefaults now also runs at the end of the build, on the shadow the gated statement left, over the same handoffIdentities list; the build refuses (ST-5) exactly what the inspection refuses, and the failed transaction leaves no shadow. The helper moved to identity.go with a doc comment covering both callers. TestBuildShadowRefusesAStatementThatReplacesAnIdentityColumn uses the statement from the finding and asserts ErrInvariantViolation and no shadow.
C1-F5	Drop resolves the shadow's owner from the proof, inspect from the live catalog, under one identically worded rule.	fixed	Both are deliberate and each site now says why: drop uses the proof's owner because the shadow was created under SET LOCAL ROLE of that owner, so it is the owner the engine's shadow has, and the source may no longer exist to ask; inspect uses the live owner because the build already asserts proof owner = live owner at creation, so an owner the source acquired since the proof is shape drift and is refused like any other. No code change: making drop read the live owner would break the orphan case the drop exists for.
C1-closing	Three defensive branches cannot fire as distinct outcomes (empty proof, !found, ErrNoRows).	rejected	Agreed and left as they are, for the reason given: each buys a precise message on a path the invariant would otherwise refuse with a less useful one.
C2-1	The requirement for a dedicated, non-recycling direct session is invisible from the copy-and-swap docs.	fixed	The design doc's pkg/schemachange row now describes the session as a dedicated direct server session, distinct from the working pool, that AcquireTableLock refuses to open through a transaction-pooling proxy; the package doc says the same in its first paragraph.
C2-2	ST-5 / LK-1 refusals collapse into one errors.Is target; a closed cause set with a typed error (as pkg/preflight does for the shape gate) would let importers branch and render fixed text.	deferred	Agreed on the shape and the reason. Not in this PR: the cause set should be closed over the whole shadow lifecycle (build, drop, inspect, and the copier's and cutover's refusals that are not written yet), so it lands once, with the invariant registry rows that name each cause, rather than being extended per PR. Tracked as the next pkg/schemachange leaf.
C2-3	BuiltShadow has ten accessors and no marshalling, so persisting the proof for the checkpoint is hand-rolled per importer.	deferred	Agreed. Held until cutover fixes the proof's field set: a MarshalJSON or exported Proof view shipped now would change shape at least once more, and the two fingerprints already carry the comparison the resume path needs.
C2-4	ErrShadowExists has two answers with no stated ordering.	fixed	The package doc now states the resume order — inspect first, compare with the checkpoint, continue on agreement; drop only on disagreement or when the change is abandoned — and the asymmetry (a read transaction versus a destroyed, possibly resumable shadow and a full rebuild).

"What holds" (invariant dispositions; the 14 killed mutants; the in-transaction confirmation's shape; the assert.Equal(built, inspected) contract) — no action.

Source: block/pg-sprite#121, review comments 5769205600 and 5769206335 and review 5272950893 at head 60df87b; fixes in the follow-up commit.