schemachange: enable the write fence always and check it at swap

[Kiran01bm]

The quiesced Runner's write fence now fires for every session, and its swap refuses when the fence is no longer in place.

Why

The fence was an ordinary (ORIGIN) trigger, so a session running with session_replication_role = replica skipped it: its write succeeded after the verification pass, the run still reached done, and the row was gone from the live table. Nothing between that pass and the rename looked at the fence again, so a fence disabled or dropped mid-run went unnoticed too. The Runner doc promised "a writer that reaches the table gets an error, never a silent loss" (CO-1).

What

• The fence is enabled ALWAYS when installed. A resumed run installs over it, so a fence an older engine left ORIGIN is upgraded rather than refused.
• Under its ACCESS EXCLUSIVE lock, the Runner's swap re-reads the fence by the source's OID and refuses as the new cutover-unfenced (class environmental, CO-1) unless the engine's trigger is there, calling the engine's function, enabled ALWAYS. Nothing is swapped; the next Run reinstalls the fence and verifies again.
• The check is held by the Runner only: a library caller of Cutover that quiesces the table by its own means has no fence to find.
• Limits, stated in the Runner doc: ALWAYS closes the replica-role hole by itself; the swap check only catches a fence that is disabled, dropped, or replaced at swap time. A table owner who disables the fence, writes, and restores it between the pass and the swap is still not seen.
• CO-1 Enforced:, refusal-classes.md, engine-role.md, schemabot-integration.md, and the copy-and-swap design updated (extends CO-1).

Before / after

orders is being changed by the quiesced Runner. After the clean verification pass, a session runs SET session_replication_role = replica; INSERT INTO orders (id, qty) VALUES (999999, 1). Only what the fence does with that insert differs.

ALTER TABLE orders DROP COLUMN note

Before                                     After

fence installed, fires for origin          fence installed, enabled ALWAYS:
sessions only                              fires for every session
        │                                          │
        ▼                                          ▼
copy, then a clean verification pass       copy, then a clean verification pass
        │                                          │
        ▼                                          ▼
the replica-role session inserts           the replica-role session inserts
row 999999: the fence does not fire,       row 999999: the fence refuses it
the insert succeeds                        with SQLSTATE 55006
        │                                          │
        ▼                                          ▼
the swap does not look at the fence        the swap finds the fence in place,
and renames                                enabled ALWAYS, and renames
        │                                          │
        ▼                                          ▼
the run is done, and the live table        the run is done; the insert was
has no row 999999: a lost write            refused, so no write was lost

With the fence disabled (DISABLE TRIGGER) or dropped before the swap: before, the swap went ahead; after, it refuses as cutover-unfenced and the source stays in place.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Review 1/2 of adb5a99: 0 blocking, 2 non-blocking.

I read the whole PR at adb5a99 (a merge of main into the branch; the PR's own change against current main is thirteen files, five of them docs), the install, the swap transaction, the resume path that re-enters it, and the catalog columns the new read relies on. I ran the PR's fence tests plus the refusal-table tests against PostgreSQL 16 (all pass), and the three new fence tests on 14 and 18. The change enables the engine's write fence ALWAYS when it installs it, and has the quiesced Runner's swap re-read the fence under its ACCESS EXCLUSIVE lock and refuse as the new cutover-unfenced unless the trigger is there, calling the engine's function, enabled ALWAYS. It is correct as a closure of the replica-role hole, and the swap check does what it says for a fence that is disabled or dropped: the read happens in the swap transaction after LOCK TABLE of source and shadow (cutover_swap.go#L63), so nothing can change the trigger between the read and the rename. The retry claim also holds: a run resumed after cutover-unfenced re-runs the whole verification pass (runner.go#L406-L426), and I confirmed with a probe that a row written while the fence was disabled makes that pass stop with a DivergenceError (source and shadow differ in 1 of 3 chunks) rather than reach the swap. Nothing here is blocking. The two findings are about how much of "the fence is still the engine's fence" the check actually reads, and which branches of it a test pins.

Invariants: extends CO-1 (the quiesced proof's quiescence is now confirmed again at the swap, not only installed before the copy); upholds LK-2 (the read runs inside the swap's bounded transaction and takes no lock of its own) and CO-9 (every catalog name in the new query is pg_catalog-qualified, so it does not depend on search_path).

Blocking

None.

Non-blocking

1. The swap's read matches the trigger by name, function, and statement kinds, but not by whether it fires for every statement, so a replaced fence the docs say is refused is not. writeFenceStateSQL joins on the trigger's name, its function's name and schema, and tgtype, and confirmWriteFence then checks only tgenabled (fence.go#L149-L167). A trigger of the same name replaced with WHEN (false), or with UPDATE OF <column>, or with arguments, has the same tgtype, the same function, and tgenabled = 'A', yet it does not refuse every write. refusal-classes.md and the CauseCutoverUnfenced comment promise the refusal when the trigger "was disabled, dropped, or replaced" (refusal.go#L88-L93). I ran it: after the verification pass, a table owner replaces the fence with a WHEN (false) one, inserts a row, and the run completes with that row gone from the live table. The caller who can do this owns the table, and the PR's own limits already say an owner can disable the fence, write, and restore it, so this is the same class and not a regression; it is non-blocking for that reason. But the read advertises "shape", and a one-column difference from the installed definition is cheap to cover. Suggested fix: also require the columns that describe when the trigger fires to be at their installed values in the same query (tgqual null, tgattr empty, tgnargs = 0, NOT tgdeferrable); I did not run that edit.

Test that fails on adb5a99

func TestRunnerRefusesTheSwapWhenTheFenceWasReplacedByAConditionalOne(t *testing.T) {
	f := newRunnerFixture(t)
	f.createOrders(t)
	opts := schemachange.RunOptions{AfterCheckpoint: func(_ context.Context, cp checkpoint.Checkpoint) error {
		if cp.Phase == checkpoint.PhaseCutover {
			f.exec(t, `CREATE OR REPLACE TRIGGER pgsprite_write_fence
				BEFORE INSERT OR UPDATE OR DELETE OR TRUNCATE ON %s.orders
				FOR EACH STATEMENT WHEN (false) EXECUTE FUNCTION pgsprite.pgsprite_write_fence()`)
			f.exec(t, `ALTER TABLE %s.orders ENABLE ALWAYS TRIGGER pgsprite_write_fence`)
			f.exec(t, `INSERT INTO %s.orders (id, qty) VALUES (999999, 1)`)
		}
		return nil
	}}

	_, err := f.run(t, "orders", `ALTER TABLE %s.orders DROP COLUMN note`, opts)

	require.Error(t, err)
	assert.Equal(t, schemachange.CauseCutoverUnfenced, schemachange.RefusalCauseOf(err))
	assert.True(t, f.hasColumn(t, "orders", "note"), "the source was not swapped out")
}

--- FAIL: TestRunnerRefusesTheSwapWhenTheFenceWasReplacedByAConditionalOne (1.16s) on adb5a99 (An error is expected but got nil.); it passes once the read rejects the conditional trigger.

2. The swap check's branches are pinned only for a disabled or dropped fence; the origin-only branch and each of the query's match conditions are not. The PR's two swap tests disable the trigger (tgenabled = 'D') or drop it. Nothing pins the other half of enabled != triggerEnabledAlways (fence.go#L161), which is the one that matters most to the PR's own story: a fence an older engine, or a later ENABLE TRIGGER, left enabled for origin sessions only ('O') does not fire for a replica-role writer, and that is exactly the hole being closed. The resume test reaches 'O' only through a reinstall, which repairs it before the swap ever looks. Changing the check to refuse only 'D' keeps every test in the package green. The same is true of the query's other conditions: relaxing the tgtype condition, the function-schema condition, the function-name condition, or the trigger-name condition (each independently, see the table below) also leaves everything green, because the only trigger in any test is the engine's own, with a name and function that match each other. Suggested fix: add the origin-only case below (it passes on adb5a99 and fails under the 'D'-only mutation), and, if finding 1 is taken, pin the shape conditions with a differently shaped replacement trigger as in that test.

Test that passes on adb5a99 and fails when the check refuses only a disabled fence

func TestRunnerRefusesTheSwapWhenTheFenceIsOriginOnly(t *testing.T) {
	f := newRunnerFixture(t)
	f.createOrders(t)
	opts := schemachange.RunOptions{AfterCheckpoint: func(_ context.Context, cp checkpoint.Checkpoint) error {
		if cp.Phase == checkpoint.PhaseCutover {
			f.exec(t, `ALTER TABLE %s.orders ENABLE TRIGGER pgsprite_write_fence`)
		}
		return nil
	}}

	_, err := f.run(t, "orders", `ALTER TABLE %s.orders DROP COLUMN note`, opts)

	require.Error(t, err)
	assert.Equal(t, schemachange.CauseCutoverUnfenced, schemachange.RefusalCauseOf(err))
	assert.Equal(t, "O", f.fenceEnabled(t, "orders"))
}

Passes on adb5a99; with if enabled != triggerEnabledAlways changed to if enabled == "D" it gives --- FAIL: TestRunnerRefusesTheSwapWhenTheFenceIsOriginOnly (2.10s).

Verified

• The PR's fence, refusal-table, and drop-fence tests pass on PostgreSQL 16 (go test ./pkg/schemachange/ -run 'TestRunner|TestDropWriteFence|Refusal', 103s); the replica-role, swap-refusal, and resume-upgrade tests also pass on 14 and 18.
• writeFenceTriggerType is the right tgtype for the installed trigger: BEFORE (2), INSERT (4), DELETE (8), UPDATE (16), TRUNCATE (32), row-level bit unset.
• CREATE OR REPLACE TRIGGER and ALTER TABLE ... ENABLE ALWAYS TRIGGER run in the same install transaction, so a resumed run never leaves the fence in between; a fence left 'O' or 'D' comes back 'A'.
• CauseCutoverUnfenced is in RefusalCauses(), Invariant() (CO-1), and Class() (environmental), and the two completeness tests in refusal_test.go carry it. The live swap does not set requireWriteFence (runner_live.go#L74), so a decoding run is unchanged.
• A write made while the fence was disabled, before the refused swap, stops the resumed run's verification pass with a DivergenceError (probe, passes), so the cutover-unfenced remedy text in refusal-classes.md and schemabot-integration.md is accurate.

Mutant	Caught by
Remove the ENABLE ALWAYS statement from the install	replica-role test, resume-upgrade test, and every test that reaches the swap (the swap refuses an origin fence)
Drop .withWriteFence() at the Runner's Cutover call	the disabled-fence and dropped-fence tests
Invert !ready.verified.Live() at the swap	the disabled-fence and dropped-fence tests
Refuse only tgenabled = 'D' instead of anything but 'A'	survived (finding 2)
Make the tgtype condition always true	survived (finding 2)
Make the function-schema condition always true	survived (finding 2)
Drop the function-name condition alone	survived (finding 2); the trigger and its function share a name, so the trigger-name condition covers it
Drop the trigger-name condition alone	survived (finding 2), for the same reason

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

[aparajon]

🤖 Review 2/2 of adb5a99: OSS adoption and SchemaBot integration lenses, 0 blocking, 3 non-blocking.

This is the second of two comments; the correctness pass is the one above. The PR adds one exported refusal cause, schemachange.CauseCutoverUnfenced (cutover-unfenced, class environmental, CO-1), and keeps everything else on the exported surface where it was: CutoverOptions gains only an unexported field, Cutover and Run keep their signatures, and no plan JSON field or capability row moves. An adapter that switches on RefusalCause sees one new value, and the documented handling for it (retry Run, which reinstalls the fence and re-verifies) is bounded and safe for an adapter that retries environmental refusals. A standalone user sees no new flag or configuration. The behavior they can notice is that writers which used to slip past the fence now get 55006.

Blocking

None.

Non-blocking

1. Docs name the replica-role session but not the writers that run in it, which is what an operator will actually meet. engine-role.md#L103 now says the fence fires for a session running under session_replication_role = replica. The writers that run in that role are the ones worth naming: a logical-replication subscription's apply worker writing into the source table, and a bulk restore run with triggers disabled. Before this PR their writes reached the source silently, and the swap then dropped them with the old table; now they are refused. For a subscription apply worker that means an erroring worker that retries, with the publisher retaining WAL for the slot until the run is abandoned or completes, which is a different operational picture from "a session is refused". This is reading the PostgreSQL documentation of ENABLE ALWAYS, not a reproduction. A sentence in the "write fence" section of engine-role.md, and one in the DivergenceError row of schemabot-integration.md that currently points an operator only at "a role that owns the table and disables it", would let an operator connect a stuck subscription to the fence.

2. The fence's strength is stated in three places with three wordings. The limit reads "a role that owns the table can disable it, write, and restore it" in the Runner doc (runner.go#L138-L149), the CO-1 entry, and engine-role.md, while refusal-classes.md and CauseCutoverUnfenced say the refusal covers a fence "replaced" (see finding 1 of the correctness comment: a replaced trigger of the same shape is not seen). An integrator deciding whether a deployment can trust the fence reads whichever page they land on. If finding 1 is taken, the wording agrees by itself; if it is not, the refusal text should say "disabled, dropped, or replaced by a trigger of a different name, function, or statement kinds", which is what is checked.

3. Prose formatting. The edits leave several lines far past the wrap width the surrounding text keeps: engine-role.md#L103 (122 characters) and #L118 (119), a short ragged tail in doc.go#L23 (there. A table), the swapOnce comment, and On a decoding target it runs CheckLive under in the CO-1 entry at invariants.md#L71, which is now a one-clause line between two full ones. The CO-1 entry also grows by about ten lines in what was already one paragraph of dozens; the new swap-time check and its stated limit would read better as their own sentence group than as clauses inside the fence sentence. Content is right throughout; this is purely for the reader following CO-1.

Verified

• The exported surface changes by exactly one constant (CauseCutoverUnfenced); RefusalCauses() and the two completeness tests carry it, and refusal-classes.md and schemabot-integration.md each gain its row.
• The remedy rows are accurate: a run resumed after cutover-unfenced reinstalls the fence, re-verifies the whole table, and a row written while the fence was down makes that pass stop with a DivergenceError (probe, passes on adb5a99).
• A fence left origin-only by an older engine on a stopped run is upgraded on resume, and the resume re-verifies, so an upgrade of the engine mid-run opens no gap (resume-upgrade test passes on 14, 16, and 18).
• requireWriteFence is unexported, so a library caller that quiesces the table by its own means and calls Cutover directly is unaffected, as the PR states. A caller who installs an equivalent fence of their own has no way to opt in; that is by design here and not something to add before someone asks.
• docs/low-level-design.md line 770, which mentions writes under session_replication_role = replica bypassing capture, is about the live capture path and is not touched by this change.

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

[aparajon]

🤖 Stamping with comments: 0 blocking, 5 non-blocking (see the two review comments above).

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