feat(migrate): create the table when the desired plan is greenfield - #63
feat(migrate): create the table when the desired plan is greenfield#63Kiran01bm wants to merge 2 commits into
Conversation
Desired-state execution previously refused a plan whose table does not exist. The greenfield path now verifies absence and schema CREATE privilege, then runs the create and index builds as brief bounded steps; an occupied name is the new typed create-collision refusal. Greenfield plans order CREATE TABLE first so plan order states execution order. Amp-Thread-ID: https://ampcode.com/threads/T-01a03b04-5f75-7059-b544-bb826e67db29 Co-authored-by: Amp <amp@ampcode.com>
17f9efe to
ab8ca3a
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
An index-before-table desired file created the table on run 1 and then hard-errored on every rerun: the scratch-schema replay executed input order while the plan and the create path hoisted the CREATE TABLE. Ordering once in ParseDesired makes every replay site execute table-first by construction; the two per-site hoists are retired. Also sweeps the capability docs the create path made stale.
aparajon
left a comment
There was a problem hiding this comment.
🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head ac8129b, in a worktree, against a live PostgreSQL.
Verdict: the front door is correctly gated and the partial-failure disclosure is honest — the piece I attacked hardest, the verdict-to-step position mapping, holds because SequenceReport.Steps only grows after a commit. Two findings, neither blocking.
The load-bearing decision here is moving the ordering into ParseDesired so the proof itself carries execution order, and then having all three replay sites assert it rather than re-derive it. That converts a rule three packages had to remember into one they can only violate by forging the proof — and each of them fails closed with ErrInvariantViolation when it is violated, which is the right severity: an out-of-order proof is a bug, not a user refusal.
Findings
1. (medium) The create path pins search_path; the alter path still does not — and this PR is what makes them siblings
executeBoundedAttempt takes a searchPathSchema and issues SET LOCAL search_path = <schema>, public when it is non-empty. The create path passes at.Schema() (pkg/executor/create.go:154). The alter path passes "" (pkg/executor/optimistic.go:222).
That closes the create half of the finding I raised on #62 — where I proved on a live database that an unpinned attempt cannot see a type declared in the target schema (type "my_enum" does not exist, 42704) and will silently bind a same-named type in public instead. The alter half is unchanged.
Before this PR that asymmetry was invisible, because the create path was not reachable from the declarative front door. RunDesired now routes on TableExists, so the same desired file resolves its unqualified secondary names — column types, CHECK functions, DEFAULT nextval(...), index opclasses — one way on the run that creates the table and a different way on the run that adds a column to it. docs/limitations.md explicitly supports unmanaged enum and domain columns, so this is a shape the tool invites.
It fails toward silently binding the wrong object rather than erroring, which is why it's worth closing rather than documenting. The fix is the one already written: thread the resolved schema into the alter path's call too.
2. (low) The documented create-path call order disagrees with the implementation
docs/schemabot-integration.md §"Routing the create path's refusals" specifies the order as ParseDesired → CheckCreatePrivileges (2) → CheckTableAbsent (3) → ExecuteCreate. runCreate does absence first and privileges second.
That matters for exactly one case, and it is a case an operator hits on a fresh database: when the name is occupied and the role lacks CREATE, the doc promises a PrivilegeError carrying the exact missing GRANT, and the code returns create-collision instead. Whichever order is intended, the two should agree — and I'd argue the code's order is the better one, since absence is the cheaper check and the collision is the more actionable message.
Two smaller things in the same section: it still presents the four-step sequence as something the adapter assembles, which RunDesired now does itself; and ErrIfNotExistsUnsupported — one of the four sentinels isCreateAdmissionRefusal routes to a refusal verdict — is the only one of the four with no row in that section's routing table, so an adapter learns the code exists (it is in the canonical list in execution-model.md, correctly) but not what to do with it.
Action items
- (Finding 1) Pass the resolved schema into
executeBoundedAttemptfrom the alter path too, so a desired file's unqualified type and function names resolve identically whether the run creates the table or alters it. - (Finding 1) Add a test that pins it: a type declared only in the target schema, referenced by an
ALTER TABLE ... ADD COLUMN, must resolve — and must not silently bind a same-named type inpublic. - (Finding 2) Reconcile the documented call order with
runCreate's, and say which refusal wins when both preflights would fail. - (Finding 2) Add the
if-not-exists-unsupportedrow to the outcome table, and note thatRunDesiredmints both proofs itself so an adapter does not double-mint them.
Verified (tried to break, couldn't)
I went after the verdict-to-step mapping first, because a create that fails mid-sequence is where a disclosure bug would do real damage. ExecuteCreate appends to rep.Steps only after a step's bounded attempt returns nil, so the for i := range rep.Steps loop can never emit an OutcomeExecuted verdict for the step that failed — the failed step's verdict is appended once, afterwards, from stepErr.Step-1, and committedPrefixDetail(stepErr.Step-1, …) counts the same prefix. The two agree by construction, not by coincidence.
planStatementSQL is bounded on both ends and returns empty rather than panicking, which is the right shape for a defensive read in a result renderer — and it is genuinely defensive rather than load-bearing, since the step count equals the plan's statement count whenever the proof is well-formed.
executionOrder is a stable two-pass partition, and ParseDesired's single-CREATE TABLE admission rule makes it total: the first pass emits exactly one statement, the second preserves input order among the indexes. Both new ST-7 guards (qualifiedDesired and admitCreateSteps) check len == 0 || [0].Kind() != KindCreateTable, so an empty set and a misordered set both refuse, and admitCreateSteps returns ErrInvariantViolation rather than one of the admission sentinels — so isCreateAdmissionRefusal correctly declines to convert it into a user-facing refusal. That list also correctly excludes ErrCreateCollision, which always arrives wrapped in a SequenceStepError and is handled on the other branch.
I checked whether report.TableExists == nil could route a greenfield plan into the alter loop: diffplan.Plan sets it unconditionally, and even if it were nil the alter loop's gate refuses CREATE TABLE outright — so the nil case fails closed twice over.
ExecuteCreate still re-verifies both proofs at the point of use (cr.Schema() == at.Schema(), ds.Table() == at.Table(), non-empty on each), so runCreate minting them from a pool cannot smuggle a mismatched pair past it. The IsNameOccupied branch returns a refusal with nothing executed and the PrivilegeError branch does the same, so neither preflight failure can be mistaken for a partial apply. stopBefore reports committedPrefixDetail(0, …) for every genuinely operational failure, which is the honest zero.
The new diffplan test pins the actual regression — an index listed before its table in the desired file now plans CREATE TABLE first — with exact SQL and exact kinds, not just a count. No test functions were deleted and no assertions weakened anywhere in the diff.
go build ./... and go test ./pkg/... pass at head; one preflight test needs isolation under local container contention (63s in the full run, 1.43s and passing on its own), and CI is green on PostgreSQL 14 through 18.
This review was generated by Claude Code (claude-opus-5).
aparajon
left a comment
There was a problem hiding this comment.
🤖 Second pass on the same head (ac8129b), requested by @aparajon and performed by their agent — this one steps back from correctness and looks at the PR through two lenses: how easy this is to adopt as an OSS library, and what an orchestrator adapter has to do with it. Nothing here blocks; the correctness review above carries the findings that do.
The headline is a docs decision, and it's the right one: this PR promotes greenfield CREATE TABLE from 🟡 to ✅ and demotes CREATE TABLE ... PARTITION OF from ✅ to 🟡 in the same pass, because the create path refuses the form the imperative door never took. Shipping a feature and downgrading a neighbouring claim in one commit is rare, and it is the single strongest signal in this diff that the capability matrix can be trusted as written.
Lens 1 — adopting this as a library
The recurring ask is closed, and closed properly. I have asked on several PRs for a canonical documented list of outcome codes. docs/execution-model.md §"Outcome codes" now enumerates the closed vocabulary with a meaning per code, states the three facts an adapter should render, and — the part that makes it durable — pkg/executor/docs_test.go pins it mechanically against executor.Codes(), the same way pkg/preflight/docs_test.go pins the proof types. create-collision, duplicate-create-name, partition-of-unsupported, unsupported-create-step and if-not-exists-unsupported are all present. Nothing further owed here; consider the item retired.
The greenfield capability row now reads as a design statement, not a status. It names the lock that would matter (REFERENCES taking SHARE ROW EXCLUSIVE on each referenced live table), says why it doesn't today (desired files refuse foreign keys), and lists every refused shape. An evaluator reading that row knows both what they get and what the boundary is. That is the level the rest of the matrix should be held to.
The remaining adoption gap is the door itself. The PR body makes the case that declaring a table on a fresh database "blocks the most common first interaction anyone has with a desired-state tool" — and it is right. But RunDesired is library-only, so an evaluator's actual first interaction is still pg-sprite migrate with one imperative statement, which does not take CREATE TABLE at all. docs/limitations.md now says this plainly, which is the honest thing, but README.md is where someone lands first. One line there — desired-state execution is a Go API today, here is the entry point — would stop an evaluator concluding the feature doesn't exist because the CLI won't do it.
Terminology is clean. No "migration" outside the migrate package and CLI verb, which is the agreed carve-out for Spirit parity.
Lens 2 — what an orchestrator adapter has to do with this
Greenfield changes routing class, not just outcome. Before this PR, a desired file for a table that doesn't exist produced unsupported-statement — an author error, surfaced on the PR, never retried. After it, that same file produces one of four things: an executed plan, create-collision, a PrivilegeError at TierCreateTable, or an admission refusal. Two of those are new routing classes for this input: create-collision means re-diff the live catalog, and the privilege failure is an operator provisioning action — the role needs a GRANT, and the error carries the exact statement. An adapter whose greenfield arm currently reads "author error, tell them to fix the file" is now wrong for two of the four outcomes. That transition deserves a row in docs/schemabot-integration.md saying so explicitly, because the failure mode is silent: the adapter keeps compiling and starts giving operators the wrong instruction.
The committed-prefix story is already right, and should be linked from the new entry. A create that fails at step 3 leaves real relations; the rerun's absence check refuses with ErrRelationExists; the correct orchestrator behaviour is to keep the gate closed until someone re-diffs and converges the remainder — never to treat the failed run as a no-op because a later commit dropped the table from the desired file. The integration doc says this in the create-path section. Now that RunDesired is the way in, the RunDesired description should point at that paragraph rather than leaving an adapter to find it.
Render the verdicts from one source. On success runCreate builds result.Verdicts from len(rep.Steps) but writes Detail from len(report.Statements) — "created: all N planned statements committed". planStatementSQL exists precisely because the code declines to assume those two are equal, so the success detail is asserting an equality its neighbour guards against. Deriving both from the same count would make the disclosure self-consistent, and would mean a future divergence shows up as a smaller number rather than as a claim that is quietly false.
Progress is still step-indexed, not step-named — and now it matters. tracker.StartStep(i+1, progress.OperationBrief) gives a progress renderer "step 2 of 3". I noted this on #62 as a nice-to-have; the front door opening changes that, because greenfield is the case where the full step list is known before execution starts and where the steps are the most legible to a human ("creating table sessions", "building index sessions_user_id_idx"). An operator watching a fresh table get built is the friendliest possible progress surface, and it is currently the least specific one.
One thing worth stating in docs/invariants.md. The execution-order invariant is now enforced in three places — ParseDesired establishes it, diffplan.qualifiedDesired asserts it, admitCreateSteps asserts it under the ST-7 tag — and the position mapping between plan statements and verdicts depends on all three agreeing. That is a genuine cross-package invariant and it currently lives in three doc comments. It reads like an ST-numbered entry.
Suggested follow-ups
- Add a
README.mdline pointing at desired-state execution as a Go API, so an evaluator does not conclude greenfield is unsupported from the CLI alone. - Add a
RunDesiredgreenfield row todocs/schemabot-integration.mdnaming the four outcomes and their routing classes — especially that the privilege failure is an operator action, not an author one. - Link the committed-prefix / gate-stays-closed paragraph from that row.
- Derive
runCreate's successDetailfrom the same count as its verdicts. - (nice to have) Name the create path's steps in progress reporting now that the full step list is known up front.
- (nice to have) Promote the execution-order rule to a numbered entry in
docs/invariants.md, since three packages now assert it.
This review was generated by Claude Code (claude-opus-5).
Desired-state execution now creates a table that does not exist yet, instead of refusing the greenfield plan.
Why
The absence preflight (
CheckTableAbsent), the creation-privilege preflight (CheckCreatePrivileges), and the executor create path (ExecuteCreate) all exist, but the declarative front door never routed to them — a desired file for a brand-new table was refused withunsupported-statement, which blocks the most common first interaction anyone has with a desired-state tool: declaring a table on a fresh database.What
migrate.RunDesiredon a greenfield plan verifies the name is free and the role holdsCREATEon the schema, then hands the desired schema toExecuteCreate(consuming both proofs); a rerun converges to an empty plan.create-collision(added toverdict.Reasons()); a privilege gap refuses withinsufficient-privileges;PARTITION OF/IF NOT EXISTSshapes keep refusing withunsupported-statementbefore anything runs.CREATE TABLEfirst (indexes keep input order after it), so plan order states execution order and per-statement verdicts map positionally.Before / after