docs: safer-sequences page (the improve path) + post-#50 clarifications

[Kiran01bm]

Summary

Adds docs/safer-sequences.md — a human-first explanation of the improve path: why the planner's substituted sequence is safer than the submitted form, worked through on ADD CONSTRAINT … UNIQUE — and carries two small follow-up clarifications that missed the #50 merge.

Why

The improve path is the product's headline move, but nothing explains why the substitution is safer — the README demos show it happening, the reference doc has the per-operation matrix, and the contracts carry the machine shape, yet a reader who asks "these two forms end in the same catalog state, so what did I gain?" has no page. The answer (blocking risk traded for leftover-state risk, exclusive locks confined to catalog flips) is the argument that wins an evaluator.

What

• docs/safer-sequences.md: the worked ADD CONSTRAINT … UNIQUE comparison — one-statement vs two-step across locking, duplicate-failure behavior, transactionality, and cost; what the engine adds over running the idiom by hand (budgets per step, typed verdicts, visible substitution); the substitution families shipped today; the typed caveats and the USING INDEX structural limits.
• README (Improve paragraph) and docs/README.md index: link the page.
• Follow-ups that missed the docs: add execution-model page (autocommit-each-step, committed prefix) #50 merge: the committed-prefix section gains a per-step table for the four-step SET NOT NULL sequence (purpose, lock profile, budget class), and both design docs disambiguate needs-rewrite — a PostgreSQL table rewrite (copy-and-swap's job), not a rewording of the submitted SQL.

Before / after

before                                      after
┌───────────────────────────────┐           ┌───────────────────────────────┐
│ "why is the substituted       │           │ README Improve ──▶            │
│  sequence safer?"             │           │   docs/safer-sequences.md     │
│                               │           │   one page:                   │
│ improve.gif shows it happens  │           │   - worked UNIQUE example     │
│ reference doc: per-op matrix  │           │   - same end state, different │
│ suggest-report: JSON caveats  │           │     path (comparison table)   │
│                               │           │   - engine vs by-hand         │
│ no page answers "why safer"   │           │   - substitutions today       │
│                               │           │   - typed caveats + limits    │
└───────────────────────────────┘           └───────────────────────────────┘

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by his agent. Reviewed at head cf2ad310, in a worktree, against a live PostgreSQL 16.14: rather than reading the page, I ran every statement it publishes through the real engine and measured the locks and durations it claims, on tables large enough for the claims to be falsifiable (up to 2936 MB).

Verdict: the page is the right page to write and almost all of it holds up under measurement — the SET NOT NULL per-step table is exactly true, and the worked UNIQUE example is byte-exact against real executed_sql. But the substitution table folds ADD PRIMARY KEY into the UNIQUE example, and for a primary key on a nullable column step 2 is not metadata-only: PostgreSQL must add NOT NULL, which scans the heap under ACCESS EXCLUSIVE. I reproduced that as an operator-visible failure — the step the page calls a brief catalog flip is cancelled by the brief statement budget and leaves a committed prefix. The planner's own comment makes the same claim, so this is a docs finding with an engine follow-up behind it. Finding 2 publishes a substitution the executor deliberately refuses. Neither is a live safety gate — both are accuracy on the page that teaches operators what to expect — but finding 1 describes a real outcome someone will hit.

Findings

1. "Confining every exclusive lock to a brief, metadata-only catalog flip" is false for ADD PRIMARY KEY on a nullable column, and the substitution table presents that case as covered by the UNIQUE example. Line 95 reads ADD PRIMARY KEY / ADD UNIQUE (direct) | 2 steps: … (this page's example), and the comparison table's "Lock to attach the constraint | ACCESS EXCLUSIVE, brief and metadata-only" is written for that shared row. For UNIQUEit is correct. ForPRIMARY KEYon a column that is not alreadyNOT NULL, ADD CONSTRAINT … PRIMARY KEY USING INDEXmust setattnotnull, and PostgreSQL validates that by scanning the heap — under ACCESS EXCLUSIVE, which is precisely the lock the substitution exists to keep brief. The isolating control, all three on 3M rows with the same --statement-timeout 20ms`:

nullable    + PRIMARY KEY -> failed (budget-statement-exceeded)   <-- step 2 of 2 (brief) cancelled
already NN  + PRIMARY KEY -> executed natively, all 2 steps committed
nullable    + UNIQUE      -> executed natively, all 2 steps committed   (the page's worked example)

The failure is not "big table," it is nullability — the two PK runs differ in nothing else. What the operator gets:

failed (budget-statement-exceeded)
  failed at: step 2: ALTER TABLE "public"."nn" ADD CONSTRAINT "nn_pkey" PRIMARY KEY USING INDEX "nn_pkey"
  committed before the failure (their state remains):
    1. CREATE UNIQUE INDEX CONCURRENTLY "nn_pkey" ON "public"."nn" ("id")

Bare timings for the same statement confirm the mechanism scales with the table, and attnotnull flips f→t across it: 0.610 ms already-NOT NULL at 3M rows, 68.274 ms nullable at 3M rows (112×), 576.157 ms nullable on a 2936 MB / 12M-row table. That last number is a real ACCESS EXCLUSIVE stall on every reader and writer, on the step the page describes as metadata-only.

The engine shares the belief. usingIndexSequence is reached for ConstraintPrimaryKey and ConstraintUnique through the same branch with no nullability check, and its comment states "Step 2 consumes the index into the constraint under a brief lock and does not scan." Step 2 is then admitted as StepBrief, whose doc says it is "a catalog change that must prove itself effectively instant" — so the brief budget is exactly the wrong budget for this one case, which is why the run above fails rather than merely being slow.

2. The substitution table publishes DETACH PARTITION … CONCURRENTLY as a substitution the planner makes today; the executor deliberately refuses it. Row 5 of line 98 lists DETACH PARTITION (non-concurrent) → 1 statement: DETACH PARTITION … CONCURRENTLY``, and the planner does substitute it — but executor.ErrUnsupportedSequenceSteprefuses the step on purpose, with a good reason: "a cancelledDETACH PARTITION CONCURRENTLY` leaves a detach-pending partition state this executor does not own detecting or recovering." End to end on a real partitioned table:

$ pg-sprite migrate --dry-run --alter 'ALTER TABLE dp.events DETACH PARTITION events_2026'
  pg-sprite will run a safer online sequence instead:
    1. ALTER TABLE dp.events DETACH PARTITION events_2026 CONCURRENTLY;
  1 statement, 1 step to run, 0 refused
  apply: re-run without --dry-run

$ pg-sprite migrate --alter 'ALTER TABLE dp.events DETACH PARTITION events_2026'
  refused (unsupported-statement)
    detail: DETACH PARTITION: step is not a shape the sequence executor can run safely

The planner/executor/dry-run disagreement is pre-existing and not this PR's regression — worth its own issue, since 0 refused plus "re-run without --dry-run" is an actively misleading pair. This PR's part is smaller: the page should not list it under "the substitutions the planner makes today" without saying execution refuses it. Note also that postgres-online-ddl-reference.md, which the page names as the full matrix, doesn't cover this case either.

3. (precision) unsupported-partitioned-parent is not emitted by the planner. The closing paragraph says "CREATE INDEX CONCURRENTLY is not supported on partitioned tables — the planner refuses with a typed reason (unsupported-partitioned-parent)". That reason comes from pkg/plan.RefuseUnsupportedPartitionedParent, pkg/migrate.partitionedParentVerdict (via preflight.UnsupportedPartitionedParentError), and pkg/executor.ErrUnsupportedPartitionedParent — never the planner. The linked reference section doesn't attribute it to the planner either. Layer attribution is load-bearing on this page precisely because the rest of it is careful about which layer does what; "pg-sprite refuses" would be both true and sufficient.

4. (nit, engine not docs) --dry-run and execution render the same sequence with different qualification. An unqualified --alter yields ON "users" in dry-run and ON "public"."users" in executed_sql, because dry-run renders from the submitted statement while execution renders post-resolveTarget. The page's block is the executed form and matches it byte-for-byte (a qualified --alter dry-run matches exactly too), so the page is right — but anyone diffing dry-run output against the page will see a difference the page can't explain.

Action items

1. (Finding 1) Split ADD PRIMARY KEY out of the UNIQUE row, or caveat it: on a nullable column, step 2 adds NOT NULL and scans under ACCESS EXCLUSIVE. The honest guidance is to reach NOT NULL first via the 4-step sequence this same page documents, then add the PK — which composes two substitutions the engine already has.
2. (Finding 1, engine follow-up) Have usingIndexSequence consult attnotnull for ConstraintPrimaryKey and either sequence SET NOT NULL ahead of adoption or admit step 2 under a scan-class budget rather than StepBrief; fix the "does not scan" comment either way. Happy to file this separately if you'd rather keep this PR docs-only.
3. (Finding 2) Mark the DETACH PARTITION row as refused at execution (or drop it), and file the planner/executor/dry-run disagreement — 0 refused on a step that cannot run is the part worth fixing in the engine.
4. (Finding 3) Attribute the partitioned refusal to pg-sprite rather than the planner.
5. (optional) (Finding 4) Normalize dry-run rendering to the resolved target so published sequences match dry-run output.

Verified (tried to break, couldn't)

The parts I most expected to be approximations are exact. The worked example's two-statement block is byte-identical to the executed_sql a real run produces, including quoting and schema qualification — I diffed them rather than eyeballing. usingIndexSequence really does emit exactly those two steps with the constraint's name reused as the index name, so the "no rename happens at adoption time" claim is structural, not aspirational; the _key/_pkey suffix and identifier-limit fitting behave as described. ADD CHECK / ADD FOREIGN KEY → NOT VALID + VALIDATE CONSTRAINT is correct, and StepValidateConstraint's own doc confirms the page's "a lock that lets reads and writes proceed" and the separate validate budget. The SET NOT NULL per-step table added to execution-model.md is the strongest part of the PR: every claim measured true on a 2936 MB table — NOT VALID CHECK 3.564 ms, VALIDATE CONSTRAINT 4771.646 ms under SHARE UPDATE EXCLUSIVE, SET NOT NULL 1.370 ms (the PG 12+ validated-CHECK scan skip, genuinely a catalog flip), drop-scaffold brief. That is the same table finding 1 stalls for 576 ms on, which is what makes the contrast decisive rather than theoretical. All five caveat names (non-transactional, separate-transactions, invalid-index-on-failure, validation-scan, detach-finalize-on-failure) exist in suggest-report.md; plan-report.md carries safer_sql as claimed; lint really does emit warning[blocking-idiom] with the safer form for the blocking statement, so the lede's division of labor across migrate/--dry-run/suggest/lint is accurate. All seven link targets and all four table-of-contents anchors resolve. The needs-rewrite clarifications in high-level-design.md and low-level-design.md are correct and worth having — "rewrite" meaning a PostgreSQL table rewrite rather than rewording the SQL is a genuine reader trap. The invalid-unique-index row is subtle and right: an invalid unique index does still enforce uniqueness against new writes. Docs-only diff, no test surface touched, no leaks. Process note: all four action items from #50 landed, including the lede fix about a failed step leaving state of its own, and #49's Example_run shipped with the merge — nothing carries over.

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

[aparajon]

🤖 Second pass, same head (cf2ad310), through the two lenses @aparajon asks pg-sprite changes to be judged on: how easily an outside team adopts this, and the seam an orchestrator embedding the engine consumes. Correctness findings are in the comment above; nothing here blocks.

Lens 1 — OSS adoption

This is the page that decides whether a skeptical DBA believes the project, and it is written at that level. The improve path is pg-sprite's actual differentiator — plenty of tools will diff a schema and emit DDL; the claim that the tool will substitute a safer sequence for the statement you wrote is the one that sounds too good to be true, and the one an evaluator will assume is marketing until shown otherwise. Publishing the mechanics, the exact SQL, and the locks is the right response to that skepticism. The page earns credibility in two specific places: the "Same end state, different path" table, which puts the submitted and substituted forms side by side per-property instead of asserting "safer"; and the paragraph that names the trade out loud — the safer sequence converts blocking risk into leftover-state risk, blocking paid by every query, leftover state paid by one operator with a documented recovery path. A page that only listed benefits would read worse. That paragraph is the single most persuasive thing in the PR.

The highest-leverage addition: a short "verify this yourself" section. The page makes falsifiable claims about locks and durations, and its authority rests entirely on the reader taking them on faith. A DBA evaluating a tool that promises to run DDL on their production tables will want to reproduce them, and right now nothing tells them how. Three lines would close it:

-- in one session, during step 1 and again during step 2:
SELECT mode, granted FROM pg_locks WHERE relation = 'users'::regclass;
-- and \timing on, or:
SELECT query, state, wait_event_type FROM pg_stat_activity WHERE query LIKE '%users%';

This is not a nice-to-have — it is how I found the finding in the comment above. A reader who runs the pg_locks check on ADD PRIMARY KEY will discover the nullable-column case themselves, and it is far better for the project if the page invites that than if a stranger finds it in production. "Here is how to check we are telling the truth" is the strongest possible framing for exactly this page.

The gap an adopter feels next is the other half of the story: when the substitution does not happen. After "what does it do for me" the immediate question is "when won't it save me" — and the honest answer (needs-rewrite operations, refusals, the USING INDEX structural limits) is scattered across the reference matrix and the design docs. The caveats section gestures at it in the last paragraph, which is the right instinct but the wrong weight: the structural limits of the USING INDEX form are buried in a sentence at the very end of the page, after the reader has been told five times that the sequence is safe. A short "what this does not cover" section, sibling to the substitution table, would land better than a trailing clause — and it is where finding 1's nullable-PK caveat naturally belongs.

Small structural note: the table of contents on a 118-line page is more scaffolding than a reader needs, and the two tables plus the five-item caveat list already give it shape. Not worth changing on its own; worth not adding to.

Lens 2 — the seam an orchestrator consumes

"Automation branches on fields, never on prose" is exactly the right principle, and the substitution table is the one place this PR violates it. The page states the principle in the "What the engine adds" section — and then publishes, as prose, the authoritative list of substitutions the engine performs. That list is precisely what an integrator needs to reason about ("which of my statements will pg-sprite transform?"), and prose is the wrong medium for it: finding 2 is a row that is already wrong, and there is no mechanism that would have caught it. The suggest report already carries the substitution per-statement, so the durable fix is for the page to teach the reader to ask the tool — one suggest invocation showing a substitution and its caveats, and the table demoted to an at-a-glance summary explicitly labeled as such. Then a new substitution can't ship with a stale doc row as its only description.

The per-step table added to execution-model.md is the real seam contract in this PR, and it deserves a test. Statement → what it does → lock and duration → budget class is exactly the mapping an orchestrator needs in order to set timeouts and decide what a failure means, and it is the first place the StepKind → budget-class relationship is written down for an outside reader. Which means it will silently go stale the first time a StepKind is added: nothing connects the enum to the doc. A table-driven test that enumerates the StepKind values and asserts each one appears in the doc would pin it cheaply — the same discipline the project already applies to keeping the verdict codes and the reference matrix honest. This is worth more than it looks, because the budget class is the field an integrator's retry logic branches on: StepBrief failing means "something is holding a lock, retry is reasonable," while StepValidateConstraint failing means "the scan didn't fit, raise the budget." Finding 1 is the failure mode when that mapping is wrong for a case, and a doc without a pin is how the mapping drifts.

For an orchestrator specifically, the committed-prefix framing is the part that matters most, and it improved here. An automated system that submits a statement and gets back "step 2 of 4 failed, steps 1 through 1 committed, their state remains" can decide whether to retry, roll forward, or escalate to a human — and the invalid-index-recovery.md link means the escalation has somewhere to point. The lede clarification carried over from #50 (a failed step leaving state of its own) is what makes that contract legible to someone reading the docs before writing the integration rather than after their first partial failure. Worth keeping the two pages cross-linked as tightly as they are now; the safer-sequence page is where a reader learns why there is a committed prefix at all, which is the thing that makes the execution model's contract feel motivated rather than defensive.

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

[aparajon]

🤖 Approving. Docs-only, and the substantive parts hold up under measurement — the SET NOT NULL per-step table is exactly true on a 2.9 GB table (1.370 ms for the catalog flip) and the worked UNIQUE example is byte-identical to real executed_sql.

Two accuracy items to land, per the comments above: ADD PRIMARY KEY should not be folded into the UNIQUE example (on a nullable column step 2 adds NOT NULL and scans under ACCESS EXCLUSIVE — I reproduced it failing the brief statement budget), and the DETACH PARTITION row is a substitution the executor deliberately refuses. Both are page fixes; the engine follow-ups behind them (planner nullability check, and the dry-run "0 refused" disagreement) are pre-existing and fine to file separately.

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