docs: the optimistic attempt and the table-size guard - #59
Conversation
The two front doors were introduced in high-level-design.md but the bounded attempt's full design — the entry-to-exit map, which routes the size guard covers and why, the wrong-guess cost model, and how the ladder evolves once copy-and-swap lands — had no home. Also corrects high-level-design.md's stale claim that the guard measures pg_class.relpages (it measures pg_total_relation_size).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head Verdict: the page is excellent and one of its central claims is wrong. The entry-to-exit map is the artifact this repo has been missing, the wrong-guess timelines are the clearest explanation of the design I've read, and the "why gamble at all" answer is genuinely persuasive. But the TL;DR's rung 1 and the Q&A both say proof-based routes are never size-checked, and three of the five planner reasons that reach execution — including Not blocking in the mechanical sense — docs-only, no code, nothing deployed — and the same shape as #57, which I stamped with the correction as the top action item. Stamping on the same terms: Finding 1 should land before merge.
1. Three of the five executing proof reasons are size-guarded, including the one the sentence namesRung 1 of the ladder:
and the Q&A, which is the page's canonical answer to the question it was written to settle:
Both are false for
{
"outcome": "refused",
"reason": "not-native-safe-table-too-large",
"statement": "ALTER TABLE z59m.t ADD COLUMN c1 int",
"detail": "table is 83771392 bytes on disk (heap, indexes, and TOAST), above the configured 10485760-byte size threshold. …"
}The axis is not proof-vs-bet.
And the reason that rule is right is the most interesting thing this page could say, so it's worth saying rather than trading away: Three places on the page already have this right, which is why I read it as a sentence-level fix rather than a rethink:
Outside the page,
as does the CLI help ("Planner-proven online steps (concurrent index builds, constraint validation) are not size-guarded") and Why this one is worth landing before merge rather than as a follow-up: this page is now the authoritative deep treatment, and the claim is load-bearing in the direction that costs an operator something. A reader who believes rung 1 concludes that adding a nullable column always converges, and at the 1 GiB default that is refused on any table big enough to care about. One knock-on: 2. The exit inventory publishes the failure-path codes for the refusal exitsExits 3a and 3b are typed as Those are {
"outcome": "refused",
"reason": "not-native-safe-budget-exceeded",
"cause": "statement-budget",
"statement": "ALTER TABLE z59m.t ALTER COLUMN v TYPE varchar(300)",
"detail": "cancelled after the 150ms statement budget: the change does real rewrite work, not an in-place catalog change, …",
"forced": true
}
This matters more than a token typo because of the page's own invariant 4 — "automation never branches on error prose" — and invariant 5 — "a budget refusal is not a failure." The inventory contradicts invariant 5 in the table two hundred lines above it: it labels a refusal with the code that only appears on the failure it says this isn't. An integrator wiring retry policy from this page's inventory writes a matcher that never fires. Narrow fix, and it doesn't touch the prose at line 262 ("stable codes —
Same treatment would help row 4, which is typed 3. The inventory has no row for the one ending that leaves DDL on the tableSix exits, and the "DDL executed?" column answers Yes / Yes / No / No / No / Yes. There is no row for $ pg-sprite migrate --alter 'ALTER TABLE z59m.nn ALTER COLUMN v SET NOT NULL' --json
{
"outcome": "failed",
"code": "execution-failed",
"failed_step": 2,
"failed_step_sql": "ALTER TABLE \"z59m\".\"nn\" VALIDATE CONSTRAINT \"nn_v_not_null\"",
"detail": "sequence step 2 of 4 failed; the 1 committed steps' state remains — …",
"executed_sql": [
"ALTER TABLE \"z59m\".\"nn\" ADD CONSTRAINT \"nn_v_not_null\" CHECK (\"v\" IS NOT NULL) NOT VALID"
]
}
exit=1and the residue is real — The page's opening paragraph hands this off — "what a mid-sequence failure leaves behind is [execution-model.md]" — which is a fine division of labour for the mechanics. But this page owns the claim that it maps every exit ("the full entry-to-exit map every statement walks", "Every ending is typed", "the exit inventory catalogs the endings"), and an inventory whose safety-relevant column never says "yes, partially" is the one place a reader would confidently conclude that no ending can leave debris — which is the opposite of the guarantee the rest of the page is careful about. One row, pointing at | 7 | Sequence stopped mid-flight (step failed, external cancellation) | 4. The declarative door has a whole-plan admission gate that the map doesn't have
Live, against a desired file that drops one column and adds another: {
"outcome": "refused",
"reason": "destructive-change",
"detail": "planned statement 1 discards live structure (ALTER TABLE z59.d2 DROP v); desired-state execution runs no destructive statement — run it deliberately through the imperative front door; admission is all-or-nothing, so the plan's other statement, even if non-destructive, was not run"
}No This is worth a node in the map rather than a footnote, because it's the difference between "each statement is judged" and "the plan is judged, then each statement is judged", and it's the gate an embedder hits first. The per-statement point matters too: the size guard is not a plan-level check, so a caller that pre-flights the plan can still take a size refusal on statement 3 of 5 with statements 1 and 2 already committed — which is exactly the Also(nit) The page's only use of the word "migration" is the anchor (nit, not this PR) (nit) The peer table gives Action items
Verified — tried to break, couldn'tEvery link and anchor resolves — 26 of them, including both anchors into The The size-guard knob section is accurate. Lane F really is still size-guarded (invariant 2's second half, and the §"Who gets size-checked" row): Multi-statement smuggling really is refused at parse — Both wrong-guess timelines reproduce. Case 2 (small table, rewrite fits):
The MySQL mapping paragraph checks out. Invariant 3 is correctly scoped. CI is green — DCO, This review was generated by Claude Code (claude-opus-5). |
|
🤖 Second pass, requested by @aparajon and performed by their agent — two lenses this time: OSS adoption ease (can someone outside this team decide to trust pg-sprite from this page?) and orchestrator integration (can an embedder wire routing from it?). Nothing here blocks; the correctness pass is the other comment. Both lenses land on the same missing artifact: a
Lens 1 — adoptionWhat this page does that nothing else in the repo does: it answers the actual objection. Every evaluator arrives at "you attempt DDL on my production table and hope" and stops there. This page takes the objection seriously enough to name it a gamble in its own voice, then earns the position back with the PostgreSQL-specific argument. The peer table is fair to the competition rather than dismissive, and the amplifiers paragraph — the fair lock queue draining serially, "you can't roll back an outage" — concedes the worst case in a way that makes the rest credible. That honesty is the asset. Keep it under editing pressure. 1. Publish the mapping, don't just describe it. The page teaches a four-lane model and then leaves the reader unable to answer the only question they have: which lane is my
That's the entire page in one glance, it's the fix for the correctness finding, and it's the thing a reader will screenshot. 2. Give them a way to check. The model is only useful if a reader can test membership, and today they can't — there's no recipe on the page. There is one, it's one command, and it's how I built the finding matrix in about four minutes: $ pg-sprite migrate --dry-run --json --alter 'ALTER TABLE t ADD COLUMN c int' \
| jq -r '.statements[].decisions[].reason'
metadata-onlyThree lines next to the table above and the page stops asking to be trusted and starts asking to be verified — which is the posture the whole argument is making anyway. It also front-loads the honest caveat: the classifier told you 3. Move the default's consequence up. The most surprising behaviour in the tool is that at defaults, adding a nullable column to a table over 1 GiB is refused. 4. Lens 2 — orchestrator integration5. The exit inventory is a contract, so its layering matters. The correctness finding (exits 3a/3b typed with the executor's failure codes) reads as a token typo but behaves as a seam bug: an orchestrator that writes its retry matcher from this table matches 6. The routing contract is the table this page is one step from owning, and nothing else owns it. The page already has the raw material — invariant 5 says a budget refusal is not a failure; the Phase 5 section says which refusals become routing signals — but it never tabulates the operational consequence. For an embedder that consequence is the whole integration:
The distinction that matters most: "retry later" versus "stop and tell a person" is, for an orchestrator, the difference between a transient blip and a blocked merge gate. That judgement lives in this engine's head today and in no document. 7. Pin the table by test — third time I've asked for this shape, and this one is the cheapest. for _, r := range planner.Reasons() {
assert.Contains(t, doc, fmt.Sprintf("| `%s` |", string(r)),
"docs/optimistic-attempt.md is missing a size-guard row for %q", r)
}Stronger still: build a synthetic single-decision plan per reason and assert the doc's row agrees with 8. The guard is per-statement, and the declarative door is library-only. Two clauses for embedders. Action items
This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving — the correctness and adoption/integration passes are in the two comments above. Finding 1 (the TL;DR and Q&A say proof-based routes are never size-checked; metadata-only, fast-default and binary-coercible are all guarded) is worth landing before merge, but it's a sentence-level fix on top of a model the rest of the page — and limitations.md in this same PR — already state correctly.
This review was generated by Claude Code (claude-opus-5).
Adds
docs/optimistic-attempt.md— the deep treatment of the bounded optimistic attempt and the table-size guard — and cross-links it from the docs index, high-level-design, and limitations.Why
The two front doors are introduced in
high-level-design.md, but the questions the bounded attempt keeps raising — why gamble at all, why proven routes skip the size guard, what a wrong guess costs on a small vs a large table, whether the attempt is a stopgap until copy-and-swap — had no single authoritative answer in the doc set. This page settles them once, with the full entry-to-exit map every statement walks.What
docs/optimistic-attempt.md: the escalation ladder (proof → bounded attempt → refuse/copy), the four-lane entry-to-exit flow diagram with a gate-by-gate walkthrough and typed exit inventory, theOptions.MaxTableSizeBytesknob, proof-based vs budget-based routes, Q&A, wrong-guess timelines, peer-tool comparison, and the invariants the page rests on.docs/README.mdplus pointers from the "decided shape" section, the front-doors section ofhigh-level-design.md, and the size-threshold paragraph oflimitations.md.high-level-design.md: the size guard measurespg_total_relation_size(heap + indexes + TOAST, all partitions), notpg_class.relpages—pkg/preflightexplicitly treats planner estimates as never the guard's authority.Before / after