docs: record copy-and-swap v1 design decisions

[Kiran01bm]

Records the v1 design decisions for the copy-and-swap strategy in a new docs/copy-and-swap-design.md and amends the existing docs that said something different, so the implementation PRs that follow build against one agreed shape. Docs only; no code or behaviour change.

Why

The low-level design left several copy-and-swap questions open (durable scratch database or not, when to build shadow indexes, where checkpoints live, which primary-key shapes are admitted, how sequences and identity columns are handed off, how unchanged TOAST values are preserved, how divergence is handled). Building the strategy as parallel leaf packages only works if those are answered once, up front, and written down where the code reviews can cite them. This PR is that ratification. Where a decision contradicts existing prose (AGENTS.md, the LLD, engine-role.md, the change-capture trade-off), the prose is amended in the same change so the docs do not disagree with each other.

Every PostgreSQL-specific claim in the decisions was checked against a live server; the ones the server refuted were corrected rather than softened.

What

New docs/copy-and-swap-design.md:

• v1 scope table — what the strategy admits and what it refuses with a typed reason: copy-and-swap-pk-unsupported, -replica-identity, -foreign-keys, -triggers, -partitioned, -dependent-views, -publication-member, -name-length, -logical-decoding-unavailable, -slot-headroom, -disk-headroom, -grants. Dependent views and publication membership are the two RF-2 dependents a rename swap strands; the table names both so the vocabulary and RF-2 agree.
• Decisions D1–D15, each with decision / why / alternative considered / where enforced:
  • D1 no durable scratch database — shadow is CREATE TABLE … (LIKE <source> INCLUDING ALL EXCLUDING IDENTITY) in the source schema under SET ROLE <owner>, then the gated ALTER TABLE is executed against the empty shadow. The statement reaches the shadow by one edit at the parse boundary: pkg/statement retargets the relation to the shadow name and deparses (the same single-field-and-deparse shape the CONCURRENTLY rewrite uses), and the executor re-verifies the result against the gated statement before running it — ST-7 with the shadow as the sole permitted target. A search_path approach was considered and rejected because it cannot handle schema-qualified statements and would turn the swap into a SET SCHEMA.
  • D2 indexes and constraints built up front, stating what INCLUDING ALL carries (defaults, constraints, indexes, per-column storage, column/constraint/index comments) and what the shadow builder must replicate explicitly under ST-5 (owner, ACLs, row-level security, reloptions, table comment).
  • D5 explicit sequence and identity handoff inside the cutover transaction: a shared serial/nextval sequence keeps its name and is re-owned; identity is re-added on the live table with the source sequence's options and its exact (last_value, is_called) copied with setval, which is defined on a never-advanced sequence where RESTART WITH last_value + 1 is not.
  • D6 omitted TOAST values are preserved by column-wise apply. pgoutput sends the unchanged-TOAST marker under REPLICA IDENTITY FULL as well as DEFAULT (the column is present with type byte u and no value, not omitted), so FULL is admitted but is not a way around the marker.
  • D8 deterministic bounded names, including the rename that restores the user's index and constraint names on the live table after LIKE derived the shadow's from the shadow's name — so ON CONFLICT ON CONSTRAINT u_slot keeps working and a second change of the same table derives the same names.
  • D13 unique-secondary-key moves are recovered batch-wide: key-targeted upserts over the per-key buffer, and on 23505 a savepoint rollback and delete-all-then-insert-all. Per-key delete-then-insert pairs are rejected because a cyclic exchange ({1→'B', 2→'A'} on a UNIQUE column) collides in both orders; that vector is now CO-6's test obligation.
  • D3 checkpoints in the target database; D4 single integer-family PK only; D7 checksum through the destination types; D9 drop the old table after commit by default (--keep-old opts out); D10 cut over as soon as the gate passes; D11 bound and reap logical-decoding state; D12 throttle by chunk time and slot lag; D14 divergence policy is explicit; D15 capture with pgoutput.
• Package and proof-type map for dbconn, preflight (CopySwapTarget, the route's proof), copier, checksum, decode, applier, checkpoint, schemachange, with invariant columns that agree with SAFETY.md and tcb-model.md.
• Cutover transaction, step by step — the lock_timeout retry/backoff for ACCESS EXCLUSIVE, the rule that no checksum runs under the lock, the dependent-name restoration, the sequence handoff, and the catalog recheck before commit.
• Deferred alternatives table.

Amendments to existing docs:

• AGENTS.md and .agents/checks/review.md: the "shadow-table DDL … derived by execute-and-introspect" rule now names the empty shadow and the transaction-scoped scratch schema and links D1; the review check names the single relation retarget as the only permitted AST edit.
• docs/low-level-design.md: scratch-database section and decisions §1/§2 rewritten to match D1; the throttler row of the package diagram matches D12.
• docs/engine-role.md: the tier-4 "planner scratch database" row is replaced by a note that copy-and-swap adds no privilege tier and needs no CREATEDB.
• docs/change-capture-tradeoff.md and docs/high-level-design.md: v1 implements logical decoding and refuses clusters without it; triggers stay the documented alternative with implementation deferred (D15).
• docs/invariants.md: new CO-8 "A TOAST-omitted column is never overwritten" (both replica identities; test obligation runs under both); CO-6 records the decided semantics and its refuting test vector; ST-1 reworded to one checkpoint row per (schema, table); ST-6 no longer lists a durable scratch database; ST-7 lists the planned shadow-retarget enforcement site.
• SAFETY.md and docs/tcb-model.md: pkg/applier gains CO-8; CopySwapTarget joins the proof-type registries; the core dependency list records the decision to admit jackc/pglogrepl (pinned) for pkg/decode — streaming-replication protocol and pgoutput decoding are load-bearing wire-protocol expertise under the same rubric as the parser. It is confined to pkg/decode and is not added to go.mod until that package's implementation lands.
• docs/architecture.md and docs/design-principles.md: package rows and principles that still described composite chunk keys, replica-lag throttling, or deferred cutover as v1 behaviour now match D4, D10, and D12.
• docs/README.md: index row for the new document.

Verification

• make test-unit green (the docs tests that pin proof-type and invariant lists still pass).
• Every relative Markdown link and #anchor across the tree resolves after the D13 retitle.
• Live PostgreSQL 17 checks behind the corrections: the {1→'B', 2→'A'} exchange raises 23505 for a delete-then-insert pair in either order and converges under batch-wide delete-then-insert; LIKE … INCLUDING ALL names the shadow's indexes <shadow>_pkey/<shadow>_<col>_key and ALTER INDEX … RENAME restores the constraint name with the index; ALTER SEQUENCE … RESTART WITH NULL is a syntax error while setval(seq, last_value, is_called) reproduces a never-advanced sequence exactly; renaming an identity sequence is permitted.
• Read alongside the contracts PR: the package and proof-type map here matches the types that PR declares.

---

Authored with Amp (Claude Opus 4.5).

[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 — the decided design (5b8a4855, 13 files, +448/−109 over fd4bcb17)

Recording the fifteen decisions as one authoritative page, each with its rejected alternative and its enforcement site, is the right move at this point in the build — most of these are choices a reader would otherwise have to reconstruct from three narrative documents, and several (D1's execute-and-introspect against the empty shadow, D5's is_called handling, D13's convergence argument) are decisions I would not have expected to be right on the first pass and are.

A docs PR has no code to mutate, so I did the equivalent: I took the load-bearing PostgreSQL claims and tried to falsify them against a live PostgreSQL 16 (the repo's own compose instance). Most held — ALTER INDEX … RENAME does rename the backing constraint (D8), reloptions, RLS and the table comment really are not carried by LIKE (D2), a never-advanced sequence really does report (start, false) from the sequence relation (D5). Three did not, and they are the first three findings below.

claim probed                                                  result
D8: LIKE derives the shadow's index names from the shadow      confirmed — and it discards the source's
D2: LIKE INCLUDING ALL does not carry reloptions/RLS/comment   confirmed
D8: ALTER INDEX … RENAME renames the constraint with it        confirmed
D6: v1 never changes the user's replica identity               FALSIFIED — LIKE does not carry relreplident
D2: the shadow reaches the gate with the source's shape        FALSIFIED for CHECK … NOT VALID
D5: a never-advanced sequence keeps its start value            confirmed, but only for one of three reads

The swap silently reverts a user's REPLICA IDENTITY FULL to DEFAULT (med)

D6 (:125) states that v1 "never changes the user's replica identity". LIKE … INCLUDING ALL does not copy relreplident, and it is not in D2's explicit-replication list (:56), not in the ST-5 fidelity list (invariants.md:274), and not in the checklist ST-5 cites (low-level-design.md:527):

ALTER TABLE src3 REPLICA IDENTITY FULL;
CREATE TABLE shadow3 (LIKE src3 INCLUDING ALL EXCLUDING IDENTITY);

 relname | relreplident |   reloptions
---------+--------------+-----------------
 shadow3 | d            |                    <- default
 src3    | f            | {fillfactor=70}

So the rename swap leaves the live table on DEFAULT. That is not cosmetic: a user on FULL usually has it because something else consumes their table's changes, and the fidelity gate that exists to stop exactly this class of silent post-swap difference does not look at it. Since v1 accepts both DEFAULT and FULL as valid input shapes, one of the two accepted shapes is not preserved. Adding relreplident to D2's replicate-explicitly list and to ST-5's fidelity list closes it, and it is one ALTER TABLE … REPLICA IDENTITY on the shadow.

A source CHECK … NOT VALID makes the copy fail mid-flight (med)

LIKE … INCLUDING ALL copies a NOT VALID check constraint as a validated one, and the copier's own insert shape then rejects the very rows the source legally holds:

-- source: a violating row, then the constraint added NOT VALID (a normal way to land one)
INSERT INTO src2 (amount, slot) VALUES (-5.00,'A'), (10.00,'B');
ALTER TABLE src2 ADD CONSTRAINT amount_positive CHECK (amount > 0) NOT VALID;

source constraint:  amount_positive  convalidated = f
shadow constraint:  amount_positive  convalidated = t     <- LIKE validated it

INSERT INTO shadow2 (id, amount, slot) SELECT id, amount, slot FROM src2 ON CONFLICT (id) DO NOTHING;
ERROR:  new row for relation "shadow2" violates check constraint "amount_positive"
DETAIL:  Failing row contains (1, -5.00, A).

This is ST-6's own stated failure mode — "failing hours into a copy on something knowable up front is a bug" — and it is knowable up front from pg_constraint.convalidated. The refused-operations table covers lossy conversions the user asks for; this is a pre-existing source state that the shadow builder converts into a stricter constraint on its own. Two ways out, and the second seems better: refuse with a typed reason, or have the shadow builder re-add the constraint NOT VALID so the shadow carries the source's actual semantics (which is also what ST-5 fidelity should compare — otherwise the swap quietly marks validated a constraint the user deliberately left unvalidated).

D13's 23505 recovery destroys the values D6 exists to preserve (med)

D6/CO-8 make column-wise UPDATE apply an invariant: a buffered image derived from an UPDATE that left a TOASTed column alone carries no value for that column, and inventing one "would overwrite live shadow data and silently break convergence". D13 (:236) then recovers from 23505 by reapplying the batch "as delete-all-then-insert-all: it deletes every key in the batch, then inserts every surviving image."

An insert cannot be column-wise. For any key in that batch whose image came from a TOAST-omitting UPDATE, the recovery path has to invent a value for the omitted column — and the row that held the real one was deleted earlier in the same transaction, so it cannot read it back. The outcome is either a NOT NULL failure or exactly the silent overwrite CO-8 forbids, for every key in a batch that contained one unique-key move.

The two decisions are each internally sound; it is the composition that breaks, and neither test obligation can catch it: CO-6/D13's fixed vector is seats(id int PRIMARY KEY, slot text UNIQUE), which has no TOASTable column, and CO-8's vector is a quiet update with no unique-key move. Worth stating how the recovery reconstructs an omitted column — DELETE … RETURNING the old image and merging, or restricting the delete-all-then-insert-all set to keys whose unique values actually moved — and adding a TOASTed column to the D13 vector so the interaction is exercised rather than assumed.

CO-4's watermark discard is stated per event, but a key-moving update spans two keys (med)

CO-4 (invariants.md:70) allows captured changes above the copier's watermark to be discarded for a monotonic integer PK, "the copier will read the current row anyway". ChangeEvent in the sibling contracts PR already carries OldKey *int64 — "non-nil only when an update moved the key" — and for a key-moving update the two keys can land on opposite sides of the watermark:

watermark = 1000
UPDATE t SET id = 5000 WHERE id = 5     ->  Key=5000 (above: discardable)
                                            OldKey=5 (below: its deletion MUST be applied)

Discarding on Key drops the deletion of key 5, the copier never revisits that range, and the shadow keeps a row the source no longer has. The checksum catches it, but CO-4's premise is that the protocol converges without the checksum. The discard decision needs to be per key rather than per event; more generally, nothing in CO-4, CO-5 or D13 says what the applier does with OldKey at all, even though the type already commits to carrying it — the buffer's "one entry per PK" rule has to become a deletion for OldKey plus an image for Key, and that is worth pinning where the ordering races are enumerated.

D8's name restoration needs a pairing rule, because LIKE preserves no index or constraint name (low)

D8 says LIKE derives the shadow's index names from the shadow's name and cutover then renames "the shadow's corresponding dependent … to the name the source dependent held". The first half is true in a stronger sense than the text implies — the source's own names are discarded entirely, so there is no name-derived mapping back:

source indexes:  i_amount_a  i_amount_b  src2_pkey  u_custom
shadow indexes:  shadow2_amount_idx  shadow2_amount_idx1  shadow2_pkey  shadow2_slot_idx

named UNIQUE constraint slot_uniq on the source  ->  shadow_slot_key on the shadow
CHECK constraint amount_positive                 ->  amount_positive (names are kept)

So "corresponding" has to be established by definition — columns, expression, uniqueness, predicate, opclass — and for two indexes with the same definition (i_amount_a/i_amount_b above) it cannot be established at all, though there an arbitrary pairing is harmless because the two are interchangeable. Saying that explicitly makes the D8 promise checkable, and it is the step on which ON CONFLICT ON CONSTRAINT u_slot "keeps working" depends. One dependent is missing from the rename list: INCLUDING ALL also copies extended statistics, and they are named from the shadow (src_stats on the source became shadow_amount_slot_stat), so the post-swap live table wears a _pgsprite_<hash>_new-derived statistics name unless cutover renames them too.

Three notes

D5's (last_value, is_called) read only works from one of the three obvious sources. The sentence at :108 is about the never-advanced case, and that is exactly where the three reads disagree:

SELECT last_value, is_called FROM src3_id_seq;                   ->  1 | f     (what D5 needs)
SELECT last_value FROM pg_sequences WHERE …;                     ->  NULL     (no is_called column at all)
SELECT pg_sequence_last_value('src3_id_seq'::regclass);          ->  NULL
SELECT setval('src3_id_seq', NULL, false);                       ->  NULL, and a silent no-op

Since setval is strict, a pg_sequences-based implementation silently does nothing and the claim in the doc quietly stops being true. Naming the read ("from the sequence relation, not pg_sequences") costs a clause and pins the one detail that makes the paragraph correct.

D5 promises the post-swap table "preserves generation kind, sequence name, and the source's exact issued-value state", but nothing renames the new identity sequence. ADD GENERATED … AS IDENTITY picks the name itself, which matches only when the source's identity sequence had the default name; a user who renamed theirs gets a differently-named sequence after the swap. D8's index handling already has the shape this needs — rename it to the name the source's held, in the same step.

CO-8's "Under either PK-based replica identity (DEFAULT or FULL)" reads oddly, since FULL is not PK-based — the v1 scope line means "a table with a usable PK, whose replica identity is DEFAULT or FULL". The invariant's substance is right and D6's "FULL enlarges only the old tuple" is the precise statement; it is just this one phrase.

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

[aparajon]

🤖 Review 2/2 — the doc set and the guards that pin it

The other half of this PR is a sweep: fifteen decisions land, and every narrative page that promised something else has to stop promising it. That is the part most likely to be left half-done, so I checked it mechanically rather than by reading. Two results first, because both are verifiable:

• Every markdown link in the changed files resolves, in both directions — 0 broken anchors outbound, 0 broken inbound references to the renamed #plan-time-prerequisite-… heading. The pg_sprite_scratch / CREATEDB retirement is complete: no stale reference survives anywhere in the repo.
• Terminology is clean: the diff adds zero occurrences of the banned word and removes six, and the new page has none.

Merging this with the contracts PR produces two conflicting registry rows, with no conflict marker (med)

docs/tcb-model.md, SAFETY.md and .agents/checks/review.md are all edited by both this PR and #87, from the same base. SAFETY.md conflicts, so it will get attention. docs/tcb-model.md auto-merges — into two CopySwapTarget rows that disagree with each other:

$ git merge-tree --write-tree pr87 pr88
CONFLICT (content): Merge conflict in SAFETY.md
Auto-merging docs/tcb-model.md          <- no conflict reported

$ git show <merged-tree>:docs/tcb-model.md | grep -n CopySwapTarget
94: | table name (copy-and-swap target) | preflight (planned) | `CopySwapTarget` — … the [v1 scope]
      checks … the shadow builder will accept **only** this type | ST-6, RF-1..RF-3 …
97: | table name (copy-and-swap target) | copy-and-swap preflight | `CopySwapTarget` (carries the
      integer PK and owner facts) | ST-6 for copy-and-swap |

Nothing catches this. TestDocsListEveryProofType asserts presence, so a duplicated entry passes it twice over, and the two rows even claim different invariants (ST-6 vs ST-6, RF-1..RF-3). Whichever of the two PRs merges second needs a manual reconciliation of docs/tcb-model.md even though git reports it clean — worth deciding the order and saying so on both PRs, since this row is the one a reviewer is meant to consult to learn what CopySwapTarget proves.

CI runs no docs guard on a docs-only PR (med)

The repo has seven docs_test.go guards pinning prose against code, and pkg/preflight/docs_test.go reads exactly the three registries this PR rewrites. None of them ran here:

$ gh pr checks 88
test (PostgreSQL ${{ matrix.pg }})   skipping
lint                                 skipping
build                                skipping

.github/workflows/ci.yml's changes job classifies a file as code only when it is not **/*.md and not docs/**, and every downstream job is gated on code == 'true'. The filter itself is carefully built (the predicate-quantifier: every comment is exactly right, and the fail-open on a filter error is the right default), but the consequence is that the guards which exist to stop prose from drifting are switched off precisely on the PRs that change prose. A docs-only PR could delete PreflightedTable from all three registries and go green.

I ran them locally against this branch and they pass, so nothing is broken here — but that is a fact about this PR, not a property CI holds. AGENTS.md's coverage invariant ("no behavior lands without a test that would fail without it") applies to the guards themselves: the cheapest fix is a docs-only leg that runs the docs_test.go packages, or treating **/*.md as code for the test job alone.

The sweep leaves standing the two claims that most directly contradict the new decisions (med)

The stale claims that remain are not evenly distributed — the ones left behind are the ones about replica identity and unchanged TOAST, which is the subject of the new CO-8:

Location	Still says	New decision
low-level-design.md:497	REPLICA IDENTITY is required for "unchanged-TOAST columns"	D6: replica identity is unrelated to the marker
low-level-design.md:517	the applier must "carry forward, or use REPLICA IDENTITY FULL"	D6 rejects that alternative by name
invariants.md:74	changes above the watermark "must be queued for composite/non-comparable PKs"	those PKs are refused in v1
invariants.md:85	CO-5 pins the "map ↔ FIFO-queue" mode transition	same
low-level-design.md:589	without wal_level=logical "the engine must fall back to trigger-based CDC"	typed refusal; triggers deferred
docs/README.md:85	"logical decoding as the primary implementation and a trigger-based fallback"	same
mysql-vs-postgresql.md:180	"composite-PK chunker otherwise"	refused with copy-and-swap-pk-unsupported
schemabot-integration.md:59, :382	"cutover (+ deferred cutover)"; "the trigger fallback will all live behind Apply/Stop/Cancel"	D10, D15

The first two matter most. low-level-design.md:517 tells a future implementer to reach for REPLICA IDENTITY FULL to solve unchanged TOAST, which D6 records as an alternative considered and rejected — it "does not remove the marker; it only increases WAL and mutates user configuration". Leaving both texts in the set means the doc that is easier to find gives the answer the decision rejected. The CO-4/CO-5 queue clauses are the other kind: two invariants that now require an applier mode this PR refuses, in the registry that is supposed to be true. Scoping them ("when queue mode lands") keeps them honest without deleting the future design.

docs/README.md:85 and the two schemabot-integration.md lines are in the same category and cheap. docs/capabilities.md is deliberately excluded and the PR says so up front, which is the right call — the one line worth a second look is capabilities.md:213's "scratch-database mechanics", which is the last echo of a prerequisite that no longer exists rather than a shipped-behavior claim.

CO-8 reaches the applier's rows but not the decode ones (low)

CO-8's own Enforced: line names two sites: "pkg/decode per-column presence on ChangeEvent, pkg/applier column-wise UPDATE construction from it". The applier gained CO-8 in all three places it is listed; pkg/decode gained it in none — SAFETY.md:26 still reads ST-4, CO-4, tcb-model.md's decode row is unchanged, and this PR's own package map at copy-and-swap-design.md:288 lists ST-3, ST-4, CO-4. Since presence decoding is the half of CO-8 that cannot be recovered later — an event that loses the marker cannot be un-lost by the applier — that is the row where a future reader most needs to see it.

pkg/table and pkg/copier now both own the chunker (low)

The new package map assigns Chunk and Watermark to pkg/copier, and the contracts PR puts them there. architecture.md:197 still gives the chunker its own pkg/table row — and this PR edited that row, so the package name was in view. SAFETY.md's package table has no pkg/table row at all. Either fold it into pkg/copier or say what the split is; right now the answer depends on which page you open.

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

[aparajon]

🤖 Approving. The decisions page is the right artifact and the fifteen entries are well-reasoned; my findings are all about the composition of two decisions or about a page the sweep did not reach, and none of them change what this PR decides.

Highest-value items, all verified against a live PostgreSQL 16 or with git merge-tree: LIKE … INCLUDING ALL does not carry relreplident, so the swap silently reverts a user's REPLICA IDENTITY FULL against D6's promise (ST-5's fidelity list does not cover it); a source CHECK … NOT VALID is copied as validated and the copier's insert then fails on rows the source legally holds (ST-6's own failure mode, knowable from pg_constraint.convalidated); D13's delete-all-then-insert-all recovery cannot reconstruct the TOAST-omitted column CO-8 requires be preserved, and neither test vector can catch it; and merging this with #87 auto-merges docs/tcb-model.md into two conflicting CopySwapTarget rows with no conflict marker.

Details in the two review comments above.

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

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — block/pg-sprite pull/88, follow-up commit

#	Concern	Status
1	Blocking: D13's delete-all-then-insert-all fallback inserts whole rows, but D6/CO-8 make UPDATE images partial — an unchanged-TOAST u marker has no value to re-insert	fixed — D13 now specifies two rules and CO-6 mirrors them. (a) The CO-5 buffer merges rather than replaces: a newer UPDATE overlays only its present columns onto the buffered image, so a marker survives dedup only when no buffered image ever carried that column's value, i.e. the row pre-existed on the source before the batch (an INSERT-then-partial-UPDATE inside one batch therefore never yields a marker). (b) Before the fallback deletes anything it completes every surviving marker from the current shadow row (SELECT … FOR UPDATE on the affected keys, same transaction, after the savepoint rollback) — by CO-8 the shadow's stored value is exactly what the marker stands for. A marker-bearing image with no shadow row is an invariant violation, not a case: such a row pre-exists, so its key is either above the watermark (discarded under CO-4 before buffering) or inside an in-flight chunk (whose flush CO-4 already defers); the applier aborts fail closed. The "restrict the fallback to complete batches" alternative is recorded as rejected because a cyclic exchange touching a TOASTed row would then have no converging path. CO-6 gains a second test vector: a ≥8 KiB TOASTed column untouched by both updates must survive the fallback byte-for-byte
2	CO-4 still says above-watermark changes "must be queued for composite/non-comparable PKs"; CO-5 still mandates the map↔FIFO dual store — both contradict D4	fixed — CO-4 states discard as sound because of D4's integer-family key and says composite/non-comparable keys are refused, not queued. CO-5 is a single keyed map; Spirit's mode toggle is recorded as having no v1 counterpart, returning only if queue mode is ever built. The low-level-design ordering section already said "refused in v1" and is unchanged
3	low-level-design offers REPLICA IDENTITY FULL as an unchanged-TOAST remedy; D6 rejects it	fixed — the caveat now states the marker appears under either PK-based identity, the applier carries the value forward column-wise (D6, CO-8), FULL enlarges only the old tuple and is not a remedy, and the engine never changes the user's replica identity
4	.agents/checks/review.md proof-type list omits CopySwapTarget	fixed — added between CreationRole and VerifiedShadow. pkg/preflight/docs_test.go pins only the three exported proof types; CopySwapTarget is planned, so the test is unchanged and will pick it up when the type lands and is added to the slice
5	CO-8 missing from pkg/decode in SAFETY.md, tcb-model.md, and the copy-and-swap package map	fixed — all three rows now read ST-4, CO-4, CO-8 (the design-doc row also keeps ST-3); SAFETY.md and tcb-model.md row text names per-column presence as the decode responsibility that earns it
6	Trigger fallback half-demoted: docs/README.md, design-principles.md, high-level-design.md, low-level-design.md (rds.logical_replication bullet), change-capture-tradeoff.md table still present triggers as available	fixed — each now says logical decoding is the v1 path, clusters without it get copy-and-swap-logical-decoding-unavailable, and trigger capture is the documented, deferred alternative behind the same seam, linking D15. Also swept vision.md, schemabot-integration.md ("any later trigger-capture implementation"), testing.md and invariants.md LK-2 ("if trigger capture is ever built"). The low-level-design heading "1. CDC mechanism — logical decoding with trigger fallback" is kept because its body already opens with "trigger capture is deferred" and the anchor is linked from the TOC and change-capture-tradeoff.md
7	Stale headings: ST-1 "single row"; AGENTS.md "never by AST transformation" lacks the D1 single-relation-retarget carve-out	fixed — ST-1 is "one row per target, written atomically"; AGENTS.md now names pkg/statement Qualify's single-relation retarget as the one permitted AST edit, which changes no semantics
G1	reader-lag throttling survives in low-level-design ("Aurora-aware" paragraph, package tree)	fixed — both say slot-lag throttling with replica-lag deferred, linking D12; the component diagram at L116 already said "(replica lag later)"
G2	--defer-cutover in schemabot-integration.md verb table; CO-2's "continuous (deferred-cutover) checker"	fixed — the verb row is "cutover: the gated atomic swap, taken as soon as the gates pass", noting deferred cutover is not in v1 (D10); CO-2 now describes the continuous checker as the pre-cutover re-check loop, with deferred cutover as a hypothetical longer run of the same checker
G3	NAMEDATALEN wording — the limit is 63 = NAMEDATALEN − 1	fixed — "63-byte identifier limit (NAMEDATALEN - 1)"
G4	design-principles "Temporary slot" contradicts D11's durable resume	fixed — "a durable, name-prefixed slot that resume can reattach to", linking D11
G5	stale Go comment pkg/statement/desired.go ("scratch database's job")	fixed — "execute-and-introspect's job"; comment-only change, no behavior
G6	engine-role CREATE on the database for the scratch schema — stated in low-level-design but absent from the tier table and pkg/preflight/privileges.go	noted, not fixed here — engine-role.md now states the requirement beside the tier table and records that neither the table nor the privilege probe carries it yet, as open follow-ups. It is pre-existing and applies to every declarative plan, so the probe change belongs in its own code PR with a test, not in this docs PR

[aparajon]

🤖 Re-review 1/2 — the completion rule (315e897a, 16 files +107/−57 over 5b8a4855)

The D6/D13 collision from the last round is answered properly, and the answer is the right shape: the fallback inserts whole rows, so the two new rules say where a complete row comes from — CO-5 merges instead of replacing, and anything still carrying a marker is completed from the shadow before the batch-wide delete. Extending the Why to say why the alternative (restrict the fallback to already-complete batches) is worse — a cyclic exchange on a TOASTed row would then have no converging path at all — is the part that makes this a decision rather than a patch.

One of the two rules rests on a claim about pgoutput that is not true, and it is the claim that turns a live case into an abort.

An UPDATE that moves the primary key produces exactly the image the doc calls impossible (blocking)

docs/copy-and-swap-design.md:251-256:

A marker-bearing image whose shadow row is absent is a protocol error, not a case to handle: the row pre-exists on the source, so its key is either above the copier watermark (discarded under CO-4 before buffering) or inside an in-flight chunk (whose flush CO-4 already defers until the chunk lands). The applier aborts the change fail closed if it observes one.

The enumeration is complete for a row whose key does not change. It omits the case where the row pre-exists under a different key. I probed it on a real PostgreSQL 16 with wal_level = logical — one table, one out-of-line column, one UPDATE that moves the PK and never touches that column:

CREATE TABLE seats (id int PRIMARY KEY, slot text UNIQUE, payload text);
ALTER TABLE seats ALTER COLUMN payload SET STORAGE EXTERNAL;
INSERT INTO seats VALUES (5,'A',repeat('x',20000)), (2,'B',repeat('y',20000));
UPDATE seats SET id = 6 WHERE id = 5;      -- moves the PK, payload untouched
UPDATE seats SET slot = 'C' WHERE id = 2;  -- plain update, payload untouched

decodes to

UPDATE: old-key: id[integer]:5 new-tuple: id[integer]:6 slot[text]:'A' payload[text]:unchanged-toast-datum
UPDATE:                                    id[integer]:2 slot[text]:'C' payload[text]:unchanged-toast-datum

The first record is a marker-bearing image keyed 6. Key 6 has never existed on the source, so the shadow has no row 6, and nothing about that is a protocol error — it is a single legal statement. Walk it through the new rules:

• CO-5's merge (invariants.md:88-91, restated at copy-and-swap-design.md:245-247) leaves the marker in place: no buffered image for key 6 ever held the column's value. So the parenthetical that follows — "that is, the row already existed on the source before the batch" — is false here. The equivalence it asserts between "no buffered image ever held the value" and "the row pre-existed on the source" is exactly what a key move breaks, and CO-5 states it unconditionally, independent of the fallback.
• The completion read then targets key 6 in the shadow, finds nothing, and by copy-and-swap-design.md:255-256 / invariants.md:113-115 the applier aborts.

Fail-closed is the right default and this is not a corruption path, so it is not a safety hole. It is a correctness hole in the decision: a workload that moves a primary key on a row with a TOASTed column will abort the change, and the doc forecloses handling it by name, so whoever implements Phase 6 will implement the abort deliberately.

The value is not missing — it is in the shadow row for the old key, and #87 landed the field that addresses it: pkg/decode/types.go:50, OldKey *int64, "non-nil only when an update moved the key." Completion runs before the fallback deletes anything, so the old key's shadow row is still present at that point; reading the marked columns from OldKey's row when the image carries one closes the case without inventing a value, and keeps SELECT … FOR UPDATE in the same transaction as written. The remaining absent-row case after that really is a protocol error, and the fail-closed sentence can stay for it.

This is the same blind spot as the CO-4 finding from the last round, which is still open: invariants.md:73-78 narrows why above-watermark discard is sound (D4, monotonic integer key — a good narrowing) but still states the discard per captured change. For UPDATE t SET id=5000 WHERE id=5 with the watermark at 1000, the new key is above and discardable while the old key's deletion is below and must be applied; discarding the change strands a row the source no longer has, and CO-4's premise is convergence without the checksum. OldKey is now on main with a doc comment and no entry in the registry or the package map saying what the applier does with it — the one type in the copy-and-swap contracts whose semantics the doc set does not cover.

The test vector's threshold is not the property it needs (low)

invariants.md:118-119 asks the second vector for "a ≥8 KiB TOASTed column left untouched by both updates." Size is not the criterion — out-of-line storage is, and a large value is not necessarily stored out of line. My first attempt at the probe above used the default EXTENDED storage, and 20 000 bytes of repeat('x', 20000) compressed inline and decoded as the full value, with no marker at all:

UPDATE: old-key: id[integer]:5 new-tuple: id[integer]:6 slot[text]:'A' payload[text]:'xxxxxxxxxx…'

Only after SET STORAGE EXTERNAL did the same statement emit unchanged-toast-datum. So a fixture written to the letter of "≥8 KiB TOASTed column" can pass while never exercising CO-8 once — the vector would assert the value survives a path it never took.

internal/testutil on main already gets this right, which is the cheap fix: workload.go:49 forces ALTER COLUMN blob SET STORAGE EXTERNAL, and ToastBytes() at :54-58 exists, in its own words, "so a test can prove that the blob column really is stored out of line." Stating the obligation as out-of-line storage proven by that helper, rather than as a byte count, makes the vector honest about what it covers.

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

[aparajon]

🤖 Re-review 2/2 — the merge, the links, and the sweep

The duplicate registry row predicted last round is now real (med)

#87 merged at 03:28Z; this head is 315e897a at 02:51Z, so the branch still sits on fd4bcb17 and has not seen it. Merging as-is:

$ git merge-tree --write-tree origin/main 315e897a
Auto-merging .agents/checks/review.md
Auto-merging SAFETY.md
CONFLICT (content): Merge conflict in SAFETY.md
Auto-merging docs/tcb-model.md
Auto-merging docs/testing.md

SAFETY.md conflicts, which is fine — it is loud and gets resolved by hand. docs/tcb-model.md auto-merges cleanly, and the clean result holds two rows for the same raw input:

94 | table name (copy-and-swap target) | preflight (planned)     | `CopySwapTarget` — … | ST-6, RF-1..RF-3 for the copy-and-swap route |
97 | table name (copy-and-swap target) | copy-and-swap preflight | `CopySwapTarget` (carries the integer PK and owner facts) | ST-6 for copy-and-swap |

Two validating passages and two different invariant sets for one proof type, in the table whose whole job is to be the single answer to "what proves this input was validated." TestDocsListEveryProofType (pkg/preflight/docs_test.go:24-31) asserts with assert.Contains, so a duplicate satisfies it twice over — the same blindness as the omission case, from the other side. Nothing in the diff shows it and nothing in CI catches it; it only appears if you merge the two trees. Rebasing and dropping the older row is all it needs.

Worth noting docs/invariants.md is untouched on main since the base, so the registry rewrite in this PR carries no comparable risk. docs/testing.md also auto-merges cleanly and correctly (the main side adds a Test harness subsection under TM-5; this side edits the frozen-SQL bullet 25 lines above).

Renaming the ST-1 heading orphans an inbound link (low)

invariants.md:252 becomes ### ST-1 — The checkpoint is one row per target, written atomically — a good fix, since the old heading contradicted its own body. But docs/design-principles.md:182 still points at the old slug:

([invariants ST-1](invariants.md#st-1--the-checkpoint-is-a-single-row-written-atomically))

That is the only broken link in the tree: I resolved every relative markdown link and fragment across all 42 .md files against GitHub's heading-slug rules, and this is the one that does not resolve. There is no link checker in CI, so nothing will report it.

Smaller things

• NAMEDATALEN - 1 reached one of three sites. copy-and-swap-design.md:168 is now precise about the 63-byte limit, but copy-and-swap-design.md:16 ("fits in PostgreSQL's NAMEDATALEN limit") and tcb-model.md:94 ("derived names within NAMEDATALEN") still name the macro rather than the bound. Off by one in the reader's head, not the code.
• architecture.md:197 is the one residual from the last round still standing. It gives pkg/table the "PK-range chunker over one integer-family primary key" row while this PR's package map hands Chunk and Watermark to pkg/copier; low-level-design.md:632's tree agrees with architecture.md. Either reading is defensible (a chunker in pkg/table can still hand pkg/copier the proof type it mints) — it just needs to say which, once.

The guards ran this time, but the hazard behind them did not move

Last round the whole test/lint/build set was skipped, because changes classes a tree of .md edits as "no code." This head touches one Go file — the one-line comment fix at pkg/statement/desired.go:187 — so code == 'true' and all five PostgreSQL legs ran, including the docs_test.go guards that read the three registries this PR rewrites. That is real coverage — though not yet of the addition it looks like it covers: proofTypes on this head is still {"PreflightedTable", "AbsentTarget", "CreationRole"} (docs_test.go:25), and CopySwapTarget only enters that list on main, from #87. So the CopySwapTarget addition to .agents/checks/review.md:19 is unpinned until the rebase — and pinned by a Contains afterwards, which is the duplicate-blindness above.

But it is coverage by accident: a comment edit unrelated to any of the prose flipped the filter. The next docs-only PR gets the same skip, and the guards that exist specifically to pin prose against code are the ones it switches off. Not this PR's job to fix — worth a follow-up, since the value of seven docs_test.go files is conditional on a filter that treats their subject as inert.

What the sweep got right

Every residual from the last round except architecture.md:197 is addressed, and addressed by aligning the prose with a recorded decision rather than by deleting the awkward sentence:

• The two low-level-design.md sites that gave REPLICA IDENTITY FULL as the unchanged-TOAST remedy — the pages a reader is most likely to reach first — now give D6's answer and say why FULL is not one (it enlarges only the old tuple; the marker stays), plus the line that the engine never changes the user's replica identity.
• D15 reaches every page that still offered trigger capture as an available fallback (README.md:84-88, design-principles.md:136-145, high-level-design.md:326-329, low-level-design.md:591-594, change-capture-tradeoff.md:83-84, vision.md:103-104, schemabot-integration.md:381-383, testing.md:56), and each one keeps the alternative documented rather than deleting it. Same for D12 (low-level-design.md:540-541, :635), D10 (schemabot-integration.md:59), D11 (design-principles.md:146-149).
• CO-8 now appears in all three package maps — SAFETY.md:26, tcb-model.md:43, and the design page's own map — with pkg/decode named, which was the gap: CO-8's Enforced: line leads with pkg/decode and no decode row carried it.
• AGENTS.md:88-89 recording Qualify as the one permitted AST edit is an honesty fix, not a loophole: the ban is on deriving semantics at the AST level, and retargeting one relation name derives none. The matching comment at pkg/statement/desired.go:187 now says "execute-and-introspect's job" instead of naming a scratch database D1 rejected.
• engine-role.md:54-57 noting that the scratch schema needs CREATE on the database — a requirement of every declarative plan, not only this route — and recording that neither the tier table nor preflight's privilege probe covers it yet, is the right way to handle a gap found mid-sweep: state it where a reader will hit it, mark it open, don't quietly widen the tier table.

Approving on the same basis as last round — this is a docs PR and every finding above is a doc claim to correct, none of them a reason to hold the set. The pgoutput one in 1/2 is worth doing before Phase 6 reads this page as its spec, and the rebase is worth doing before merge.

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

[aparajon]

🤖 Re-approving 315e897a. The D6/D13 collision from the last round is resolved properly, and the sweep closes every residual but one. Findings are in the two comments above — the pgoutput completion rule needs the key-move case (a PK-moving UPDATE emits exactly the marker-bearing image D13 calls a protocol error; OldKey, landed by #87, addresses it), and the branch wants a rebase before merge so docs/tcb-model.md does not end up with two CopySwapTarget rows.

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

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — block/pg-sprite pull/88, follow-up commit

Re-review of 315e897: three blocking findings.

#	Concern	Status
1	docs/change-capture-tradeoff.md:39 still says unchanged TOAST is "omitted unless REPLICA IDENTITY FULL", contradicting D6/CO-8 (FULL enlarges only the old tuple; the marker still arrives)	fixed — the capture-completeness row now states the unchanged-TOAST marker arrives under every replica identity, that FULL enlarges only the old tuple, and that the applier skips the column column-wise, linking D6 and CO-8. Swept the other two REPLICA IDENTITY FULL mentions (low-level-design.md:559, mysql-vs-postgresql.md:171): both concern row identity for delete/update matching, not TOAST, so they stand
2	copy-and-swap-design.md D13's fail-closed rule assumes CO-4 defers every in-flight-chunk flush, but invariants.md CO-4 is delete-scoped and low-level-design.md allows tombstone retention as an alternative; under tombstone retention a marker-bearing UPDATE for a key in an in-flight chunk flushes with the shadow row absent and D13 would abort on a permitted interleaving	fixed by stating the real requirement — D13 now names CO-4's two admissible disciplines, shows the tombstone form breaks the fallback's shadow-row read, and fixes the applier's choice: a flush touching any key inside an in-flight chunk is deferred until the chunk lands (mutual exclusion per overlapping key range); the tombstone form is not available to the v1 applier. Only under that discipline is an absent shadow row a protocol error. low-level-design.md's CO-4 bullet gains one sentence recording that v1 takes the mutual-exclusion form for every buffered change, not only deletes, linking D13 — so the two documents agree rather than offering the alternative D13 rules out. invariants.md CO-4 is unchanged: it already points at the low-level-design section for the full rule set
3	docs/design-principles.md:182 links the pre-rename ST-1 anchor #st-1--the-checkpoint-is-a-single-row-written-atomically	fixed — #st-1--the-checkpoint-is-one-row-per-target-written-atomically. Grepped docs/, AGENTS.md, SAFETY.md, README.md for st-1--: this was the only occurrence. All intra-doc anchors across those files were checked with a slug script; every one resolves

Also in this push: merge of origin/main (a8d171c, through PR #91). Conflicts in .agents/checks/review.md and SAFETY.md resolved by taking main's fully qualified proof-type lists (they already carry preflight.CopySwapTarget) and keeping this PR's CO-8 / per-column-presence additions on the applier and decode rows plus the pglogrepl recorded decision. docs/tcb-model.md auto-merged into two CopySwapTarget rows; collapsed into one that carries main's owner-role fact today and this PR's v1-scope facts once the passage lands. PR #91's derived proof-type registry test (internal/safety/proof_types_test.go) replaces the pkg/preflight/docs_test.go this PR had extended; it passes on the merged tree.

[aparajon]

🤖 Re-review 1/2 — 9e9e3394, the follow-up commit (docs: fix TOAST remedy claim, scope D13 fail-closed rule, repair ST-1 anchor)

Re-reviewing only what moved past my last approval at 315e897a: one docs commit over four files, plus the merge of main that brought #87 and #91 in.

The merge was hand-resolved, and it did not cost anything. git merge-tree --write-tree 315e897 a8d171c conflicts in .agents/checks/review.md, SAFETY.md, and docs/tcb-model.md — the docs/tcb-model.md overlap with #91 that was foreseeable from the two branches touching the same tables. So the recorded tree is not a mechanical replay and the deletion count against one is meaningless. Checking the direction that matters, git diff a8d171c e8cd19c over those three files is additive on every hunk (CO-8 added to the TCB and SAFETY component tables, the pglogrepl recorded decision, the expanded CopySwapTarget row, the tightened shadow-DDL review rule). It removes exactly one line — SAFETY.md's "The future decode path will add pglogrepl" — immediately superseded by the four-line recorded decision that replaces it and says strictly more. Nothing #91 or #87 added to those files is missing.

All three follow-ups land, and I checked each claim rather than the commit message.

The capture-completeness row is now right, and agrees with the invariant it has to agree with. The old row said unchanged TOAST is omitted "unless REPLICA IDENTITY FULL", which offered a remedy that does not exist. CO-8 (docs/invariants.md:139-143) is unambiguous the other way — "Under either PK-based replica identity (DEFAULT or FULL), pgoutput sends an unchanged TOASTed value in the new tuple as the unchanged-TOAST marker" — and the new row states the same thing, gives the reason in the right place (FULL enlarges only the old tuple), names the actual remedy (the applier's column-wise skip) and links D6. That is the version an operator can act on: the old one invited someone to set FULL and believe the gap closed.

D13's fail-closed rule is now conditional on a discipline the design actually commits to. The previous text asserted that a marker-bearing image with no shadow row is a protocol error because such a key "is either above the copier watermark … or inside an in-flight chunk (whose flush CO-4 already defers until the chunk lands)" — which read CO-4 as mandating deferral when it admits two forms. The new passage (copy-and-swap-design.md:251-264) names both, shows the interleaving that breaks the read under the other one ("a marker-bearing UPDATE for such a key would flush while the copier has not yet written the row, and the fallback would find no shadow row on a permitted interleaving"), then fixes the choice — deferral for every buffered change, tombstone retention unavailable to the v1 applier — and only then re-derives the fail-closed claim. That is the right repair: it makes the guarantee follow from a stated decision instead of from a misreading, and it widens the rule from deletes to any buffered change, which is what the shadow read needs. low-level-design.md:458-460 carries the same choice next to the invariant text, so the two do not have to be reconciled by a reader.

The ST-1 anchor resolves, and I checked the heading rather than the link text. invariants.md:252 is ### ST-1 — The checkpoint is one row per target, written atomically, whose slug is st-1--the-checkpoint-is-one-row-per-target-written-atomically — exactly what design-principles.md:182 now cites. The two anchors the same commit adds check out too: copy-and-swap-design.md:123 ### D6 — Preserve omitted TOAST values → d6--preserve-omitted-toast-values, and low-level-design.md:426 ## Copy and apply ordering (the core correctness subtlety) → copy-and-apply-ordering-the-core-correctness-subtlety. A link that resolves to the wrong section is the failure mode worth spending the two minutes on, because it looks identical to a link that resolves to the right one.

One finding, in 2/2. It is about the registry entry these three fixes now lean on, not about the fixes.

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

[aparajon]

🤖 Re-review 2/2 — the invariant the fix now leans on (low-med)

D13 now says the shadow read "holds only under one of the two chunk-overlap disciplines CO-4 admits", and fixes v1 on the deferral one. CO-4's own body admits only the other one, and only for deletes:

### CO-4 — The copy/apply ordering invariants
…a delete for a key inside
an in-flight chunk must be re-applied after that chunk lands. Full statement and the races these
resolve:
[low-level-design § copy and apply ordering](low-level-design.md#copy-and-apply-ordering-the-core-correctness-subtlety).
*Enforced:* copier/applier SQL shapes + flush scheduling.

— docs/invariants.md:78-82. "Re-applied after that chunk lands" is tombstone retention, which is precisely the form D13 now says the v1 applier does not have. The two-alternatives statement lives only in low-level-design.md:455-460, which CO-4 defers to for the "Full statement"; and low-level-design.md:458 is where the v1 choice was just recorded. So the entry is not contradicted by the design docs, but it is the least specific of the three places that now describe this rule, and it is the one a reviewer is told to check against.

Two consequences, both of the shape the registry is meant to prevent.

A future applier checked against CO-4 as written passes by implementing tombstone retention, which D13 has just ruled out — and it would pass the entry's *Enforced:* line too, since "copier/applier SQL shapes + flush scheduling" describes both forms equally well. The reader who goes to the registry first, which is what citing invariant IDs in review is for, gets the answer the design just rejected.

And CO-4 is scoped to deletes, while the rule that now governs is broader. D13's motivating case is a marker-bearing UPDATE: the fallback's SELECT … FOR UPDATE on the shadow row is what fails when a key inside an in-flight chunk flushes early, and low-level-design.md:458 says so — "v1 takes the mutual-exclusion form for every buffered change, not only deletes, because the unique-key fallback reads the shadow row to complete unchanged-TOAST markers." An UPDATE for a key inside an in-flight chunk is now the interesting case, and CO-4 says nothing about it.

The fix belongs in this PR rather than a later one, because this PR is what makes the entry stale: before 9e9e3394 there was no recorded choice for CO-4 to be behind. Either shape works — state both admissible forms in CO-4 and name the v1 choice, or narrow CO-4 to the v1 choice outright — but the scope needs widening from "a delete" to "any buffered change for a key inside an in-flight chunk" either way, and the *Enforced:* line should name the mutual exclusion specifically, so it distinguishes the two.

Worth pairing with the test obligation. CO-8's obligation (invariants.md:147-149) is stated concretely enough to be checkable — "a convergence test updates other columns while leaving a ≥8 KiB column untouched … run under both REPLICA IDENTITY DEFAULT and FULL". CO-4 has none, and the case D13 turns on is exactly the kind that needs one: a marker-bearing UPDATE for a key inside an in-flight chunk, asserting the flush waits rather than finding an absent shadow row. That is the test that would fail if someone later implemented the tombstone form.

Re-approving at 9e9e3394. The three follow-ups are right, the merge cost nothing, and this is a registry-alignment note rather than a defect in the change.

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

[aparajon]

🤖 Re-approving at 9e9e3394. All three follow-ups from the last round land, and I verified the claims rather than the commit message: the capture-completeness row now agrees with CO-8 under both replica identities and points at the remedy that exists, D13's fail-closed rule is now derived from a recorded v1 choice (deferral for every buffered change) instead of from a misreading of CO-4, and the three anchors resolve against their actual headings. The hand-resolved merge of main is additive on every hunk over the three conflicted files and loses nothing from #87 or #91.

One low-med finding in 2/2: CO-4's registry text still states only the tombstone-retention form, and only for deletes, so it is now the least specific of the three places describing this rule — and the one a reviewer is told to check against. Worth aligning in this PR, since this PR is what creates the gap.

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

[morgo]

🤖 Review posted by Morgan's AI agent.

Approving at 9e9e3394. I checked the "docs only; no code or behaviour change" claim first, since there's a .go file in the diff: pkg/statement/desired.go changes one line of a doc comment — "that is the scratch database's job" → "that is execute-and-introspect's job" — which is stale prose being brought in line with D1's decision not to have a durable scratch database. The claim holds.

The invariant amendments are the substantive part, and they tighten rather than relax. Two are worth calling out because they close hazards that would otherwise have been found the expensive way:

CO-5's new merge-never-replace rule. Dedup overlaying only the columns a newer image carries is not a detail — with pgoutput's partial images, a replace-on-dedup would let a later UPDATE's unchanged-TOAST marker erase a buffered image that actually held the value. Stating that the marker survives dedup only when no buffered image for that key ever held the column is the precise form of that rule, and it's honestly labelled as this doc set's addition rather than inherited from Spirit.

CO-6's TOAST completion before the fallback delete. This is the one I'd have gone looking for. The batch-wide delete-all-then-insert-all recovery reinserts whole rows, so an image still carrying an unchanged-TOAST marker would reinsert a fabricated value over live shadow data — silent corruption that the checksum would catch only later, if at all. Requiring the fallback to complete such an image from the current shadow row under SELECT … FOR UPDATE in the same transaction, and to treat a missing shadow row as an invariant violation rather than a reason to guess, is exactly right: fail closed on the case that shouldn't happen.

The D13 reasoning behind that fallback is also sound. A cyclic exchange {1→'B', 2→'A'} on a UNIQUE column collides in both orders, so per-key delete-then-insert retry cannot converge — no ordering heuristic rescues it, and the batch-wide form is the smallest thing that does. Making that a named test obligation with the concrete seats vector, plus a second vector adding a ≥8 KiB untouched TOASTed column, means the two hazards are pinned together.

CO-8's underlying claim about pgoutput is correct: the unchanged-TOAST marker is a property of how the new tuple is encoded, so it appears under REPLICA IDENTITY FULL just as under DEFAULT — FULL gives you a fuller old tuple, not a resolved TOAST value. Admitting FULL while stating plainly that it is not a way around the marker is the right pair of decisions.

One coupling worth being explicit about, since it's now load-bearing. CO-5's watermark rule changed from "discarded only for a monotonic integer PK, and must be queued for composite/non-comparable PKs" to discarded outright, sound because D4 restricts v1 to one integer-family PK. That's strictly safer than queueing — but the safety now lives in a preflight refusal (copy-and-swap-pk-unsupported) rather than in the buffer. If that refusal is ever loosened, or a route reaches the buffer without passing it, the discard becomes unsound with nothing local to catch it. Worth a pointer in CO-5 naming the preflight proof type as its precondition, so the dependency is visible from the invariant that depends on it.

ST-1's re-key from a fixed id=1 to (schema_name, table_name) is consistent — the ON CONFLICT matches, and the guarantee is correctly narrowed to "never a partial pair for one target" rather than continuing to claim there is always exactly one row, which per-target keying can't support before a target's first write. Good that it was narrowed rather than restated.

Docs-only, 12/12 checks green, mergeStateStatus CLEAN.

[Kiran01bm]

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

All findings across the three review rounds are fixed in the follow-up commit or in an earlier commit on this branch, except the docs-only CI guard, which is tracked as an internal follow-up (roadmap R22); the 13:17 round-3 first comment raised no findings, so no action.

#	Finding	Status	Explanation
C1-F1	Shadow inherits relreplident from CREATE TABLE, not from the source	fixed	D2 replicates the source's replica identity onto the shadow; ST-5 and the fidelity checklist compare relreplident
C1-F2	NOT VALID constraints become validated on the empty shadow	fixed	D2 re-adds CHECK … NOT VALID on the shadow so convalidated matches; ST-5 compares convalidated
C1-F3	D13 recovery rule misstated what fails closed	fixed (in 315e897a)	D13 now scopes the fail-closed rule to the cutover transaction and names the checkpoint row that recovery reads
C1-F4	Watermark rule discards above-watermark changes wholesale; loses a PK-moving UPDATE	fixed	D4 and CO-4 judge per key: old key below the watermark is deleted, new key above it is imaged (OldKey); test obligation UPDATE t SET id = 5000 WHERE id = 5 at watermark 1000 added
C1-F5	Index pairing by name breaks on renamed indexes; extended statistics unhandled	fixed	D8 pairs by definition, treats identical-definition indexes as interchangeable, and renames extended statistics with the table
C1-N1	pg_sequences / pg_sequence_last_value() hide is_called	fixed	D5 reads (last_value, is_called) from the sequence relation
C1-N2	New identity sequence keeps a shadow-derived name after swap	fixed	D5 renames the identity sequence to the source's sequence name at cutover step 4
C1-N3	"PK-based" phrasing contradicts the NOT NULL UNIQUE admission	fixed	Reworded to "usable primary key or NOT NULL UNIQUE key" everywhere; rg PK-based is clean
C2-F1	Auto-merge with the contracts PR yields two CopySwapTarget registry rows	fixed (in e8cd19c)	The merge of main reconciled docs/tcb-model.md to one row; TestDocsListEveryProofType passes
C2-F2	CI runs no docs_test.go guard on docs-only PRs	deferred	CI change, scoped out of this PR as the comment suggests; tracked as an internal follow-up (roadmap R22: docs-only leg running the guard packages)
C2-F3	Sweep left stale replica-identity / TOAST / queued-PK claims standing	fixed (in 315e897a)	low-level-design.md TOAST paragraphs and invariants.md watermark bullet now match D6 and the v1 composite-PK refusal
C2-F4	CO-8 decode rows contradicted the unchanged-TOAST decision	fixed (in 315e897a)	CO-8 restated around the marker; out-of-line test obligation added this round
C2-F5	pkg/table planned in three docs, owned by none	fixed	Folded into pkg/copier in the package map, architecture, and planned tree
C3-F1	Completion rule reads only Key; a PK-moving UPDATE completes from the wrong row	fixed	D13 and CO-6 complete from the Key or OldKey row; CO-6 second vector moves a PK
C3-F2	No test vector exercises an out-of-line TOAST value	fixed	CO-6 and CO-8 add the SET STORAGE EXTERNAL vector asserted through ToastBytes()
C4-F1	Duplicate CopySwapTarget row still present	fixed (in e8cd19c)	Same reconciliation as C2-F1
C4-F2	ST-1 anchor broken	fixed (in 9e9e3394)	Anchor repaired; the link script over the changed docs reports only the two pre-existing external misses
C4-F3	Identifier limit stated as 64 bytes at two sites	fixed	Both sites now say 63 bytes (NAMEDATALEN - 1)
C4-F4	architecture.md package row promises the retired scratch database	fixed	Row rewritten for the transaction-scoped scratch schema
C4-F5	Docs-only CI guard (restated)	deferred	Same follow-up as C2-F2
C5	Round 3, 1/2: no findings	—	No action
C6-F1	CO-4 registry text does not name the precondition the code will check	fixed	CO-4 states the CopySwapTarget proof / copy-and-swap-pk-unsupported refusal as its precondition and the mutual exclusion with the tombstone
R1	CO-4 / CO-5 should point at the preflight proof type as precondition	fixed	Same CO-4 rewrite; CO-5 carries the two-entry OldKey rule
S1 (pg-sprite-pr88-review-9e9e3394.md, head 9e9e3394)	Reaper keyed on table hash alone can reap a same-named table's slot in another database	fixed	D11 hashes database.schema.table, commits the checkpoint row before slot creation, scopes the reaper to pg_replication_slots.database = current_database() and active_pid IS NULL, and adds the copy-and-swap-slot-collision refusal

Source: #88, review comments 5594080811, 5594081296, 5596228680, 5596229228, 5602493022, 5602494086 and review 5155516816 at head 9e9e3394