From ab8ca3a32dce5747eeb43635d82717c109610140 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 28 Aug 2026 19:16:37 +1000 Subject: [PATCH 1/3] feat(migrate): create the table when the desired plan is greenfield 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 --- CHANGELOG.md | 13 ++ docs/capabilities.md | 4 +- docs/cli-output-examples.md | 3 +- docs/limitations.md | 1 + pkg/diffplan/diffplan.go | 21 +++- pkg/migrate/desired.go | 161 +++++++++++++++++++++--- pkg/migrate/desired_integration_test.go | 131 ++++++++++++++++++- pkg/migrate/desired_test.go | 12 +- pkg/preflight/tier.go | 26 +++- pkg/preflight/tier_test.go | 28 +++++ pkg/verdict/verdict.go | 7 ++ pkg/verdict/verdict_test.go | 1 + 12 files changed, 371 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5512461..4ed308a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,19 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed โ€” observable outcomes for automation callers +- **Desired-state execution now creates a table that does not exist yet** + instead of refusing the plan. `migrate.RunDesired` on a greenfield plan + verifies the target name is free and the role holds `CREATE` on the + schema, then runs the `CREATE TABLE` and the index builds as brief + bounded steps; a rerun converges to an empty plan. An occupied name is a + new typed refusal reason, **`create-collision`** (added to + `verdict.Reasons()`); `PARTITION OF` and `IF NOT EXISTS` shapes refuse + with `unsupported-statement` before anything runs. A caller that relied + on the previous greenfield `unsupported-statement` refusal now sees the + create execute. Greenfield plan statements are additionally ordered + `CREATE TABLE` first (indexes keep their input order after it), so the + plan states execution order and a greenfield plan's fingerprint changes + when the desired file listed an index before its table. - **`diff` now exits 2 when the derived plan contains a statement execution would refuse**, in all three output modes (default report, `--sql`, `--json`) โ€” the same CI-gate contract as `migrate --dry-run`. Previously diff --git a/docs/capabilities.md b/docs/capabilities.md index bc5fe79..cc6cde4 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -156,7 +156,7 @@ Status legend: โœ… T1 (supported today) ยท ๐ŸŸก T2 (planned; typed refusal today | Operation | Status | Online-safety problem? | Behavior and why | | --- | --- | --- | --- | -| `CREATE TABLE ... PARTITION OF` | โœ… | Yes | Executed, with a typed warning: creating a partition takes a brief `ACCESS EXCLUSIVE` on the **parent** and queues behind long-running queries | +| `CREATE TABLE ... PARTITION OF` | ๐ŸŸก | Yes | Typed refusal at both doors: the imperative door does not take `CREATE TABLE`, and the declarative create path refuses the form โ€” attaching a partition takes a brief `ACCESS EXCLUSIVE` on the **parent**, which the greenfield absence proof does not cover. The partition-aware flow is planned | | `ATTACH PARTITION` | โœ… | Yes | Executed; the safer idiom (pre-prove the bound with a validated `CHECK` so the attach skips its scan) is surfaced as guidance. A classify-first flow that constructs the proof itself is planned | | `DETACH PARTITION [CONCURRENTLY]` | โœ… | Yes | `CONCURRENTLY` is the idiom; the blocking form is rewritten to it | | Partitioned parents in the **declarative model** | ๐ŸŸก | Yes | Typed refusal: the model does not yet carry partition keys, and rendering a partitioned parent as a plain `CREATE TABLE` would be silently wrong | @@ -171,7 +171,7 @@ Status legend: โœ… T1 (supported today) ยท ๐ŸŸก T2 (planned; typed refusal today | Unlogged tables | ๐ŸŸก | Yes | Typed refusal: persistence is not modeled, converging it (`SET LOGGED`) is a full rewrite, and rendering the table as plain `CREATE TABLE` would silently change crash-safety | | Explicit column collations | ๐ŸŸก | Yes | Typed refusal: dropping a `COLLATE` clause from a rendered baseline silently changes sort order and index semantics; a collation delta cannot converge without a rewrite | | Columns whose default uses a sequence the column does not own | ๐ŸŸก | Yes | Typed refusal: in a desired-state model that sequence exists only inside the scratch transaction, so no derived plan can reference it. Column-owned (`serial`-style) sequences are fine | -| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | ๐ŸŸก | Yes โ€” a `REFERENCES` clause takes a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table | Planned as an owned operation. The new table itself has no readers or writers to protect; the online-safety problem is the `REFERENCES` clause, whose lock on each referenced live table queues behind long-running queries and blocks writers behind it โ€” exactly the run-it-under-a-bounded-`lock_timeout` job this engine owns and owner tooling does not do. The absence preflight (`CheckTableAbsent`), the creation-privilege preflight (`CheckCreatePrivileges`), and the executor create path (`ExecuteCreate` โ€” plain `CREATE TABLE` plus plain index builds; `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, and `IF NOT EXISTS` are typed refusals at admission, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth) are in place; the declarative front door does not route to them yet. `diff --sql` already emits the statement | +| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: the absence preflight (`CheckTableAbsent`) verifies the name is free, `CheckCreatePrivileges` verifies the role can create in the schema, and the executor runs the `CREATE TABLE` and the index builds as brief bounded steps under the engine's `lock_timeout` / `statement_timeout` budgets. An occupied name is a typed `create-collision` refusal; `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | ### Types and non-table objects diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 2e4f0b5..d04c743 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -77,7 +77,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | Reason | Meaning | |---|---| -| `unsupported-statement` | No safe path is known for the statement โ€” only `ALTER TABLE` and `CREATE INDEX` reach classification โ€” or a desired-state plan needs a table that does not exist yet. | +| `unsupported-statement` | No safe path is known for the statement โ€” only `ALTER TABLE` and `CREATE INDEX` reach classification โ€” or a greenfield create plan carries a shape the create path refuses (`PARTITION OF`, `IF NOT EXISTS`). | | `index-statement` | Index maintenance (`DROP INDEX`, `REINDEX`) has a native safe idiom (`CONCURRENTLY`) and is never attempted; the verdict's `safer_idiom` names it. | | `not-native-safe-table-too-large` | The size guard skipped the optimistic attempt: the table exceeds the configured bound and the change is not provably metadata-only. | | `insufficient-privileges` | The connected role lacks the access the change needs; `detail` names the exact missing GRANT (see [engine-role.md](engine-role.md)). | @@ -87,6 +87,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `backend-unavailable` | The change routes to an execution strategy this build does not implement (copy-and-swap). | | `destructive-change` | The desired-state plan discards live structure โ€” a dropped column, constraint, index, or `NOT NULL` โ€” and desired-state execution runs no destructive statement; run the drop deliberately instead ([execution model](execution-model.md)). | | `plan-fingerprint-mismatch` | The plan recomputed at execution time does not carry the pinned fingerprint: the plan a reviewer approved is not the plan that would execute, so nothing runs ([execution model](execution-model.md)). | +| `create-collision` | The greenfield create plan's target name is already occupied โ€” a relation or standalone type took it after the plan was derived. Nothing runs; re-derive the plan against the live catalog and review what it says now. | ## Migrate diff --git a/docs/limitations.md b/docs/limitations.md index 202e0b1..de48d85 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -41,6 +41,7 @@ composition of the model boundaries above with those gates. At a glance: | Desired-file edit | Outcome today | | --- | --- | +| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the name is free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and the index builds as brief bounded steps. An occupied name (a relation or standalone type) is a typed `create-collision` refusal; `PARTITION OF` and `IF NOT EXISTS` are typed refusals before anything runs. | | Add a column | Converges. Runs as a bounded attempt of the submitted form, so the table-size guard applies (below). | | Widen a column type (`varchar(50)` โ†’ `varchar(255)`) | Converges โ€” the same bounded attempt, under the same size guard. | | Add an index | Converges via `CREATE INDEX CONCURRENTLY`. Not size-guarded: long online work on a large table is the pattern's purpose. | diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index 26899a5..dd7b646 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -153,20 +153,31 @@ func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plan.S } // qualifiedDesired renders the desired statements as the plan for a table -// that does not exist yet, qualified onto the target schema. +// that does not exist yet, qualified onto the target schema. The CREATE +// TABLE is ordered first regardless of its input position โ€” an index +// cannot be built before its table exists โ€” and the indexes keep their +// input order after it, so the plan states the exact order the create +// path executes and a plan statement's verdict is the verdict of the step +// at the same position. func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.Change, error) { statements := ds.Statements() changes := make([]schemadiff.Change, 0, len(statements)) + var create *schemadiff.Change for _, st := range statements { qualified, err := statement.Qualify(st.SQL(), schema) if err != nil { return nil, fmt.Errorf("qualify desired statement: %w", err) } - kind := schemadiff.ChangeCreateTable if st.Kind() == statement.KindCreateIndex { - kind = schemadiff.ChangeCreateIndex + changes = append(changes, schemadiff.Change{SQL: qualified, Kind: schemadiff.ChangeCreateIndex}) + continue } - changes = append(changes, schemadiff.Change{SQL: qualified, Kind: kind}) + create = &schemadiff.Change{SQL: qualified, Kind: schemadiff.ChangeCreateTable} } - return changes, nil + if create == nil { + // A DesiredSchema proof guarantees exactly one CREATE TABLE; a set + // without one here means the proof was forged or mutated. + return nil, errors.New("desired schema carries no CREATE TABLE") + } + return append([]schemadiff.Change{*create}, changes...), nil } diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index ce8ba2f..47c4b53 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -10,6 +10,7 @@ import ( "github.com/block/pg-sprite/pkg/diffplan" "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/preflight" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" @@ -88,13 +89,21 @@ type DesiredResult struct { // statement, so a statement that became unsafe after planning refuses // instead of running โ€” stopping at the first refusal or failure. // -// Plan-time admission is all-or-nothing: a plan that needs a table that -// does not exist yet, contains a destructive statement, routes any -// statement away from execution, or does not match the pinned fingerprint -// is refused before anything runs. Execution-time semantics are -// committed-prefix: once statements start running, an executed statement -// stays committed even when a later one refuses or fails, and the result's -// verdicts disclose exactly how far convergence got. +// A table that does not exist yet takes the greenfield create path +// instead: the plan is the desired schema itself, and after the same +// whole-plan admission the executor's create path verifies the name is +// free and the role can create in the schema, then runs the CREATE TABLE +// and the index builds as brief bounded steps. An occupied name is a typed +// [verdict.ReasonCreateCollision] refusal โ€” the caller re-derives the plan +// against the live catalog rather than assuming the occupant's shape. +// +// Plan-time admission is all-or-nothing: a plan that contains a +// destructive statement, routes any statement away from execution, or +// does not match the pinned fingerprint is refused before anything runs. +// Execution-time semantics are committed-prefix: once statements start +// running, an executed statement stays committed even when a later one +// refuses or fails, and the result's verdicts disclose exactly how far +// convergence got. // // The result-and-error contract mirrors [Run]'s three shapes. A refusal โ€” // at plan admission or on a mid-plan statement โ€” returns the result with a @@ -137,6 +146,14 @@ func RunDesired(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, opt if refused, ok := admitPlan(req, report); !ok { return refused, nil } + if report.TableExists != nil && !*report.TableExists { + // The table does not exist: the plan is the desired schema itself + // and converging it means creating the table. The create path runs + // the whole plan through the executor's greenfield sequence โ€” the + // per-statement Run pipeline below states facts about an existing + // table and its gate refuses CREATE TABLE outright. + return runCreate(ctx, pool, req, report, opts) + } result := DesiredResult{Plan: report} for i, ps := range report.Statements { @@ -176,12 +193,130 @@ func RunDesired(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, opt return result, nil } +// runCreate is the greenfield branch of desired-state execution: the plan's +// table does not exist, so converging it means creating it. The absence and +// creation-access proofs are minted here โ€” in the session that executes, at +// the point of use โ€” and the executor's create path runs the CREATE TABLE +// first and then the index builds, each as one brief bounded step. +// +// The result mirrors the convergence loop's shapes. An occupied target name +// or a missing creation grant is a whole-plan refusal โ€” nothing has +// executed. Once steps start committing, semantics are committed-prefix: a +// created table stays created when a later index build fails, the verdicts +// disclose exactly how far the create got, and a rerun re-derives the plan +// against the live catalog โ€” which now sees the table โ€” and converges the +// remainder through the alter loop. +func runCreate(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, report plan.Report, opts Options) (DesiredResult, error) { + result := DesiredResult{Plan: report} + stopBefore := func(err error) (DesiredResult, error) { + result.Outcome = verdict.OutcomeFailed + result.Detail = committedPrefixDetail(0, len(report.Statements), stoppedBeforeVerdict) + return result, err + } + at, err := preflight.CheckTableAbsent(ctx, pool, req.Schema, report.Table) + if preflight.IsNameOccupied(err) { + result.Outcome = verdict.OutcomeRefused + result.Reason = verdict.ReasonCreateCollision + result.Detail = fmt.Sprintf( + "the plan creates %s.%s but the name is already occupied (%v); the live catalog changed "+ + "since the plan was derived โ€” re-derive the plan and review what it says now; nothing was executed", + report.Schema, report.Table, err) + return result, nil + } + if err != nil { + return stopBefore(fmt.Errorf("verify %s.%s is absent: %w", report.Schema, report.Table, err)) + } + role, err := preflight.CheckCreatePrivileges(ctx, pool, req.Schema) + var privErr *preflight.PrivilegeError + if errors.As(err, &privErr) { + result.Outcome = verdict.OutcomeRefused + result.Reason = verdict.ReasonInsufficientPrivileges + result.Detail = privErr.Error() + "; nothing was executed" + return result, nil + } + if err != nil { + return stopBefore(fmt.Errorf("verify creation access in schema %s: %w", report.Schema, err)) + } + opts.logger().Debug("create preflight passed", + "schema", at.Schema(), "table", at.Table(), "role", role.Role()) + + rep, execErr := executor.ExecuteCreate(ctx, pool, at, role, req.Desired, opts.Budget.Brief, opts.retry()) + // The plan's statements and the executor's steps share one order โ€” the + // CREATE TABLE first, then the indexes in input order โ€” so the verdict + // at position i is the verdict of Plan.Statements[i]. + for i := range rep.Steps { + result.Verdicts = append(result.Verdicts, createStepVerdict(report, i, opts)) + } + if execErr == nil { + result.Outcome = verdict.OutcomeExecuted + result.Detail = fmt.Sprintf("created: all %d planned statements committed", len(report.Statements)) + return result, nil + } + var stepErr *executor.SequenceStepError + if !errors.As(execErr, &stepErr) { + // No step error means nothing started: the executor refused the + // set at admission, from the statements' shapes alone. + if isCreateAdmissionRefusal(execErr) { + result.Outcome = verdict.OutcomeRefused + result.Reason = verdict.ReasonUnsupportedStatement + result.Detail = fmt.Sprintf("the create path refused the plan: %v; nothing was executed", execErr) + return result, nil + } + return stopBefore(fmt.Errorf("create %s.%s: %w", report.Schema, report.Table, execErr)) + } + failed := verdict.Verdict{ + Outcome: verdict.OutcomeFailed, + Code: string(executor.OutcomeCode(execErr)), + Statement: planStatementSQL(report, stepErr.Step-1), + Table: report.Schema + "." + report.Table, + Detail: "the step's bounded attempt failed and rolled back; Code names the outcome", + } + result.Verdicts = append(result.Verdicts, failed) + result.Outcome = verdict.OutcomeFailed + result.Detail = committedPrefixDetail(stepErr.Step-1, len(report.Statements), "failed") + return result, fmt.Errorf("planned statement %d: %w", stepErr.Step, execErr) +} + +// createStepVerdict renders one committed create-path step as the executed +// verdict of the plan statement at the same position. +func createStepVerdict(report plan.Report, i int, opts Options) verdict.Verdict { + return verdict.Verdict{ + Outcome: verdict.OutcomeExecuted, + Statement: planStatementSQL(report, i), + Table: report.Schema + "." + report.Table, + Detail: fmt.Sprintf("committed within budgets (lock %s, statement %s): the change was effectively instant", + opts.Budget.Brief.LockTimeout, opts.Budget.Brief.StatementTimeout), + } +} + +// planStatementSQL returns the plan statement at i, empty when the position +// is out of range โ€” a defensive read: the executor's step count equals the +// plan's statement count by construction, and a mismatch must not panic a +// result renderer. +func planStatementSQL(report plan.Report, i int) string { + if i < 0 || i >= len(report.Statements) { + return "" + } + return report.Statements[i].SQL +} + +// isCreateAdmissionRefusal reports whether err is one of the create path's +// static admission refusals: decided from the desired statements' shapes +// before anything executes, so it maps to a refusal verdict, not an +// operational error. +func isCreateAdmissionRefusal(err error) bool { + return errors.Is(err, executor.ErrPartitionOfUnsupported) || + errors.Is(err, executor.ErrIfNotExistsUnsupported) || + errors.Is(err, executor.ErrUnsupportedCreateStep) || + errors.Is(err, executor.ErrDuplicateCreateName) +} + // admitPlan is the all-or-nothing plan-time admission: it refuses the whole // plan โ€” before anything runs โ€” when the plan cannot converge the table as // a unit. The checks run from the caller's contract outward: the pinned // fingerprint first (the caller's approval is void whatever else holds), -// then the table's existence, then the destructive guard, then the routed -// dispositions. An empty (already-converged) plan never reaches admission: +// then the destructive guard, then the routed dispositions. An empty +// (already-converged) plan never reaches admission: // the caller resolves it first, because it carries no plan identity for // the pin to verify and nothing would run anyway. func admitPlan(req DesiredRequest, report plan.Report) (DesiredResult, bool) { @@ -194,14 +329,6 @@ func admitPlan(req DesiredRequest, report plan.Report) (DesiredResult, bool) { report.Fingerprint, req.ExpectedFingerprint) return refused, false } - if report.TableExists != nil && !*report.TableExists { - refused.Reason = verdict.ReasonUnsupportedStatement - refused.Detail = fmt.Sprintf( - "table %s.%s does not exist; desired-state execution converges an existing table โ€” "+ - "create the table from the plan's SQL script first", - report.Schema, report.Table) - return refused, false - } for i, ps := range report.Statements { if !ps.Destructive { continue diff --git a/pkg/migrate/desired_integration_test.go b/pkg/migrate/desired_integration_test.go index 65f78ad..12750cd 100644 --- a/pkg/migrate/desired_integration_test.go +++ b/pkg/migrate/desired_integration_test.go @@ -2,14 +2,17 @@ package migrate_test import ( "fmt" + "net/url" "testing" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/diffplan" + "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/migrate" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" @@ -27,8 +30,8 @@ func parseDesired(t *testing.T, sql string) statement.DesiredSchema { // no-op re-run, the plan-time admission refusals, the fingerprint pin, and // the committed-prefix shapes when execution stops partway. func TestRunDesired(t *testing.T) { - url := testutil.StartPostgres(t) - pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + serverURL := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: serverURL}) require.NoError(t, err) defer pool.Close() @@ -70,14 +73,54 @@ CREATE INDEX t_v_idx ON t (v);` assert.Empty(t, res.Verdicts) }) - t.Run("refuses a greenfield plan and creates nothing", func(t *testing.T) { + t.Run("creates the greenfield table and re-runs as a no-op", func(t *testing.T) { schema := testutil.NewSchema(t, pool) + req := migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, desiredSQL)} + res, err := migrate.RunDesired(t.Context(), pool, req, runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, res.Outcome) + require.NotNil(t, res.Plan.TableExists) + assert.False(t, *res.Plan.TableExists, "the plan must record that the table was absent") + require.Len(t, res.Verdicts, len(res.Plan.Statements), + "every planned statement carries a verdict") + for _, v := range res.Verdicts { + assert.Equal(t, verdict.OutcomeExecuted, v.Outcome) + } + + var typ string + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT data_type FROM information_schema.columns + WHERE table_schema = $1 AND table_name = 't' AND column_name = 'v'`, schema).Scan(&typ)) + assert.Equal(t, "text", typ) + var indexValid bool + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT i.indisvalid FROM pg_index i + WHERE i.indexrelid = ($1 || '.t_v_idx')::regclass`, schema).Scan(&indexValid)) + assert.True(t, indexValid, "the index build must have completed and validated") + + // The convergence oracle: a second run derives an empty plan and + // runs nothing. + res, err = migrate.RunDesired(t.Context(), pool, req, runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, res.Outcome) + assert.Empty(t, res.Plan.Statements, "the created table plans no statements") + assert.Empty(t, res.Verdicts) + }) + + t.Run("refuses a create when a standalone type occupies the name", func(t *testing.T) { + // A standalone type is not a table, so the plan is greenfield โ€” + // but the create would collide with the type's own composite name. + // The absence check turns that into a typed whole-plan refusal. + schema := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TYPE %s.t AS ENUM ('a')", schema)) + require.NoError(t, err) + res, err := migrate.RunDesired(t.Context(), pool, migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, desiredSQL)}, runOptions()) - require.NoError(t, err, "a plan-time refusal is a result, not an error") + require.NoError(t, err, "an occupied name is a refusal, not an error") assert.Equal(t, verdict.OutcomeRefused, res.Outcome) - assert.Equal(t, verdict.ReasonUnsupportedStatement, res.Reason) + assert.Equal(t, verdict.ReasonCreateCollision, res.Reason) assert.Empty(t, res.Verdicts, "nothing was attempted") var exists bool @@ -87,6 +130,84 @@ CREATE INDEX t_v_idx ON t (v);` assert.False(t, exists, "the refused plan must not create the table") }) + t.Run("refuses a greenfield create without schema CREATE", func(t *testing.T) { + // The role precedes the schema so LIFO cleanup drops the schema โ€” + // and with it the grant the role depends on โ€” before the role. + const password = "desired-create-password" + role := testutil.NewRole(t, pool, "LOGIN PASSWORD '"+password+"'") + schema := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", + schema, pgx.Identifier{role}.Sanitize())) + require.NoError(t, err) + u, err := url.Parse(serverURL) + require.NoError(t, err) + u.User = url.UserPassword(role, password) + engine, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: u.String()}) + require.NoError(t, err) + defer engine.Close() + + res, err := migrate.RunDesired(t.Context(), engine, + migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, desiredSQL)}, runOptions()) + require.NoError(t, err, "a missing grant is a refusal, not an error") + assert.Equal(t, verdict.OutcomeRefused, res.Outcome) + assert.Equal(t, verdict.ReasonInsufficientPrivileges, res.Reason) + assert.Contains(t, res.Detail, "GRANT CREATE ON SCHEMA", + "the refusal names the exact provisioning statement") + assert.Empty(t, res.Verdicts, "nothing was attempted") + }) + + t.Run("refuses a desired PARTITION OF before anything runs", func(t *testing.T) { + schema := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.events (id int, at date, PRIMARY KEY (id, at)) PARTITION BY RANGE (at)", schema)) + require.NoError(t, err) + + res, err := migrate.RunDesired(t.Context(), pool, migrate.DesiredRequest{ + Schema: schema, + Desired: parseDesired(t, + "CREATE TABLE t PARTITION OF events FOR VALUES FROM ('2026-01-01') TO ('2026-02-01')"), + }, runOptions()) + require.NoError(t, err, "a create-path admission refusal is a result, not an error") + assert.Equal(t, verdict.OutcomeRefused, res.Outcome) + assert.Equal(t, verdict.ReasonUnsupportedStatement, res.Reason) + assert.Contains(t, res.Detail, "PARTITION OF") + assert.Empty(t, res.Verdicts, "nothing was attempted") + + var exists bool + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT EXISTS (SELECT 1 FROM information_schema.tables + WHERE table_schema = $1 AND table_name = 't')`, schema).Scan(&exists)) + assert.False(t, exists, "the refused plan must not create the partition") + }) + + t.Run("a failed index build keeps the created table and discloses the prefix", func(t *testing.T) { + // The desired index's name is already taken by an index on another + // table, so the create path commits the CREATE TABLE and stops on + // the index step โ€” committed-prefix semantics, disclosed by the + // verdicts, with the collision's stable code on the failed one. + schema := testutil.NewSchema(t, pool) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v text)", schema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE INDEX t_v_idx ON %s.other (v)", schema)) + require.NoError(t, err) + + res, err := migrate.RunDesired(t.Context(), pool, + migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, desiredSQL)}, runOptions()) + require.Error(t, err, "a mid-plan execution failure returns the failed result with the error") + assert.Equal(t, verdict.OutcomeFailed, res.Outcome) + require.Len(t, res.Verdicts, 2, "the committed create and the failed index build") + assert.Equal(t, verdict.OutcomeExecuted, res.Verdicts[0].Outcome) + assert.Contains(t, res.Verdicts[0].Statement, "CREATE TABLE") + assert.Equal(t, verdict.OutcomeFailed, res.Verdicts[1].Outcome) + assert.Equal(t, string(executor.CodeCreateCollision), res.Verdicts[1].Code) + + var exists bool + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT EXISTS (SELECT 1 FROM information_schema.tables + WHERE table_schema = $1 AND table_name = 't')`, schema).Scan(&exists)) + assert.True(t, exists, "the committed CREATE TABLE stays committed") + }) + t.Run("refuses a destructive plan and drops nothing", func(t *testing.T) { schema := testutil.NewSchema(t, pool) _, err := pool.Exec(t.Context(), fmt.Sprintf( diff --git a/pkg/migrate/desired_test.go b/pkg/migrate/desired_test.go index cc63e57..f25b93a 100644 --- a/pkg/migrate/desired_test.go +++ b/pkg/migrate/desired_test.go @@ -76,15 +76,15 @@ func TestAdmitPlan(t *testing.T) { assert.Equal(t, report, res.Plan, "a refusal carries the plan it refused") }) - t.Run("refuses a greenfield plan", func(t *testing.T) { + t.Run("admits a greenfield plan", func(t *testing.T) { + // A table that does not exist takes the create path after + // admission; admission itself only vets the plan's content โ€” the + // pin, the destructive guard, and the routed dispositions. report := executable() exists := false report.TableExists = &exists - res, ok := admitPlan(DesiredRequest{}, report) - require.False(t, ok) - assert.Equal(t, verdict.ReasonUnsupportedStatement, res.Reason) - assert.Contains(t, res.Detail, "app.t") - assert.Equal(t, report, res.Plan, "a refusal carries the plan it refused") + _, ok := admitPlan(DesiredRequest{}, report) + assert.True(t, ok) }) t.Run("refuses a destructive statement anywhere in the plan", func(t *testing.T) { diff --git a/pkg/preflight/tier.go b/pkg/preflight/tier.go index 7439c94..e5c4061 100644 --- a/pkg/preflight/tier.go +++ b/pkg/preflight/tier.go @@ -16,15 +16,32 @@ import ( // CheckPrivileges, so every consumer of the routed plan derives the same // answer. A step shape the engine does not execute fails closed here, // before anything runs. +// +// A CREATE TABLE step derives the off-ladder TierCreateTable โ€” the +// greenfield create plan's shape: one CREATE TABLE plus CREATE INDEX steps +// on the table it creates, checked by CheckCreatePrivileges, never by +// CheckPrivileges, whose ladder states facts about an existing table. A +// set that mixes CREATE TABLE with ALTER TABLE fails closed: the +// off-ladder tier proves creation access only and cannot vouch for the +// ladder rungs an alter on an existing table needs โ€” and no front door +// produces such a set. func RequiredTier(execSQL []string) (Tier, error) { tier := TierAlterInPlace + var createsTable, altersTable bool for _, sql := range execSQL { st, err := statement.ParseOne(sql) if err != nil { return 0, fmt.Errorf("derive privilege tier: %w", err) } switch st.Kind() { - case statement.KindAlterTable, statement.KindCreateIndex: + case statement.KindCreateTable: + createsTable = true + case statement.KindAlterTable: + altersTable = true + if st.BuildsIndex() { + tier = TierIndexBuild + } + case statement.KindCreateIndex: if st.BuildsIndex() { tier = TierIndexBuild } @@ -33,5 +50,12 @@ func RequiredTier(execSQL []string) (Tier, error) { sql, st.Kind()) } } + if createsTable { + if altersTable { + return 0, fmt.Errorf("derive privilege tier: the set mixes CREATE TABLE with ALTER TABLE; " + + "the off-ladder create tier cannot vouch for the ladder rungs an existing-table alter needs") + } + return TierCreateTable, nil + } return tier, nil } diff --git a/pkg/preflight/tier_test.go b/pkg/preflight/tier_test.go index 8160298..0021233 100644 --- a/pkg/preflight/tier_test.go +++ b/pkg/preflight/tier_test.go @@ -71,6 +71,22 @@ func TestRequiredTier(t *testing.T) { execSQL: []string{"ALTER TABLE s.t ALTER COLUMN c TYPE bigint"}, tier: preflight.TierAlterInPlace, }, + // The greenfield create plan derives the off-ladder create tier โ€” + // checked by CheckCreatePrivileges, never by the ladder walk โ€” + // whether or not the plan also builds the new table's indexes. + { + name: "create table derives the off-ladder create tier", + execSQL: []string{"CREATE TABLE s.t (id int)"}, + tier: preflight.TierCreateTable, + }, + { + name: "create plan with index builds stays the create tier", + execSQL: []string{ + "CREATE TABLE s.t (id int, c int)", + "CREATE INDEX t_c_idx ON s.t (c)", + }, + tier: preflight.TierCreateTable, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -89,4 +105,16 @@ func TestRequiredTier(t *testing.T) { _, err := preflight.RequiredTier([]string{"not sql at all"}) require.Error(t, err) }) + + t.Run("a set mixing create table with alter table fails closed", func(t *testing.T) { + // The off-ladder create tier proves creation access only; it + // cannot vouch for the ladder rungs an existing-table alter needs, + // and no front door produces such a set. + _, err := preflight.RequiredTier([]string{ + "CREATE TABLE s.t (id int)", + "ALTER TABLE s.u ADD COLUMN c int", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "mixes CREATE TABLE with ALTER TABLE") + }) } diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index a63a144..916cd75 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -87,6 +87,12 @@ const ( // does not carry the fingerprint the caller pinned, so the plan a // reviewer approved is not the plan that would execute; nothing runs. ReasonPlanFingerprintMismatch Reason = "plan-fingerprint-mismatch" + // ReasonCreateCollision: the create plan's target name is already + // occupied โ€” a relation or standalone type took it after the plan was + // derived โ€” so the greenfield create cannot run; the caller re-derives + // the plan against the live catalog rather than assuming the + // occupant's shape. + ReasonCreateCollision Reason = "create-collision" ) // Reasons returns the closed set of non-zero Reason values. It is part of @@ -105,6 +111,7 @@ func Reasons() []Reason { ReasonBackendUnavailable, ReasonDestructiveChange, ReasonPlanFingerprintMismatch, + ReasonCreateCollision, } } diff --git a/pkg/verdict/verdict_test.go b/pkg/verdict/verdict_test.go index 81eefac..5b47bc0 100644 --- a/pkg/verdict/verdict_test.go +++ b/pkg/verdict/verdict_test.go @@ -106,6 +106,7 @@ func TestReasonsPinsWireTokens(t *testing.T) { "backend-unavailable", "destructive-change", "plan-fingerprint-mismatch", + "create-collision", }, got) } From ac8129bf6c8cb4d0f1ae57a414ba89792f2b5864 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 28 Aug 2026 23:04:25 +1000 Subject: [PATCH 2/3] fix(statement): order desired statements for execution at parse 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. --- CHANGELOG.md | 11 ++++--- README.md | 7 +++-- docs/limitations.md | 2 +- docs/optimistic-attempt.md | 7 +++-- pkg/diffplan/diffplan.go | 33 ++++++++++----------- pkg/diffplan/diffplan_integration_test.go | 36 +++++++++++++++++++++++ pkg/executor/create.go | 32 ++++++++------------ pkg/migrate/desired_integration_test.go | 28 ++++++++++++++++++ pkg/schemadiff/desired.go | 2 ++ pkg/statement/desired.go | 32 +++++++++++++++++--- pkg/statement/desired_test.go | 20 +++++++++++++ 11 files changed, 159 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed308a..773fbc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,10 +17,13 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `verdict.Reasons()`); `PARTITION OF` and `IF NOT EXISTS` shapes refuse with `unsupported-statement` before anything runs. A caller that relied on the previous greenfield `unsupported-statement` refusal now sees the - create execute. Greenfield plan statements are additionally ordered - `CREATE TABLE` first (indexes keep their input order after it), so the - plan states execution order and a greenfield plan's fingerprint changes - when the desired file listed an index before its table. + create execute. Desired-file statements are additionally ordered for + execution at parse โ€” the `CREATE TABLE` first, indexes keeping their + input order after it โ€” everywhere the file replays: the greenfield plan, + the create path's steps, and the scratch-schema introspection that + derives a diff once the table exists. The plan states execution order, a + greenfield plan's fingerprint changes when the desired file listed an + index before its table, and an index-first file converges on rerun. - **`diff` now exits 2 when the derived plan contains a statement execution would refuse**, in all three output modes (default report, `--sql`, `--json`) โ€” the same CI-gate contract as `migrate --dry-run`. Previously diff --git a/README.md b/README.md index 69431ba..64be774 100644 --- a/README.md +++ b/README.md @@ -68,9 +68,10 @@ refusal โ€” never a silently wrong or incomplete result: - **Unlogged tables and explicit column collations** are outside the declarative model: converging either is a table (or column) rewrite, so export and diff refuse rather than plan one. -- **Greenfield `CREATE TABLE` apply** is not user-reachable yet: the - executor create path exists as a library building block, but the - declarative front door does not route to it. +- **Desired-state execution has no CLI verb yet** โ€” `migrate.RunDesired` + (including the greenfield `CREATE TABLE` path for a table that does not + exist) is library-only; the CLI's `migrate` takes one imperative + statement. - **Non-table objects** โ€” views, standalone sequences, enums, domains, extensions, functions, triggers โ€” are outside the declarative model, which covers one ordinary table plus its indexes per file. diff --git a/docs/limitations.md b/docs/limitations.md index de48d85..5a13998 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -29,7 +29,7 @@ with a typed refusal โ€” never a silently wrong or incomplete result: | Column collations | An explicit `COLLATE` on a column is not managed: converging a collation delta rewrites the column and its indexes. Export refuses a collated column โ€” a baseline without the clause would silently change sort order and index semantics โ€” and a collation delta (including on an added column) is a typed `diff` refusal. | | Non-table objects | Views, materialized views, standalone sequences, enums, domains, extensions, functions, and triggers are outside the model. A serial column's owned sequence is the one exception: it round-trips through the `serial` pseudo-types โ€” and ownership is verified through the catalog (`pg_depend`), so a hand-written `nextval` default on a standalone sequence that merely carries the serial-style name refuses rather than exporting as `serial` and silently privatizing a shared sequence. A column may *use* an unmanaged type (an enum, a domain) โ€” the type text round-trips โ€” but the type's definition is not managed. | | Multiple tables per file | A desired file is single-table scoped: exactly one `CREATE TABLE` plus `CREATE INDEX` statements on it. Multi-table schemas are managed as one file per table. | -| Greenfield table creation | Not user-reachable yet: the executor create path (`executor.ExecuteCreate`) exists as a library building block, but the declarative front door does not route to it. The path runs a plain `CREATE TABLE` plus plain index builds on the table born that run, and refuses at admission โ€” before anything executes โ€” every clause that binds to an existing object the absence proof does not cover: `PARTITION OF`, `INHERITS`, `LIKE`, and `OF`, plus `IF NOT EXISTS`. `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse (`statement.ParseDesired`); the create path's admission re-checks them as defense in depth. | +| Greenfield table creation | Reachable through desired-state execution (`migrate.RunDesired`, library-only today โ€” no CLI verb): a plan whose table does not exist routes to the executor create path (`executor.ExecuteCreate`). The path runs a plain `CREATE TABLE` plus plain index builds on the table born that run, and refuses at admission โ€” before anything executes โ€” every clause that binds to an existing object the absence proof does not cover: `PARTITION OF`, `INHERITS`, `LIKE`, and `OF`, plus `IF NOT EXISTS`. `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse (`statement.ParseDesired`); the create path's admission re-checks them as defense in depth. | | Changed index or constraint definition | A redefinition diffs to drop-and-recreate, the drop is destructive, and desired-state execution refuses any plan containing a destructive statement โ€” the whole plan, including the harmless recreate. Run the drop deliberately first (`DROP INDEX CONCURRENTLY` directly against the database; `ALTER TABLE ... DROP CONSTRAINT` through the imperative front door), then rerun โ€” the remaining plan converges the recreate. | ## What desired-state execution converges today diff --git a/docs/optimistic-attempt.md b/docs/optimistic-attempt.md index e1f4953..7c08ac8 100644 --- a/docs/optimistic-attempt.md +++ b/docs/optimistic-attempt.md @@ -154,9 +154,10 @@ What happens to one statement, in order: verb โ€” [limitations.md](limitations.md)), and runs a whole-plan admission gate before any statement enters the walk: the plan is refused all-or-nothing when the plan derived at execution time is not the pinned one (`plan-fingerprint-mismatch`), - the target table does not exist (`unsupported-statement`), any planned statement - discards live structure (`destructive-change`), or the plan as a whole does not - route to execute. Past admission, each derived statement walks the same gates + any planned statement discards live structure (`destructive-change`), or the plan + as a whole does not route to execute. A plan whose table does not exist routes past + admission to the executor's greenfield create path โ€” brief bounded steps, not the + per-statement walk below. Past admission, each derived statement walks the same gates below โ€” including the size guard, which is per-statement, never plan-level: a multi-statement plan can be refused at statement 3 with statements 1 and 2 already committed (the committed prefix remains, Exit 7). diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index dd7b646..e2b2ac4 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -153,31 +153,30 @@ func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plan.S } // qualifiedDesired renders the desired statements as the plan for a table -// that does not exist yet, qualified onto the target schema. The CREATE -// TABLE is ordered first regardless of its input position โ€” an index -// cannot be built before its table exists โ€” and the indexes keep their -// input order after it, so the plan states the exact order the create -// path executes and a plan statement's verdict is the verdict of the step -// at the same position. +// that does not exist yet, qualified onto the target schema. The statements +// arrive in execution order โ€” the CREATE TABLE first, the indexes in input +// order after it, ordered once by statement.ParseDesired โ€” so the plan +// states the exact order the create path executes and a plan statement's +// verdict is the verdict of the step at the same position. func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.Change, error) { statements := ds.Statements() + if len(statements) == 0 || statements[0].Kind() != statement.KindCreateTable { + // A DesiredSchema proof guarantees a CREATE TABLE ordered first; a + // set that does not lead with one means the proof was forged or + // mutated. + return nil, errors.New("desired schema does not lead with a CREATE TABLE") + } changes := make([]schemadiff.Change, 0, len(statements)) - var create *schemadiff.Change for _, st := range statements { qualified, err := statement.Qualify(st.SQL(), schema) if err != nil { return nil, fmt.Errorf("qualify desired statement: %w", err) } - if st.Kind() == statement.KindCreateIndex { - changes = append(changes, schemadiff.Change{SQL: qualified, Kind: schemadiff.ChangeCreateIndex}) - continue + kind := schemadiff.ChangeCreateIndex + if st.Kind() == statement.KindCreateTable { + kind = schemadiff.ChangeCreateTable } - create = &schemadiff.Change{SQL: qualified, Kind: schemadiff.ChangeCreateTable} - } - if create == nil { - // A DesiredSchema proof guarantees exactly one CREATE TABLE; a set - // without one here means the proof was forged or mutated. - return nil, errors.New("desired schema carries no CREATE TABLE") + changes = append(changes, schemadiff.Change{SQL: qualified, Kind: kind}) } - return append([]schemadiff.Change{*create}, changes...), nil + return changes, nil } diff --git a/pkg/diffplan/diffplan_integration_test.go b/pkg/diffplan/diffplan_integration_test.go index ce6d539..7ef7b2d 100644 --- a/pkg/diffplan/diffplan_integration_test.go +++ b/pkg/diffplan/diffplan_integration_test.go @@ -230,3 +230,39 @@ func TestPlanMissingTableEmitsFullDesiredSchema(t *testing.T) { schemadiff.ChangeCreateIndex, }, kinds) } + +// A greenfield plan must state execution order even when the desired file +// lists an index before its table: the CREATE TABLE is planned first, so a +// plan statement's verdict is the verdict of the create-path step at the +// same position. +func TestPlanMissingTableOrdersCreateTableFirst(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + report, err := diffplan.Plan(t.Context(), pool, diffplan.Request{ + Schema: schema, + Desired: parseDesired(t, + "CREATE INDEX events_id_idx ON events (id);\nCREATE TABLE events (id bigint PRIMARY KEY);"), + }) + require.NoError(t, err) + + require.NotNil(t, report.TableExists) + assert.False(t, *report.TableExists) + var sqls []string + var kinds []schemadiff.ChangeKind + for _, ch := range report.Statements { + sqls = append(sqls, ch.SQL) + kinds = append(kinds, ch.Kind) + } + assert.Equal(t, []string{ + fmt.Sprintf("CREATE TABLE %s.events (id bigint PRIMARY KEY)", schema), + fmt.Sprintf("CREATE INDEX events_id_idx ON %s.events USING btree (id)", schema), + }, sqls) + assert.Equal(t, []schemadiff.ChangeKind{ + schemadiff.ChangeCreateTable, + schemadiff.ChangeCreateIndex, + }, kinds) +} diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 7187d64..1fd324f 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -170,10 +170,10 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT } // admitCreateSteps qualifies every desired statement into the proof's -// schema, re-parses it, and admits it by shape and target. The CREATE -// TABLE is ordered first regardless of its input position โ€” an index -// cannot be built before its table exists โ€” and the indexes keep their -// input order after it. Every step claims the names it will occupy in the +// schema, re-parses it, and admits it by shape and target. The statements +// arrive in execution order โ€” the CREATE TABLE first, the indexes in input +// order after it, ordered once by statement.ParseDesired โ€” and the steps +// keep that order. Every step claims the names it will occupy in the // same pg_class namespace โ€” the table plus the first-choice index names // of its index-backed constraints, or an explicit index name โ€” so a name // claimed twice within the set โ€” decidable here โ€” is refused before @@ -185,9 +185,13 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // index) claims nothing decidable and is exempt. func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([]statement.Statement, error) { desired := ds.Statements() - var createStep statement.Statement - var haveCreate bool - indexSteps := make([]statement.Statement, 0, len(desired)) + // INV: ST-7 โ€” a DesiredSchema proof guarantees a CREATE TABLE ordered + // first; a set that does not lead with one means the proof was forged + // or mutated. + if len(desired) == 0 || desired[0].Kind() != statement.KindCreateTable { + return nil, fmt.Errorf("%w: ST-7: desired schema does not lead with a CREATE TABLE", ErrInvariantViolation) + } + steps := make([]statement.Statement, 0, len(desired)) claimed := make(map[string]struct{}, len(desired)) for i, raw := range desired { st, names, err := admitCreateStep(at, raw.SQL()) @@ -200,19 +204,9 @@ func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([] } claimed[name] = struct{}{} } - if st.Kind() == statement.KindCreateTable { - createStep = st - haveCreate = true - continue - } - indexSteps = append(indexSteps, st) - } - if !haveCreate { - // A DesiredSchema proof guarantees exactly one CREATE TABLE; a - // set without one here means the proof was forged or mutated. - return nil, fmt.Errorf("%w: ST-7: desired schema admitted without a CREATE TABLE", ErrInvariantViolation) + steps = append(steps, st) } - return append([]statement.Statement{createStep}, indexSteps...), nil + return steps, nil } // admitCreateStep qualifies one desired statement into the proof's schema, diff --git a/pkg/migrate/desired_integration_test.go b/pkg/migrate/desired_integration_test.go index 12750cd..1a60dfd 100644 --- a/pkg/migrate/desired_integration_test.go +++ b/pkg/migrate/desired_integration_test.go @@ -108,6 +108,34 @@ CREATE INDEX t_v_idx ON t (v);` assert.Empty(t, res.Verdicts) }) + t.Run("creates from an index-first desired file and re-runs as a no-op", func(t *testing.T) { + // The index precedes its table in the file. Run 1 must still create + // table-first, and run 2 โ€” where the desired file replays on the + // scratch schema โ€” must converge rather than error on the index + // referencing a table that does not exist yet. + schema := testutil.NewSchema(t, pool) + + req := migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, + "CREATE INDEX t_v_idx ON t (v);\nCREATE TABLE t (id int PRIMARY KEY, v text);")} + res, err := migrate.RunDesired(t.Context(), pool, req, runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, res.Outcome) + require.Len(t, res.Verdicts, len(res.Plan.Statements)) + var indexValid bool + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT i.indisvalid FROM pg_index i + WHERE i.indexrelid = ($1 || '.t_v_idx')::regclass`, schema).Scan(&indexValid)) + assert.True(t, indexValid, "the index build must have completed and validated") + + // The convergence oracle: a second run derives an empty plan and + // runs nothing. + res, err = migrate.RunDesired(t.Context(), pool, req, runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, res.Outcome) + assert.Empty(t, res.Plan.Statements, "the created table plans no statements") + assert.Empty(t, res.Verdicts) + }) + t.Run("refuses a create when a standalone type occupies the name", func(t *testing.T) { // A standalone type is not a table, so the plan is greenfield โ€” // but the create would collide with the type's own composite name. diff --git a/pkg/schemadiff/desired.go b/pkg/schemadiff/desired.go index 23ed419..cb34dd6 100644 --- a/pkg/schemadiff/desired.go +++ b/pkg/schemadiff/desired.go @@ -46,6 +46,8 @@ func IntrospectDesired(ctx context.Context, db *pgxpool.Pool, desired statement. if _, err := tx.Exec(ctx, setPath); err != nil { return Model{}, fmt.Errorf("set scratch search_path: %w", err) } + // Statements arrive in execution order โ€” the CREATE TABLE first โ€” so + // an index never replays before the table it targets exists. for _, st := range desired.Statements() { if _, err := tx.Exec(ctx, st.SQL()); err != nil { return Model{}, fmt.Errorf("execute desired statement on scratch schema: %w", err) diff --git a/pkg/statement/desired.go b/pkg/statement/desired.go index cce1f05..e98480e 100644 --- a/pkg/statement/desired.go +++ b/pkg/statement/desired.go @@ -46,7 +46,8 @@ var ( // DesiredSchema is a validated desired-state schema file: exactly one // CREATE TABLE plus any number of CREATE INDEX statements on that table. // Statement SQL is canonical (parsed and deparsed through the PostgreSQL -// grammar), in input order, one statement per entry. +// grammar), one statement per entry, held in execution order: the CREATE +// TABLE first, the indexes in input order after it. // // Only [ParseDesired] produces a non-zero value, so holding one is proof // the set-level admission rules held: a single unqualified CREATE TABLE, @@ -59,9 +60,11 @@ type DesiredSchema struct { // Table returns the unqualified name of the single CREATE TABLE target. func (ds DesiredSchema) Table() string { return ds.table } -// Statements returns the admitted statements in input order, the CREATE -// TABLE among them. The slice is a copy: mutating it cannot invalidate the -// admission proof the value carries. +// Statements returns the admitted statements in execution order: the +// CREATE TABLE first, the indexes in input order after it โ€” an index +// cannot be built before its table exists, and every consumer replays +// this order verbatim. The slice is a copy: mutating it cannot invalidate +// the admission proof the value carries. func (ds DesiredSchema) Statements() []Statement { return slices.Clone(ds.statements) } // ParseDesired parses a desired-state schema file and admits only what the @@ -99,9 +102,30 @@ func ParseDesired(sql string) (DesiredSchema, error) { ErrWrongIndexTarget, st.table, ds.table) } } + ds.statements = executionOrder(ds.statements) return ds, nil } +// executionOrder hoists the CREATE TABLE to the front, keeping the indexes +// in input order after it. Ordering once at admission โ€” rather than at each +// replay site โ€” means every consumer of the proof (the scratch-schema +// introspection, the greenfield plan, the create path's steps) executes an +// index-before-table file correctly by construction. +func executionOrder(statements []Statement) []Statement { + ordered := make([]Statement, 0, len(statements)) + for _, st := range statements { + if st.kind == KindCreateTable { + ordered = append(ordered, st) + } + } + for _, st := range statements { + if st.kind != KindCreateTable { + ordered = append(ordered, st) + } + } + return ordered +} + // admitDesiredStatement applies the per-statement admission rules and // returns the statement's kind and target. seenTable is the CREATE TABLE // target admitted so far, empty when none. diff --git a/pkg/statement/desired_test.go b/pkg/statement/desired_test.go index fb949f3..2df3b61 100644 --- a/pkg/statement/desired_test.go +++ b/pkg/statement/desired_test.go @@ -24,6 +24,26 @@ create index events_name_idx on events (name);`) assert.Equal(t, "CREATE INDEX events_name_idx ON events USING btree (name)", statements[1].SQL()) } +// A desired file may list an index before its table; Statements must +// return execution order โ€” the CREATE TABLE first โ€” because every +// consumer replays the slice verbatim and an index cannot be built +// before its table exists. +func TestParseDesiredOrdersTableFirst(t *testing.T) { + ds, err := ParseDesired(`create index events_name_idx on events (name); +create table events (id bigint primary key, name varchar(50) not null); +create index events_id_idx on events (id);`) + require.NoError(t, err) + + statements := ds.Statements() + require.Len(t, statements, 3) + assert.Equal(t, KindCreateTable, statements[0].Kind()) + assert.Equal(t, KindCreateIndex, statements[1].Kind()) + assert.Equal(t, "CREATE INDEX events_name_idx ON events USING btree (name)", statements[1].SQL(), + "indexes keep their input order after the hoisted CREATE TABLE") + assert.Equal(t, KindCreateIndex, statements[2].Kind()) + assert.Equal(t, "CREATE INDEX events_id_idx ON events USING btree (id)", statements[2].SQL()) +} + // Statements returns a copy: mutating the returned slice must not change // what a later caller observes, so a validated DesiredSchema stays valid. func TestDesiredSchemaStatementsIsDefensiveCopy(t *testing.T) { From d2504e81cd64aae7aa13f5a88c6294a910f45d4d Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Sun, 30 Aug 2026 15:17:09 +1000 Subject: [PATCH 3/3] address second-round review: pin search_path on alter path, doc fixes Alter attempts now run with search_path pinned to the target schema (same contract ExecuteCreate already had), with a regression test. Doc call order reconciled with runCreate (absence before privileges), if-not-exists-unsupported added to the refusal routing table, greenfield routing-class table and README Go-API pointer added, success Detail derived from the executed step count, and the parse-time statement ordering guarantee promoted to invariant ST-8. --- CHANGELOG.md | 8 +++++ README.md | 4 +++ docs/execution-model.md | 13 ++++--- docs/invariants.md | 15 ++++++++ docs/schemabot-integration.md | 30 ++++++++++++++-- pkg/diffplan/diffplan.go | 6 ++-- pkg/executor/create.go | 4 +-- pkg/executor/optimistic.go | 21 ++++++----- pkg/executor/optimistic_integration_test.go | 40 +++++++++++++++++++++ pkg/migrate/desired.go | 5 ++- 10 files changed, 126 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 773fbc7..7eb26ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). derives a diff once the table exists. The plan states execution order, a greenfield plan's fingerprint changes when the desired file listed an index before its table, and an index-first file converges on rerun. +- **Alter attempts now run with `search_path` pinned to the target + schema** (then `public`) whenever the statement is schema-qualified โ€” + the same resolution the create path and introspection use. A statement's + unqualified secondary names โ€” a column's type, an expression's + function โ€” resolve in the target schema, where previously they resolved + via the session's ambient `search_path` and could silently bind a + same-named object in `public`. A caller that relied on ambient + resolution for secondary names must qualify them. - **`diff` now exits 2 when the derived plan contains a statement execution would refuse**, in all three output modes (default report, `--sql`, `--json`) โ€” the same CI-gate contract as `migrate --dry-run`. Previously diff --git a/README.md b/README.md index 64be774..1d5b0fb 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,10 @@ routes the statement, then executes the routed SQL โ€” the planner's safer nativ default when the submitted form blocks (reported in the verdict's `executed_sql`), a bounded optimistic native attempt otherwise. A gated `--force` runs the submitted form as-is under the same budgets. Changes without an available backend get a structured refusal (exit code 2). +Desired-state execution โ€” converging a live table onto a `CREATE TABLE` file, including +creating the table when it does not exist yet โ€” is a Go API today: `migrate.RunDesired` +in [`pkg/migrate`](pkg/migrate/desired.go); the CLI's `migrate` verb takes one imperative +statement. The design docs and the phased build plan live in [docs/](docs/) โ€” start with [docs/README.md](docs/README.md); the vision โ€” what pg-sprite is and is not โ€” diff --git a/docs/execution-model.md b/docs/execution-model.md index d003406..9f76dc4 100644 --- a/docs/execution-model.md +++ b/docs/execution-model.md @@ -56,10 +56,15 @@ autocommit-each-step has two shapes in the executor: - **Brief catalog steps (step kind `brief`) and `VALIDATE CONSTRAINT` (step kind `validate-constraint`)** each run as one short *explicit* transaction: `BEGIN` โ†’ `SET LOCAL lock_timeout` / `statement_timeout` โ†’ the statement โ†’ - `COMMIT` (`pkg/executor`'s bounded runner). The explicit `BEGIN` exists - only because the budgets are applied with `SET LOCAL`, which is scoped to - that transaction โ€” functionally it is still one statement, one - transaction, committed immediately, rolled back atomically on failure. + `COMMIT` (`pkg/executor`'s bounded runner). When the preflight proof + carries a schema, the same `SET LOCAL` pins `search_path` to that schema + then `public`, so the statement's unqualified secondary names โ€” a + column's type, an expression's function โ€” resolve in the target schema, + exactly as the introspection read path resolves them. The explicit + `BEGIN` exists only because the settings are applied with `SET LOCAL`, + which is scoped to that transaction โ€” functionally it is still one + statement, one transaction, committed immediately, rolled back + atomically on failure. - **`CREATE INDEX CONCURRENTLY` (step kind `concurrent-index-build`)** is true autocommit on a dedicated budgeted session: it refuses to run inside any transaction block and internally manages multiple transactions of its diff --git a/docs/invariants.md b/docs/invariants.md index b75308c..590538d 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -249,6 +249,20 @@ every desired statement's target against the absence proof the same way), `pkg/s (proof construction). *Source:* adversarial review of the optimistic front door. +### ST-8 โ€” A desired schema's statements carry execution order in the proof + +A `statement.DesiredSchema` orders its statements for execution at construction โ€” the +`CREATE TABLE` first, the indexes keeping their input order after it โ€” so every replay of +the file states the same order and the position mapping between a greenfield plan's +statements and the create path's step verdicts holds by construction, not by each replay +site re-deriving the rule. A set that does not lead with a `CREATE TABLE` means the proof +was forged or mutated, and every consumer refuses it fail-closed rather than reordering. +*Enforced:* `pkg/statement` (`ParseDesired` establishes the order), `pkg/diffplan` +(`qualifiedDesired` asserts it when rendering the greenfield plan), `pkg/executor` +(`admitCreateSteps` asserts it before anything runs); `pkg/schemadiff`'s scratch +materialization relies on it to run the `CREATE TABLE` before its indexes. +*Source:* adversarial review of the declarative front door. + ## Refusals and preflight (RF) Each refusal is a preflight **error with a stated reason** โ€” never a warning, never attempted. @@ -346,4 +360,5 @@ about **how we write and review the code**. | ST-1, ST-2, ST-3, ST-4 | 8 | kill/resume, cross-version refuse, orphan-slot reap, failover reconcile | | ST-6 | 1 onward, complete by 8 | preflight matrix | | ST-7 | 1 | target-mismatch refusal + single-statement-by-construction tests | +| ST-8 | 2 (declarative model) | parse-time ordering + forged-proof refusal tests at every replay site | | OC-1..OC-6 | shape APIs from 2; bind at 11 | engine-contract tests | diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 773191b..48e6403 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -143,14 +143,39 @@ package. Landing this is one of: ### Routing the create path's refusals +A greenfield desired file โ€” the table does not exist on the target โ€” has no single routing +class, and an adapter must not fold its outcomes into one arm. `migrate.RunDesired` resolves +it to one of four things: + +| Outcome | Routing class | +| --- | --- | +| Executed | The table and its indexes exist; a rerun converges to an empty plan | +| `create-collision` refusal | **Re-plan**: re-diff the live catalog โ€” something now owns the name; never blindly retry | +| `insufficient-privileges` refusal (`*preflight.PrivilegeError`, `Tier == TierCreateTable`) | **Operator provisioning action**: the role needs the exact `GRANT` the error carries โ€” not a desired-file fix, and not retryable until granted | +| Admission refusal (`unsupported-statement`) | **Author action**: the desired file states a shape the create path refuses; retrying unchanged cannot succeed | + +Only the last is an author error. An adapter that surfaces every greenfield refusal as +"fix your desired file" gives operators the wrong instruction for the two middle rows. A +create that failed mid-sequence follows the +[committed-prefix contract](execution-model.md#the-committed-prefix) โ€” the closing +paragraph of this section says what that means for the gate: it stays closed until the +live catalog is re-diffed; the failed run is never a no-op. + The greenfield `CREATE TABLE` path is a fixed call order, all inside the apply session: 1. `statement.ParseDesired` โ€” parse and validate the desired file (refuses `REFERENCES`, `CONCURRENTLY`, qualified names). -2. `preflight.CheckCreatePrivileges` โ€” mint the `CreationRole` proof for the target schema. -3. `preflight.CheckTableAbsent` โ€” mint the `AbsentTarget` proof for the table name. +2. `preflight.CheckTableAbsent` โ€” mint the `AbsentTarget` proof for the table name. +3. `preflight.CheckCreatePrivileges` โ€” mint the `CreationRole` proof for the target schema. 4. `executor.ExecuteCreate` โ€” consume both proofs and run the set. +`migrate.RunDesired` runs this sequence itself when the plan is greenfield โ€” the adapter +does not assemble it and must not mint either proof separately (a proof minted outside the +executing session proves nothing about it). The order decides which refusal wins when both +preflights would fail: absence is checked first, so an occupied name refuses as +`create-collision` even when the role also lacks `CREATE` โ€” the collision is the more +actionable message (the change is not a create at all) and absence is the cheaper check. + Both proofs share one rule the adapter must respect: they are **minted inside the apply session and consumed there** โ€” never serialized into `SchemaChange.Metadata`, carried across the plan/apply boundary, or reused across retries. Absence or privilege at plan time proves @@ -174,6 +199,7 @@ them, don't retry them uniformly: | --- | --- | --- | | `ErrDuplicateCreateName` (`duplicate-create-name`) | The desired set claims one relation name twice โ€” including a first-choice implicit constraint-index name; refused at admission, nothing ran | Fix the desired file; retrying unchanged cannot succeed | | `ErrPartitionOfUnsupported` (`partition-of-unsupported`) | `PARTITION OF` binds to a live parent the absence proof does not cover | Fix the desired file; out of the create path's scope | +| `ErrIfNotExistsUnsupported` (`if-not-exists-unsupported`) | `CREATE ... IF NOT EXISTS` succeeds as a name-only no-op over a relation it cannot vouch for โ€” the opposite of the absence proof's fail-closed contract; refused at admission, nothing ran | Fix the desired file: state the plain `CREATE`; the absence check owns collision handling | | `ErrUnsupportedCreateStep` (`unsupported-create-step`) | A desired statement is not a shape the create path can run | Fix the desired file | | `ErrCreateCollision` (`create-collision`) | A concurrent writer took a needed name after a valid proof | Re-diff the live catalog and re-plan โ€” the world changed; never blindly retry the create | diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index e2b2ac4..4739f84 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -161,9 +161,9 @@ func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plan.S func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.Change, error) { statements := ds.Statements() if len(statements) == 0 || statements[0].Kind() != statement.KindCreateTable { - // A DesiredSchema proof guarantees a CREATE TABLE ordered first; a - // set that does not lead with one means the proof was forged or - // mutated. + // INV: ST-8 โ€” a DesiredSchema proof guarantees a CREATE TABLE + // ordered first; a set that does not lead with one means the proof + // was forged or mutated. return nil, errors.New("desired schema does not lead with a CREATE TABLE") } changes := make([]schemadiff.Change, 0, len(statements)) diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 1fd324f..5e416e7 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -185,11 +185,11 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // index) claims nothing decidable and is exempt. func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([]statement.Statement, error) { desired := ds.Statements() - // INV: ST-7 โ€” a DesiredSchema proof guarantees a CREATE TABLE ordered + // INV: ST-8 โ€” a DesiredSchema proof guarantees a CREATE TABLE ordered // first; a set that does not lead with one means the proof was forged // or mutated. if len(desired) == 0 || desired[0].Kind() != statement.KindCreateTable { - return nil, fmt.Errorf("%w: ST-7: desired schema does not lead with a CREATE TABLE", ErrInvariantViolation) + return nil, fmt.Errorf("%w: ST-8: desired schema does not lead with a CREATE TABLE", ErrInvariantViolation) } steps := make([]statement.Statement, 0, len(desired)) claimed := make(map[string]struct{}, len(desired)) diff --git a/pkg/executor/optimistic.go b/pkg/executor/optimistic.go index 94cb5f3..fe5f56f 100644 --- a/pkg/executor/optimistic.go +++ b/pkg/executor/optimistic.go @@ -174,9 +174,13 @@ func (b Budget) validate() error { // grammar), and a target mismatch is refused before anything executes, so a // proof for one table cannot smuggle SQL against another. Each attempt is a // new transaction, so neither an aborted transaction nor its settings can -// leak through the pool. On success the change is committed: it was -// effectively instant. If the lock budget is exhausted across all bounded -// attempts, a *BudgetError carrying the attempt count is returned. +// leak through the pool. When the proof carries a schema, each attempt runs +// with search_path pinned to that schema then public, so the statement's +// unqualified secondary names resolve in the target schema โ€” the same +// resolution the create path and the introspection read path use. On +// success the change is committed: it was effectively instant. If the lock +// budget is exhausted across all bounded attempts, a *BudgetError carrying +// the attempt count is returned. // Statement timeouts and all other failures return immediately: repeating // work that exceeded its execution budget is not a lock-acquisition // strategy. @@ -209,8 +213,13 @@ func executeNative(ctx context.Context, pool *pgxpool.Pool, pt preflight.Preflig return fmt.Errorf("%w: ST-7: statement targets %q but preflight verified %q", ErrInvariantViolation, qualifiedName(st.Schema(), st.Table()), qualifiedName(pt.Schema(), pt.Table())) } + // The attempt runs with search_path pinned to the proof's schema (when + // the proof carries one โ€” an unqualified lookup carries none and runs + // under the session default), so the statement's unqualified secondary + // names โ€” a column's type, an expression's function โ€” resolve in the + // target schema whether the run creates the table or alters it. return executeWithLockRetryObserved(ctx, retry, func(ctx context.Context) error { - return executeNativeAttempt(ctx, pool, st, b) + return executeBoundedAttempt(ctx, pool, st, b, pt.Schema()) }, sleepContext, func(attempt int) { if tracker != nil { tracker.SetAttempt(attempt) @@ -218,10 +227,6 @@ func executeNative(ctx context.Context, pool *pgxpool.Pool, pt preflight.Preflig }) } -func executeNativeAttempt(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, b Budget) error { - return executeBoundedAttempt(ctx, pool, st, b, "") -} - // executeBoundedAttempt is the shared transactional attempt behind the // optimistic and create paths. When searchPathSchema is set, the // transaction's search_path is pinned to that schema then public โ€” the diff --git a/pkg/executor/optimistic_integration_test.go b/pkg/executor/optimistic_integration_test.go index 3f14efc..bb712d7 100644 --- a/pkg/executor/optimistic_integration_test.go +++ b/pkg/executor/optimistic_integration_test.go @@ -67,6 +67,46 @@ func TestExecuteNativeCommitsInstantChange(t *testing.T) { assert.Equal(t, "integer", columnType(t, pool, schema, "t", "age"), "the committed change must be visible") } +// Alter attempts run with search_path pinned to the proof's schema then +// public โ€” the same policy the create path and the introspection read path +// set โ€” so an ALTER's unqualified type reference resolves in the target +// schema, and resolves there even when public holds a type of the same +// name. Without the pin the attempt would run under the session default: +// the target schema's type would be invisible (SQLSTATE 42704) and a +// same-named type in public would bind silently instead. +func TestExecuteNativeResolvesTypesInTargetSchema(t *testing.T) { + pool, schema := newPool(t) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema)) + require.NoError(t, err) + pt := mustPreflight(t, pool, schema, "t") + + // The type lives in the target schema and, under the same name, in + // public too โ€” resolution must pick the target schema's copy. + typeName := schema + "_mood" + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TYPE %s.%s AS ENUM ('happy', 'sad')", schema, typeName)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TYPE public.%s AS ENUM ('decoy')", typeName)) + require.NoError(t, err) + t.Cleanup(func() { + _, err := pool.Exec(context.WithoutCancel(t.Context()), + fmt.Sprintf("DROP TYPE IF EXISTS public.%s", typeName)) + assert.NoError(t, err) + }) + + st := mustParse(t, fmt.Sprintf("ALTER TABLE %s.t ADD COLUMN m %s", schema, typeName)) + require.NoError(t, executor.ExecuteNative(t.Context(), pool, pt, st, budget, executor.DefaultRetryPolicy())) + + var udtSchema string + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT udt_schema FROM information_schema.columns + WHERE table_schema = $1 AND table_name = 't' AND column_name = 'm'`, + schema).Scan(&udtSchema)) + assert.Equal(t, schema, udtSchema, + "the column's type must resolve in the proof's schema, not public") +} + func TestExecuteNativeCancelsWhenLockBlocked(t *testing.T) { pool, schema := newPool(t) _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema)) diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index 47c4b53..96d6d5a 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -249,7 +249,10 @@ func runCreate(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, repo } if execErr == nil { result.Outcome = verdict.OutcomeExecuted - result.Detail = fmt.Sprintf("created: all %d planned statements committed", len(report.Statements)) + // The count comes from the committed steps โ€” the same source as + // the verdicts above โ€” so the disclosure cannot claim more than + // the executor reported committing. + result.Detail = fmt.Sprintf("created: all %d planned statements committed", len(rep.Steps)) return result, nil } var stepErr *executor.SequenceStepError