diff --git a/README.md b/README.md index 855421b..5dbe152 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,10 @@ refusal — never a silently wrong or incomplete result: - **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. +- **Greenfield create shapes** the create path cannot run — `PARTITION OF`, + `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, or a relation name the desired + set claims twice — refuse at plan time, and the same rules re-check at + apply. The codebase is partitioned into a small safety-critical core and a periphery — **[SAFETY.md](SAFETY.md)** says which packages are which and the diff --git a/docs/capabilities.md b/docs/capabilities.md index 706ecf7..238fd64 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -182,7 +182,7 @@ Status legend: ✅ T1 (supported today) · 🟡 T2 (planned; typed refusal today | Operation | Status | Engine path | Online-safety problem? | Behavior and why | | --- | --- | --- | --- | --- | -| `CREATE TABLE ... PARTITION OF` | 🟡 | native, planned flow | 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 | +| `CREATE TABLE ... PARTITION OF` | 🟡 | native, planned flow | Yes | Typed refusal at both doors: the imperative door does not take `CREATE TABLE`, and the declarative create path refuses the form at plan time and re-checks it at apply — 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` | ✅ | native, as-is | 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]` | ✅ | native, safer sequence | Yes | `CONCURRENTLY` is the idiom; the blocking form is rewritten to it | | Partitioned parents in the **declarative model** | 🟡 | native, planned flow | 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 | @@ -198,7 +198,7 @@ Status legend: ✅ T1 (supported today) · 🟡 T2 (planned; typed refusal today | Unlogged tables | 🟡 | native, planned flow | 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 | 🟡 | native, planned flow | 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 | 🟡 | native, planned flow | 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) | ✅ | native, as-is | 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: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every relation name the desired file states (explicit index names and first-choice constraint-index and column-sequence names) is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution — drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. `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 | +| Greenfield `CREATE TABLE` apply (the table does not exist yet — a fresh database or a new table in a live one) | ✅ | native, as-is | 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: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every relation name the desired file states (explicit index names and first-choice constraint-index and column-sequence names) is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution — drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names refuse at plan time and are re-checked at apply, 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 3ac4dc2..a19c7a6 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 greenfield create plan carries a shape the create path refuses (`PARTITION OF`, `IF NOT EXISTS`). | +| `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`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, or a duplicate claimed relation name). These greenfield shapes refuse in the plan and are re-checked at apply. | | `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)). | diff --git a/docs/invariants.md b/docs/invariants.md index 7880c56..021ba1f 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -259,7 +259,7 @@ site re-deriving the rule. A set that does not lead with a `CREATE TABLE` means 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 +(`checkCreateSteps` asserts it before anything is planned or run); `pkg/schemadiff`'s scratch materialization relies on it to run the `CREATE TABLE` before its indexes. *Source:* adversarial review of the declarative front door. diff --git a/docs/limitations.md b/docs/limitations.md index 2971310..6fc667f 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -32,7 +32,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 | 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. | +| 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`). Planning refuses every clause that binds to an existing object the absence proof does not cover (`PARTITION OF`, `INHERITS`, `LIKE`, and `OF`), plus `IF NOT EXISTS` and duplicate claimed relation names. Apply re-checks the same shape rules before anything executes. `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 @@ -44,7 +44,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 table name and every relation name the desired file states (explicit index names and first-choice constraint-index and column-sequence names) are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. Names the server invents rather than names the desired file states are outside this coverage. An occupied claimed name is a typed `create-collision` refusal before anything runs — drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. `PARTITION OF` and `IF NOT EXISTS` are also typed refusals before execution. | +| A desired file whose table does not exist yet | Converges — the greenfield create path verifies the table name and every relation name the desired file states (explicit index names and first-choice constraint-index and column-sequence names) are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. Names the server invents rather than names the desired file states are outside this coverage. An occupied claimed name is a typed `create-collision` refusal before anything runs — drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. Unsupported create shapes (`PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`) and duplicate claimed names refuse at plan time and are re-checked at apply. | | 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/docs/plan-report.md b/docs/plan-report.md index 24063b9..0f8f79f 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -62,7 +62,7 @@ consumer rendering either into a shared surface must clamp and escape them. | `route` | string | always | The planner's aggregate route for the statement (see Routes). | | `backend` | string | except refusals | The assigned execution strategy (see Backends); absent for refusals. | | `disposition` | string | always | What execution would do with this statement now (see Dispositions). | -| `reason` | string | refusals only | Typed refusal cause for this statement: `unsupported-statement` for a planner-level refusal, `unsupported-partitioned-parent` when target facts refuse it. An unknown value must be treated as refused. | +| `reason` | string | refusals only | Typed refusal cause for this statement: `unsupported-statement` for a planner-level refusal or, on a greenfield plan, a create shape the create path refuses (`PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, a duplicate claimed relation name); `unsupported-partitioned-parent` when target facts refuse it. An unknown value must be treated as refused. | | `decisions` | array | always | The planner's per-operation classifications (below). | | `exec_sql` | array | native route | The ordered SQL the native backend would run — the safer sequence when the planner constructed one, or the statement as written for a table that does not exist yet (the greenfield create path runs plain builds; see Fingerprint). Absent for non-native routes. | | `execution` | string | with `exec_sql` | The typed execution contract for `exec_sql` (see Execution contracts). A consumer that runs the statements itself branches on this — it is what says the steps must not be wrapped in a transaction block. Present exactly when `exec_sql` is. | @@ -122,11 +122,13 @@ treat the statement and report as refused. | Value | Meaning | |---|---| -| `unsupported-statement` | The planner knows no safe path for the statement (planner-level refusal). The same token the run path's refusal verdict carries, so a dry-run report and a run receipt for the same statement match on the typed field alone. | +| `unsupported-statement` | The planner knows no safe path for the statement (planner-level refusal), or — on a greenfield plan, where the table does not exist — the create path refuses the statement's shape: `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, or a relation name the desired set claims twice. The same token the run path's refusal verdict carries, so a dry-run report and a run receipt for the same statement match on the typed field alone. The report carries no per-statement cause; `migrate.RunDesired`'s refusal detail and the text diff name it. | | `unsupported-partitioned-parent` | Target facts show that the statement cannot run safely on a partitioned parent. | On the apply path, refusal checks have deterministic precedence: table size, then partition support, then privileges. +On the greenfield create path, a decidable shape refusal takes precedence over the table-absence +and privilege checks because it needs no connection. ### Backends (`backend`) diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 30650a0..9c51dcd 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -144,15 +144,19 @@ 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: +class, and an adapter must not fold its outcomes into one arm. The plan resolves +shape-decidable author errors before the apply window, and `migrate.RunDesired` re-checks +them when admitting execution. A greenfield run resolves 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, then fix the occupant**: the table name or a claimed index, constraint-index, or sequence name is occupied. Re-diff the live catalog to see what holds it; re-planning alone reproduces the refusal — drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. 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 | +| Plan/admission refusal (`unsupported-statement`) | **Author action**: the desired file states a shape the create path refuses; retrying unchanged cannot succeed | + +Greenfield refusals have deterministic precedence: a decidable shape refusal comes before the +table-absence and privilege checks because it needs no connection. 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 @@ -206,10 +210,10 @@ them, don't retry them uniformly: | Outcome | What it means | Orchestrator action | | --- | --- | --- | -| `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 | +| `ErrDuplicateCreateName` (`duplicate-create-name`) | The desired set claims one relation name twice — including a first-choice implicit constraint-index name; refused in the plan and re-checked at apply, 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; refused in the plan and re-checked at apply | 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 in the plan and re-checked at apply, 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; refused in the plan and re-checked at apply | Fix the desired file | | `ErrCreateCollision` (`create-collision`) | A claimed index, constraint-index, or sequence name was already occupied, or a concurrent writer took a needed name after the absence checks | Re-diff the live catalog to see what holds the name, then drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column — re-planning alone reproduces the refusal; never blindly retry the create | Before the first step, `ExecuteCreate` probes `pg_class` in one schema-scoped catalog diff --git a/internal/cli/color_test.go b/internal/cli/color_test.go index 0c1ed8c..578eb2b 100644 --- a/internal/cli/color_test.go +++ b/internal/cli/color_test.go @@ -133,8 +133,8 @@ func TestDiffTextColorWrapsLabelsOnly(t *testing.T) { report.TableExists = &exists var plain, colored strings.Builder - require.NoError(t, writeDiffText(&plain, palette{}, report)) - require.NoError(t, writeDiffText(&colored, palette{enabled: true}, report)) + require.NoError(t, writeDiffText(&plain, palette{}, report, nil)) + require.NoError(t, writeDiffText(&colored, palette{enabled: true}, report, nil)) assert.Contains(t, colored.String(), ansiWarning) assert.Contains(t, colored.String(), ansiBold) @@ -260,7 +260,7 @@ func TestMachineOutputsStayPlainUnderColorAlways(t *testing.T) { return cmd.runSuggest(strings.NewReader("CREATE INDEX t_c_idx ON t (c)"), out) }}, {"diff --json", func(out io.Writer) error { return writeJSON(out, diffReport) }}, - {"diff --sql", func(out io.Writer) error { return writePlanText(out, diffReport) }}, + {"diff --sql", func(out io.Writer) error { return writePlanText(out, diffReport, nil) }}, {"migrate --dry-run --json", func(out io.Writer) error { return writeJSON(out, dryRunReport) }}, } for _, tc := range cases { diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 9c1a235..955a4ec 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -10,6 +10,7 @@ import ( "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/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" @@ -47,14 +48,15 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { "schema", report.Schema, "table", report.Table, "changes", len(report.Statements), "table_exists", report.TableExists != nil && *report.TableExists, "disposition", string(report.Disposition)) + causes := greenfieldRefusalCauses(c.Schema, ds, report) switch { case c.JSON: err = writeJSON(out, report) case c.SQL: - err = writePlanText(out, report) + err = writePlanText(out, report, causes) default: - err = writeDiffText(out, c.palette(out), report) + err = writeDiffText(out, c.palette(out), report, causes) } if err != nil { return err @@ -68,6 +70,32 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { return nil } +// greenfieldRefusalCauses returns the create path's typed refusal for each +// statement of a refused greenfield plan, positional with +// report.Statements and nil for statements the create path admitted; nil +// for any other report. The plan report carries no field for the cause, +// and the shape check is pure over the desired schema the plan was derived +// from, so it is recomputed here rather than stored. The plan was just +// derived from the same desired schema, so the check cannot fail or +// disagree on statement count; if it does, the statements render without +// a cause rather than turning a display detail into an error. +func greenfieldRefusalCauses(schema string, ds statement.DesiredSchema, report plan.Report) []error { + if !tableMissing(report) { + return nil + } + if report.Disposition != router.DispositionRefuse { + return nil + } + causes, err := executor.CreateShapeRefusals(schema, ds) + if err != nil { + return nil + } + if len(causes) != len(report.Statements) { + return nil + } + return causes +} + // writeJSON emits the plan report as JSON. func writeJSON(out io.Writer, report plan.Report) error { if report.Statements == nil { @@ -90,7 +118,7 @@ func writeJSON(out io.Writer, report plan.Report) error { // plan (a CONCURRENTLY rewrite could not run inside a transaction block). // The header points at migrate as the executing front door: running this // script directly bypasses the gate that refuses blocking statements. -func writePlanText(out io.Writer, report plan.Report) error { +func writePlanText(out io.Writer, report plan.Report, refusalCauses []error) error { if len(report.Statements) == 0 { if _, err := fmt.Fprintln(out, "-- no changes: live table matches the desired schema"); err != nil { return fmt.Errorf("write plan: %w", err) @@ -109,19 +137,37 @@ func writePlanText(out io.Writer, report plan.Report) error { return fmt.Errorf("write plan: %w", err) } } - for _, ps := range report.Statements { - if err := writeChangeText(out, ps); err != nil { + for i, ps := range report.Statements { + var cause error + if i < len(refusalCauses) { + cause = refusalCauses[i] + } + if err := writeChangeText(out, ps, cause); err != nil { return err } } return nil } -// writeChangeText emits one annotated statement of the text plan. -func writeChangeText(out io.Writer, ps plan.Statement) error { +// writeChangeText emits one annotated statement of the text plan. A +// refused statement is emitted as an SQL comment: the script is +// copy-pasteable, and it must never carry a statement the engine refuses +// where a reader could run it by accident. +func writeChangeText(out io.Writer, ps plan.Statement, refusalCause error) error { if _, err := fmt.Fprintf(out, "-- %s\n", annotate(ps)); err != nil { return fmt.Errorf("write plan: %w", err) } + if ps.Disposition == router.DispositionRefuse { + if refusalCause != nil { + if _, err := fmt.Fprintf(out, "-- the create path refuses this statement: %v\n", refusalCause); err != nil { + return fmt.Errorf("write plan: %w", err) + } + } + if _, err := fmt.Fprintf(out, "-- %s;\n", strings.ReplaceAll(ps.SQL, "\n", "\n-- ")); err != nil { + return fmt.Errorf("write plan: %w", err) + } + return nil + } if len(ps.ExecSQL) > 0 && ps.ExecSQL[0] != ps.SQL { if _, err := fmt.Fprintf(out, "-- safer form the engine would run (not equivalent; each step in its own transaction — see %s):\n", onlineDDLReferenceURL); err != nil { @@ -162,6 +208,8 @@ func annotate(ps plan.Statement) string { s += ": needs the " + string(ps.Backend) + " backend, which is not implemented yet" case router.DispositionRewriteRequired: s += ": blocks as submitted and no online rewrite was constructed — the engine will not run it" + case router.DispositionRefuse: + s += ": refused — the engine will not run it" } return s } diff --git a/internal/cli/diff_integration_test.go b/internal/cli/diff_integration_test.go index ff1f94f..cb96a41 100644 --- a/internal/cli/diff_integration_test.go +++ b/internal/cli/diff_integration_test.go @@ -223,6 +223,43 @@ func TestDiffMissingTableEmitsFullDesiredSchema(t *testing.T) { }, sqls) } +// A greenfield desired file the create path refuses by shape exits with the +// refusal code in every rendering, the JSON report carries the typed +// refusal, and the text report names the create path's own cause — the +// same explanation the executor would give — so the author can tell a +// PARTITION OF refusal apart from any other unsupported shape. +func TestDiffGreenfieldCreateShapeRefusal(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) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.parent (id int, region text) PARTITION BY LIST (region)", schema)) + require.NoError(t, err) + desired := "CREATE TABLE child PARTITION OF parent FOR VALUES IN ('eu');" + + cmd := newDiffCmd(t, url, schema, desired) + cmd.JSON = true + var out strings.Builder + require.ErrorIs(t, cmd.run(t.Context(), &out), verdict.ErrRefused) + var report plan.Report + require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) + require.NotNil(t, report.TableExists) + assert.False(t, *report.TableExists) + assert.Equal(t, router.DispositionRefuse, report.Disposition) + require.Len(t, report.Statements, 1) + assert.Equal(t, router.DispositionRefuse, report.Statements[0].Disposition) + assert.Equal(t, verdict.ReasonUnsupportedStatement, report.Statements[0].Reason) + assert.Empty(t, report.Statements[0].ExecSQL) + + text := newDiffCmd(t, url, schema, desired) + var textOut strings.Builder + require.ErrorIs(t, text.run(t.Context(), &textOut), verdict.ErrRefused) + assert.Contains(t, textOut.String(), "the create path refuses this statement: CREATE TABLE PARTITION OF") + assert.NotContains(t, textOut.String(), "the plan creates it") +} + func TestDiffTextPlanIsExecutableSQL(t *testing.T) { url := testutil.StartPostgres(t) pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) diff --git a/internal/cli/diff_text.go b/internal/cli/diff_text.go index fcaf9cc..2820bd5 100644 --- a/internal/cli/diff_text.go +++ b/internal/cli/diff_text.go @@ -16,11 +16,23 @@ import ( // framing differs from the dry-run report where the semantics differ: diff // never executes, so execution is routed through the migrate front door, // and a missing table is the greenfield case — the plan creates the table -// from the full desired schema — not an error. -func writeDiffText(out io.Writer, pal palette, report plan.Report) error { +// from the full desired schema — not an error, unless the create path +// refuses a statement's shape, in which case the leading note says so +// instead of promising a create the refusal beneath it withdraws. causes +// is positional with report.Statements: the create path's typed refusal +// for a greenfield statement it refused by shape, nil elsewhere. The plan +// report has no field for it, so the caller recomputes it +// (greenfieldRefusalCauses) and the renderer prints it as a trailing note +// on the refused statement — the executor's explanation, which the typed +// reason alone does not carry. +func writeDiffText(out io.Writer, pal palette, report plan.Report, causes []error) error { w := &stickyWriter{out: out, pal: pal} if tableMissing(report) { - w.diag("note", "", fmt.Sprintf("the table %s.%s does not exist — the plan creates it from the full desired schema", report.Schema, report.Table)) + if report.Disposition == router.DispositionExecute { + w.diag("note", "", fmt.Sprintf("the table %s.%s does not exist — the plan creates it from the full desired schema", report.Schema, report.Table)) + } else { + w.diag("note", "", fmt.Sprintf("the table %s.%s does not exist — the plan is the full desired schema, and a statement in it is refused, so nothing would be created", report.Schema, report.Table)) + } } if len(report.Statements) == 0 { w.entry("plan:") @@ -32,6 +44,9 @@ func writeDiffText(out io.Writer, pal palette, report plan.Report) error { s, r := writeStatementDiagnostics(w, i+1, ps, "pg-sprite migrate") steps += s refused += r + if i < len(causes) && causes[i] != nil { + w.diag("note", "", "the create path refuses this statement: "+causes[i].Error()) + } } w.entry("plan:") w.printf(" %s — %s, %s to run, %d refused\n", targetText(report), diff --git a/internal/cli/diff_text_test.go b/internal/cli/diff_text_test.go index 9442cb7..96ea943 100644 --- a/internal/cli/diff_text_test.go +++ b/internal/cli/diff_text_test.go @@ -7,10 +7,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/verdict" ) // The text rendering is this renderer's own unit test: the layout below is @@ -42,7 +44,7 @@ func TestDiffTextSaferIdiomSubstitution(t *testing.T) { }) var out strings.Builder - require.NoError(t, writeDiffText(&out, palette{}, report)) + require.NoError(t, writeDiffText(&out, palette{}, report, nil)) assert.Equal(t, `statement 1: ALTER TABLE "users" ADD CONSTRAINT "u" UNIQUE ("email"); @@ -96,7 +98,7 @@ func TestDiffTextGreenfieldLeadsWithNote(t *testing.T) { }) var out strings.Builder - require.NoError(t, writeDiffText(&out, palette{}, report)) + require.NoError(t, writeDiffText(&out, palette{}, report, nil)) text := out.String() assert.True(t, strings.HasPrefix(text, "note:\n the table public.widgets does not exist — the plan creates it from the\n full desired schema\n"), "the greenfield note must lead the report: %s", text) @@ -109,6 +111,114 @@ func TestDiffTextGreenfieldLeadsWithNote(t *testing.T) { assert.NotContains(t, text, "error[table-not-found]") } +// A greenfield plan the create path refuses by shape must not promise the +// create the refusal beneath it withdraws: the leading note says the plan +// is refused, the refused statement carries the create path's own cause as +// a trailing note, and the report counts it refused. +func TestDiffTextGreenfieldRefusedNoteAndCause(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Schema, report.Table, report.ServerVersion = "public", "child", "16.10" + report.Disposition = router.DispositionRefuse + missing := false + report.TableExists = &missing + sql := `CREATE TABLE "public"."child" PARTITION OF "public"."parent" FOR VALUES IN (1)` + report.Statements = append(report.Statements, plan.Statement{ + SQL: sql, + Route: planner.RouteNative, + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedStatement, + }) + causes := []error{executor.ErrPartitionOfUnsupported} + + var out strings.Builder + require.NoError(t, writeDiffText(&out, palette{}, report, causes)) + text := out.String() + assert.True(t, strings.HasPrefix(text, "note:\n the table public.child does not exist — the plan is the full desired\n schema, and a statement in it is refused, so nothing would be created\n"), + "the greenfield note must say the plan is refused: %s", text) + assert.NotContains(t, text, "the plan creates it") + assert.Contains(t, text, "error[unsupported-statement]:\n refused — ") + assert.Contains(t, text, "note:\n the create path refuses this statement: CREATE TABLE PARTITION OF is not\n supported by the create path") + assert.Contains(t, text, "1 statement, 0 steps to run, 1 refused\n") + assert.NotContains(t, text, "apply:") + assert.True(t, diffRefused(report)) +} + +// Without a cause list the refused greenfield statement renders its typed +// refusal alone — the renderer never invents an explanation. +func TestDiffTextGreenfieldRefusedWithoutCauses(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Schema, report.Table, report.ServerVersion = "public", "child", "16.10" + report.Disposition = router.DispositionRefuse + missing := false + report.TableExists = &missing + report.Statements = append(report.Statements, plan.Statement{ + SQL: `CREATE TABLE "public"."child" ("id" int)`, + Route: planner.RouteNative, + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedStatement, + }) + + var out strings.Builder + require.NoError(t, writeDiffText(&out, palette{}, report, nil)) + assert.NotContains(t, out.String(), "the create path refuses this statement") + assert.Contains(t, out.String(), "error[unsupported-statement]:\n refused — ") +} + +// The --sql script is copy-pasteable, so a refused statement is annotated +// as refused and emitted as a comment — never as a bare statement a reader +// could run past the gate the engine enforces. +func TestPlanTextCommentsOutRefusedStatement(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Schema, report.Table, report.ServerVersion = "public", "child", "16.10" + report.Disposition = router.DispositionRefuse + missing := false + report.TableExists = &missing + refusedSQL := `CREATE TABLE "public"."child" PARTITION OF "public"."parent" FOR VALUES IN ('a +b')` + indexSQL := `CREATE INDEX "child_id_idx" ON "public"."child" ("id")` + report.Statements = append(report.Statements, + plan.Statement{ + SQL: refusedSQL, + Route: planner.RouteNative, + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedStatement, + Decisions: []planner.Decision{{ + Operation: "create table", + Route: planner.RouteNative, + Reason: planner.ReasonMetadataOnly, + }}, + }, + plan.Statement{ + SQL: indexSQL, + Route: planner.RouteNative, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + ExecSQL: []string{indexSQL}, + Execution: planner.ExecutionAutocommit, + Decisions: []planner.Decision{{ + Operation: "create index", + Route: planner.RouteNative, + Reason: planner.ReasonMetadataOnly, + }}, + }, + ) + causes := []error{executor.ErrPartitionOfUnsupported, nil} + + var out strings.Builder + require.NoError(t, writePlanText(&out, report, causes)) + text := out.String() + assert.Contains(t, text, "-- native (metadata-only): refused — the engine will not run it\n"+ + "-- the create path refuses this statement: "+executor.ErrPartitionOfUnsupported.Error()+"\n"+ + "-- "+strings.ReplaceAll(refusedSQL, "\n", "\n-- ")+";\n") + assert.Contains(t, text, "-- native (metadata-only)\n"+indexSQL+";\n") + for line := range strings.SplitSeq(strings.TrimSpace(text), "\n") { + if strings.HasPrefix(line, "--") { + continue + } + assert.Equal(t, indexSQL+";", line, "the only bare statement is the executable one") + } +} + // A greenfield index build renders as the metadata-only note the planner // already gives the CREATE TABLE — never as the safer-idiom warning that // would tell the reader a table nobody reads yet is about to be locked. @@ -135,7 +245,7 @@ func TestDiffTextGreenfieldIndexRendersMetadataOnly(t *testing.T) { }) var out strings.Builder - require.NoError(t, writeDiffText(&out, palette{}, report)) + require.NoError(t, writeDiffText(&out, palette{}, report, nil)) text := out.String() assert.Contains(t, text, "statement 1:\n "+sql+";\n") assert.Contains(t, text, "note[metadata-only]:\n create index — a brief catalog-only change") @@ -156,7 +266,7 @@ func TestDiffTextNoChanges(t *testing.T) { report.TableExists = &exists var out strings.Builder - require.NoError(t, writeDiffText(&out, palette{}, report)) + require.NoError(t, writeDiffText(&out, palette{}, report, nil)) assert.Equal(t, `plan: public.users (PostgreSQL 16.10) — no changes; the live table matches the desired schema `, out.String()) @@ -184,7 +294,7 @@ func TestDiffTextRefusedStatementDropsApply(t *testing.T) { }) var out strings.Builder - require.NoError(t, writeDiffText(&out, palette{}, report)) + require.NoError(t, writeDiffText(&out, palette{}, report, nil)) text := out.String() assert.Contains(t, text, "error[backend-unavailable]:\n refused — needs the copy-and-swap backend") assert.Contains(t, text, "1 statement, 0 steps to run, 1 refused\n") diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index eca60ec..f13c530 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -23,6 +23,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/preflight" @@ -100,6 +101,15 @@ func Plan(ctx context.Context, pool *pgxpool.Pool, req Request) (plan.Report, er return plan.Report{}, err } if !tableExists { + // Shape refusals are stamped first so the disclosure below only + // describes the statements that remain executable. + refused, refusalErr := executor.CreateShapeRefusals(req.Schema, ds) + if refusalErr != nil { + return plan.Report{}, refusalErr + } + if err := plan.RefuseUnsupportedCreateShape(&report, refused); err != nil { + return plan.Report{}, err + } plan.DiscloseGreenfieldExecution(&report) } else { targetFacts, checkErr := preflight.LookupTargetFacts(ctx, pool, req.Schema, ds.Table()) diff --git a/pkg/diffplan/diffplan_integration_test.go b/pkg/diffplan/diffplan_integration_test.go index bdec9a9..03cff4c 100644 --- a/pkg/diffplan/diffplan_integration_test.go +++ b/pkg/diffplan/diffplan_integration_test.go @@ -2,6 +2,7 @@ package diffplan_test import ( "fmt" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -232,6 +233,42 @@ func TestPlanMissingTableEmitsFullDesiredSchema(t *testing.T) { assertGreenfieldIndexExecution(t, report.Statements[1]) } +func TestPlanMissingTableRefusesCreateShapes(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) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.parent (id int) PARTITION BY RANGE (id)", schema)) + require.NoError(t, err) + + tests := []struct { + name string + sql string + refused []int + }{ + {name: "partition of", sql: "CREATE TABLE child PARTITION OF parent FOR VALUES FROM (1) TO (2)", refused: []int{0}}, + {name: "if not exists", sql: "CREATE TABLE IF NOT EXISTS t_if (id int)", refused: []int{0}}, + {name: "duplicate implicit index", sql: "CREATE TABLE t_dup (id int PRIMARY KEY); CREATE INDEX t_dup_pkey ON t_dup (id)", refused: []int{1}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + report, err := diffplan.Plan(t.Context(), pool, diffplan.Request{Schema: schema, Desired: parseDesired(t, tc.sql)}) + require.NoError(t, err) + assert.Equal(t, router.DispositionRefuse, report.Disposition) + assert.Equal(t, verdict.ReasonUnsupportedStatement, report.Reason) + for i, st := range report.Statements { + want := router.DispositionExecute + if slices.Contains(tc.refused, i) { + want = router.DispositionRefuse + assert.Equal(t, verdict.ReasonUnsupportedStatement, st.Reason) + } + assert.Equal(t, want, st.Disposition, "statement %d", i+1) + } + }) + } +} + // 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 diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 695d1ac..0649d46 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -213,40 +213,91 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // because a deterministic name the file states beats one the server // invents. A step whose name the server invents outright (an unnamed // index) claims nothing decidable and is exempt. +// The admitted steps are returned with every name they claim, so the caller +// can prove the set free in the catalog before the first step executes; +// duplicates within the set are already refused here, so the names are +// distinct. Order does not matter: the catalog probe is set membership and +// picks the reported occupant itself. func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([]statement.Statement, []string, error) { + checked, err := checkCreateSteps(at.Schema(), ds) + if err != nil { + return nil, nil, err + } + steps := make([]statement.Statement, 0, len(checked)) + names := make([]string, 0, len(checked)) + for i, step := range checked { + if step.refusal != nil { + return nil, nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(checked), step.refusal) + } + steps = append(steps, step.statement) + names = append(names, step.claims...) + } + return steps, names, nil +} + +// CreateShapeRefusals checks the connection-free create-path rules in desired +// statement order. A nil entry is admitted; a non-nil entry identifies the +// shape refusal for that statement. Parse failures and violated DesiredSchema +// invariants are returned separately because no positional plan is safe. +func CreateShapeRefusals(schema string, ds statement.DesiredSchema) ([]error, error) { + checked, err := checkCreateSteps(schema, ds) + if err != nil { + return nil, err + } + refusals := make([]error, len(checked)) + for i, step := range checked { + refusals[i] = step.refusal + } + return refusals, nil +} + +// createStep is one desired statement after shape checking: the qualified, +// re-parsed statement, the pg_class names it will claim, and the shape +// refusal that keeps it from running, nil when admitted. +type createStep struct { + statement statement.Statement + claims []string + refusal error +} + +// checkCreateSteps shape-checks every desired statement in order and marks +// the second claimant of any name with ErrDuplicateCreateName. A step's +// claims register whether or not its shape is refused, so a later statement +// that collides with a refused one is reported as the collision it is rather +// than admitted; a shape refusal already on the step is kept as its cause. +// A returned error means no positional result is safe — a parse failure or +// a violated DesiredSchema invariant. +func checkCreateSteps(schema string, ds statement.DesiredSchema) ([]createStep, error) { desired := ds.Statements() // 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, nil, fmt.Errorf("%w: ST-8: 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)) + steps := make([]createStep, 0, len(desired)) claimed := make(map[string]struct{}, len(desired)) for i, raw := range desired { - st, names, err := admitCreateStep(at, raw.SQL()) + step, err := checkCreateStepShape(schema, ds.Table(), raw.SQL()) if err != nil { - return nil, nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) + return nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) } - for _, name := range names { + for _, name := range step.claims { if _, taken := claimed[name]; taken { - return nil, nil, fmt.Errorf("desired statement %d of %d: %w: %q", i+1, len(desired), ErrDuplicateCreateName, name) + if step.refusal == nil { + step.refusal = fmt.Errorf("%w: %q", ErrDuplicateCreateName, name) + } + continue } claimed[name] = struct{}{} } - steps = append(steps, st) - } - // Order does not matter: the catalog probe is set membership and picks - // the reported occupant itself. - names := make([]string, 0, len(claimed)) - for name := range claimed { - names = append(names, name) + steps = append(steps, step) } - return steps, names, nil + return steps, nil } -// admitCreateStep qualifies one desired statement into the proof's schema, -// re-parses it by the real grammar, and admits it by shape and target. It +// checkCreateStepShape qualifies one desired statement into the target schema, +// re-parses it by the real grammar, and checks it by shape and target. It // returns the pg_class names the step will claim — for a CREATE TABLE the // table name plus the first-choice relation names of its constraints and // column-owned sequences, for a CREATE INDEX its explicit name, nothing when the @@ -255,70 +306,107 @@ func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([] // statement.Qualify rewrites only the target, so the secondary name would // resolve via search_path to an existing object the absence proof says // nothing about. -func admitCreateStep(at preflight.AbsentTarget, sql string) (statement.Statement, []string, error) { - qualified, err := statement.Qualify(sql, at.Schema()) +func checkCreateStepShape(schema, table, sql string) (createStep, error) { + qualified, err := statement.Qualify(sql, schema) if err != nil { - return statement.Statement{}, nil, err + return createStep{}, err } st, err := statement.ParseOne(qualified) if err != nil { - return statement.Statement{}, nil, err + return createStep{}, err } ops, err := statement.ParseOps(qualified) if err != nil { - return statement.Statement{}, nil, err + return createStep{}, err + } + // INV: ST-7 — the executor runs exactly the statement that was + // admitted, and only against the desired schema's own table; on the + // execution path admitCreateSteps hands in the schema the absence proof + // verified, and executeCreate has already matched the proof's table to + // the desired schema's. + if st.Table() == "" || st.Schema() != schema || st.Table() != table { + return createStep{}, fmt.Errorf("%w: ST-7: statement targets %q but desired schema is for %q", + ErrInvariantViolation, qualifiedName(st.Schema(), st.Table()), qualifiedName(schema, table)) } + step := createStep{statement: st} if len(ops) != 1 { // ParseOne admitted a single statement, so a differing op count // means the two parse boundaries disagree about the same SQL. - return statement.Statement{}, nil, fmt.Errorf("%w: statement carries %d operations", ErrUnsupportedCreateStep, len(ops)) + step.refusal = fmt.Errorf("%w: statement carries %d operations", ErrUnsupportedCreateStep, len(ops)) + return step, nil } op := ops[0] - var claims []string switch st.Kind() { case statement.KindCreateTable: - if op.PartitionOf { - return statement.Statement{}, nil, ErrPartitionOfUnsupported - } - if op.Inherits { - return statement.Statement{}, nil, fmt.Errorf("%w: INHERITS binds to an existing parent the absence proof does not cover", ErrUnsupportedCreateStep) - } - if op.Like { - return statement.Statement{}, nil, fmt.Errorf("%w: LIKE reads an existing source table the absence proof does not cover", ErrUnsupportedCreateStep) - } - if op.OfType { - return statement.Statement{}, nil, fmt.Errorf("%w: OF binds to an existing composite type the absence proof does not cover", ErrUnsupportedCreateStep) - } - if op.IfNotExists { - return statement.Statement{}, nil, ErrIfNotExistsUnsupported - } - implicit, err := statement.ImplicitRelationNames(qualified) - if err != nil { - // ParseOne already admitted this SQL as a CREATE TABLE, so a - // refusal here means the two parse boundaries disagree. - return statement.Statement{}, nil, fmt.Errorf("%w: %w", ErrUnsupportedCreateStep, err) - } - claims = append([]string{st.Table()}, implicit...) + step.claims, step.refusal = checkCreateTableShape(qualified, st.Table(), op) case statement.KindCreateIndex: - if op.Concurrent { - return statement.Statement{}, nil, fmt.Errorf("%w: a concurrent build is refused on a table born this run", ErrUnsupportedCreateStep) - } - if op.IfNotExists { - return statement.Statement{}, nil, ErrIfNotExistsUnsupported - } - if op.Name != "" { - claims = []string{op.Name} - } + step.claims, step.refusal = checkCreateIndexShape(op) default: - return statement.Statement{}, nil, fmt.Errorf("%w: kind %q", ErrUnsupportedCreateStep, st.Kind()) + step.refusal = fmt.Errorf("%w: kind %q", ErrUnsupportedCreateStep, st.Kind()) } - // INV: ST-7 — the executor runs exactly the statement that was - // admitted, and only against the target the absence proof verified. - if st.Table() == "" || st.Schema() != at.Schema() || st.Table() != at.Table() { - return statement.Statement{}, nil, fmt.Errorf("%w: ST-7: statement targets %q but absence was verified for %q", - ErrInvariantViolation, qualifiedName(st.Schema(), st.Table()), qualifiedName(at.Schema(), at.Table())) + return step, nil +} + +// checkCreateTableShape refuses the CREATE TABLE clauses that bind to a +// secondary relation or type and returns the names the table will claim: +// its own plus the first-choice relation names of its constraints and +// column-owned sequences. +// The claims are returned with the refusal so a later statement colliding +// with a refused table is still reported. +func checkCreateTableShape(qualified, table string, op statement.Op) ([]string, error) { + implicit, err := statement.ImplicitRelationNames(qualified) + if err != nil { + // ParseOne already admitted this SQL as a CREATE TABLE, so a + // refusal here means the two parse boundaries disagree. + return nil, fmt.Errorf("%w: %w", ErrUnsupportedCreateStep, err) + } + return append([]string{table}, implicit...), createTableShapeRefusal(op) +} + +// createTableShapeRefusal names the CREATE TABLE clause that keeps the +// statement off the create path, nil when the shape is admitted. +func createTableShapeRefusal(op statement.Op) error { + if op.PartitionOf { + return ErrPartitionOfUnsupported + } + if op.Inherits { + return fmt.Errorf("%w: INHERITS binds to an existing parent the absence proof does not cover", ErrUnsupportedCreateStep) + } + if op.Like { + return fmt.Errorf("%w: LIKE reads an existing source table the absence proof does not cover", ErrUnsupportedCreateStep) + } + if op.OfType { + return fmt.Errorf("%w: OF binds to an existing composite type the absence proof does not cover", ErrUnsupportedCreateStep) + } + if op.IfNotExists { + return ErrIfNotExistsUnsupported + } + return nil +} + +// checkCreateIndexShape refuses index builds that cannot run against a +// table born this run and returns the explicit index name as the step's +// claim; an unnamed index claims nothing decidable. The claim is returned +// with the refusal so a later statement colliding with a refused index is +// still reported. +func checkCreateIndexShape(op statement.Op) ([]string, error) { + var claims []string + if op.Name != "" { + claims = []string{op.Name} + } + return claims, createIndexShapeRefusal(op) +} + +// createIndexShapeRefusal names the CREATE INDEX clause that keeps the +// statement off the create path, nil when the shape is admitted. +func createIndexShapeRefusal(op statement.Op) error { + if op.Concurrent { + return fmt.Errorf("%w: a concurrent build is refused on a table born this run", ErrUnsupportedCreateStep) + } + if op.IfNotExists { + return ErrIfNotExistsUnsupported } - return st, claims, nil + return nil } // asCreateCollision maps the duplicate-name SQLSTATEs — 42P07 when a diff --git a/pkg/executor/create_test.go b/pkg/executor/create_test.go index d126264..e298327 100644 --- a/pkg/executor/create_test.go +++ b/pkg/executor/create_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/block/pg-sprite/pkg/executor" @@ -11,6 +12,43 @@ import ( "github.com/block/pg-sprite/pkg/statement" ) +func TestCreateShapeRefusals(t *testing.T) { + tests := []struct { + name string + sql string + want []error + }{ + {name: "partition", sql: "CREATE TABLE t PARTITION OF parent FOR VALUES FROM (1) TO (2)", want: []error{executor.ErrPartitionOfUnsupported}}, + {name: "if not exists", sql: "CREATE TABLE IF NOT EXISTS t (id int)", want: []error{executor.ErrIfNotExistsUnsupported}}, + {name: "existing object clause", sql: "CREATE TABLE t (LIKE source)", want: []error{executor.ErrUnsupportedCreateStep}}, + {name: "later duplicate", sql: "CREATE TABLE t (id int PRIMARY KEY); CREATE INDEX t_pkey ON t (id)", want: []error{nil, executor.ErrDuplicateCreateName}}, + // A refused step still registers its claims: the collision with the + // refused table's name is reported on the later statement instead of + // admitting it. + {name: "duplicate of refused step", sql: "CREATE TABLE IF NOT EXISTS t (id int); CREATE INDEX t ON t (id)", want: []error{executor.ErrIfNotExistsUnsupported, executor.ErrDuplicateCreateName}}, + // A step both refused by shape and colliding keeps the shape refusal + // as its cause. + {name: "refused shape wins over duplicate", sql: "CREATE TABLE t (id int PRIMARY KEY); CREATE INDEX IF NOT EXISTS t_pkey ON t (id)", want: []error{nil, executor.ErrIfNotExistsUnsupported}}, + {name: "admitted", sql: "CREATE TABLE t (id int); CREATE INDEX t_id ON t (id)", want: []error{nil, nil}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ds, err := statement.ParseDesired(tc.sql) + require.NoError(t, err) + refusals, err := executor.CreateShapeRefusals("app", ds) + require.NoError(t, err) + require.Len(t, refusals, len(tc.want)) + for i := range tc.want { + if tc.want[i] == nil { + assert.NoError(t, refusals[i]) + continue + } + assert.ErrorIs(t, refusals[i], tc.want[i], "statement %d", i+1) + } + }) + } +} + // createBudget is generous for unit tests; admission refusals return // before any database access. var createBudget = executor.Budget{LockTimeout: time.Second, StatementTimeout: 2 * time.Second} diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index 3f7ba70..8a00b64 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -362,7 +362,7 @@ func admitPlan(req DesiredRequest, report plan.Report) (DesiredResult, bool) { return refused, false } if report.Disposition != router.DispositionExecute { - refused.Reason, refused.Detail = planRefusal(report) + refused.Reason, refused.Detail = planRefusal(req, report) return refused, false } return DesiredResult{}, true @@ -387,8 +387,9 @@ func skippedRestDetail(n int) string { // planRefusal maps the first non-executable planned statement to the typed // refusal the aggregate result carries, mirroring how [Run] refuses the -// same dispositions at execution time. -func planRefusal(report plan.Report) (verdict.Reason, string) { +// same dispositions at execution time. A greenfield statement the create +// path refuses by shape carries the create path's own cause in the detail. +func planRefusal(req DesiredRequest, report plan.Report) (verdict.Reason, string) { for i, ps := range report.Statements { detail := func(why string) string { return fmt.Sprintf("planned statement %d (%s) %s; nothing was executed", i+1, ps.SQL, why) @@ -405,6 +406,9 @@ func planRefusal(report plan.Report) (verdict.Reason, string) { if reason == verdict.ReasonNone { reason = verdict.ReasonUnsupportedStatement } + if cause := createShapeCause(req, report, i); cause != nil { + return reason, detail(fmt.Sprintf("is refused by the create path: %v", cause)) + } return reason, detail("has no safe path") default: return verdict.ReasonUnsupportedStatement, detail("carries a disposition this build does not know") @@ -417,6 +421,33 @@ func planRefusal(report plan.Report) (verdict.Reason, string) { report.Disposition) } +// createShapeCause returns the create path's typed refusal for planned +// statement i of a greenfield plan, nil when the statement is not one the +// create path refused by shape. The plan report carries no field for the +// cause, and the shape check is pure over the desired schema the plan was +// derived from, so it is recomputed here rather than stored. A desired +// schema the planner just derived a plan from cannot fail the same check +// it already passed; if it does, the statement is reported without a cause +// rather than turning a refusal into an error. +func createShapeCause(req DesiredRequest, report plan.Report, i int) error { + if report.TableExists == nil || *report.TableExists { + return nil + } + if report.Statements[i].Reason != verdict.ReasonUnsupportedStatement { + return nil + } + refused, err := executor.CreateShapeRefusals(req.Schema, req.Desired) + if err != nil { + return nil + } + if len(refused) != len(report.Statements) { + // The plan and the desired schema disagree on statement count; + // no positional cause is trustworthy. + return nil + } + return refused[i] +} + // committedPrefixDetail states how far convergence got when execution // stopped at statement i (0-based) of n: the statements before it are // committed and stay committed. A stop on the first statement says plainly diff --git a/pkg/migrate/desired_integration_test.go b/pkg/migrate/desired_integration_test.go index 0842f7f..286b3e5 100644 --- a/pkg/migrate/desired_integration_test.go +++ b/pkg/migrate/desired_integration_test.go @@ -14,6 +14,7 @@ import ( "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/router" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" ) @@ -198,7 +199,10 @@ CREATE INDEX t_v_idx ON t (v);` 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.Equal(t, router.DispositionRefuse, res.Plan.Disposition, + "the refusal is decided on the plan, before the create path is entered") + assert.Contains(t, res.Detail, "is refused by the create path: "+executor.ErrPartitionOfUnsupported.Error(), + "the detail carries the create path's typed cause, not the echoed SQL alone") assert.Empty(t, res.Verdicts, "nothing was attempted") var exists bool @@ -208,6 +212,30 @@ CREATE INDEX t_v_idx ON t (v);` assert.False(t, exists, "the refused plan must not create the partition") }) + t.Run("refuses a desired set that claims one name twice before anything runs", func(t *testing.T) { + // The plan SQL says nothing about why statement 2 is refused — the + // cause is the collision with the implicit primary-key index name, + // and the detail must name it. + schema := testutil.NewSchema(t, pool) + + res, err := migrate.RunDesired(t.Context(), pool, migrate.DesiredRequest{ + Schema: schema, + Desired: parseDesired(t, "CREATE TABLE t (id int PRIMARY KEY); CREATE INDEX t_pkey ON t (id)"), + }, 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, "planned statement 2 (") + assert.Contains(t, res.Detail, executor.ErrDuplicateCreateName.Error()+`: "t_pkey"`) + 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 table") + }) + t.Run("refuses an occupied constraint-index name and preserves convergence", func(t *testing.T) { schema := testutil.NewSchema(t, pool) _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v int)", schema)) diff --git a/pkg/migrate/desired_test.go b/pkg/migrate/desired_test.go index f25b93a..32ecdd4 100644 --- a/pkg/migrate/desired_test.go +++ b/pkg/migrate/desired_test.go @@ -9,6 +9,7 @@ import ( "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" ) @@ -35,6 +36,21 @@ func TestRunDesiredRejectsForce(t *testing.T) { assert.Equal(t, DesiredResult{}, res) } +func TestCreateShapeCauseRejectsStatementCountMismatch(t *testing.T) { + ds, err := statement.ParseDesired("CREATE TABLE child PARTITION OF parent FOR VALUES IN (1)") + require.NoError(t, err) + missing := false + report := plan.Report{ + TableExists: &missing, + Statements: []plan.Statement{ + {Reason: verdict.ReasonUnsupportedStatement}, + {Reason: verdict.ReasonNone}, + }, + } + + assert.NoError(t, createShapeCause(DesiredRequest{Schema: "public", Desired: ds}, report, 0)) +} + func TestAdmitPlan(t *testing.T) { executable := func() plan.Report { exists := true diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index d0208e4..7ebba8b 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -140,22 +140,58 @@ type Report struct { // RefuseUnsupportedPartitionedParent marks executable statements as refused // when partition-aware admission rejects their execution steps. func RefuseUnsupportedPartitionedParent(report *Report, refused []bool) { + refuseStatements(report, verdict.ReasonUnsupportedPartitionedParent, func(i int) bool { + return i < len(refused) && refused[i] + }) +} + +// RefuseUnsupportedCreateShape marks the create-path statements whose +// connection-free shape checks refuse them. refused is positional over +// report.Statements — one entry per planned statement, nil where the +// statement is admitted. The report carries no field for the cause; callers +// that need the typed refusal keep the slice (executor.CreateShapeRefusals +// is pure, so it can also be recomputed from the desired schema). A length +// mismatch means the two sides no longer agree on what the plan contains, +// so no positional marking is safe and the report is left untouched. +func RefuseUnsupportedCreateShape(report *Report, refused []error) error { + if len(refused) != len(report.Statements) { + return fmt.Errorf("refuse create shapes: %d refusals for %d planned statements", len(refused), len(report.Statements)) + } + refuseStatements(report, verdict.ReasonUnsupportedStatement, func(i int) bool { + return refused[i] != nil + }) + return nil +} + +// refuseStatements withdraws every piece of execution advice from each +// statement the predicate selects and, when any statement was refused, stamps +// the reason on the report. An already-refused statement or report keeps its +// reason: the first refusal wins because an earlier mutator saw the more +// specific cause. Every refusal mutator goes through here, so a field added +// later is withdrawn in one place. +func refuseStatements(report *Report, reason verdict.Reason, refused func(i int) bool) { any := false for i := range report.Statements { - if i >= len(refused) || !refused[i] { + if !refused(i) { continue } any = true - report.Statements[i].Backend = "" - report.Statements[i].Disposition = router.DispositionRefuse - report.Statements[i].Reason = verdict.ReasonUnsupportedPartitionedParent - report.Statements[i].ExecSQL = nil - report.Statements[i].Execution = "" - withdrawSaferAdvice(&report.Statements[i]) + st := &report.Statements[i] + alreadyRefused := st.Disposition == router.DispositionRefuse + st.Backend = "" + st.Disposition = router.DispositionRefuse + if !alreadyRefused { + st.Reason = reason + } + st.ExecSQL = nil + st.Execution = "" + withdrawSaferAdvice(st) } if any { + if report.Disposition != router.DispositionRefuse { + report.Reason = reason + } report.Disposition = router.DispositionRefuse - report.Reason = verdict.ReasonUnsupportedPartitionedParent } } diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go index 0fdb3c4..f80fa6a 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -2,6 +2,7 @@ package plan_test import ( "encoding/json" + "errors" "fmt" "testing" @@ -177,6 +178,70 @@ func TestRefuseUnsupportedPartitionedParentWithdrawsExecutionAdvice(t *testing.T assert.Empty(t, r.Statements[0].Decisions[0].SaferSQLExecution) } +func TestRefuseUnsupportedCreateShapeMarksPositions(t *testing.T) { + r := plan.Report{ + Disposition: router.DispositionExecute, + Statements: []plan.Statement{ + {Backend: router.BackendNative, Disposition: router.DispositionExecute, ExecSQL: []string{"CREATE TABLE app.t (id int)"}, Execution: planner.ExecutionAutocommit}, + {Backend: router.BackendNative, Disposition: router.DispositionExecute, ExecSQL: []string{"CREATE INDEX i ON app.t (id)"}, Execution: planner.ExecutionAutocommit}, + }, + } + require.NoError(t, plan.RefuseUnsupportedCreateShape(&r, []error{errors.New("refused"), nil})) + + assert.Equal(t, router.DispositionRefuse, r.Disposition) + assert.Equal(t, verdict.ReasonUnsupportedStatement, r.Reason) + assert.Equal(t, router.DispositionRefuse, r.Statements[0].Disposition) + assert.Equal(t, verdict.ReasonUnsupportedStatement, r.Statements[0].Reason) + assert.Empty(t, r.Statements[0].Backend) + assert.Empty(t, r.Statements[0].ExecSQL) + assert.Equal(t, router.DispositionExecute, r.Statements[1].Disposition) + assert.Equal(t, router.BackendNative, r.Statements[1].Backend) + assert.NotEmpty(t, r.Statements[1].ExecSQL) +} + +func TestRefuseUnsupportedCreateShapePreservesExistingStatementReason(t *testing.T) { + r := plan.Report{ + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedPartitionedParent, + Statements: []plan.Statement{{ + Backend: router.BackendNative, + Disposition: router.DispositionRefuse, + Reason: verdict.ReasonUnsupportedPartitionedParent, + ExecSQL: []string{"CREATE TABLE app.t (id int)"}, + Execution: planner.ExecutionAutocommit, + }}, + } + require.NoError(t, plan.RefuseUnsupportedCreateShape(&r, []error{errors.New("refused")})) + + assert.Equal(t, router.DispositionRefuse, r.Disposition) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, r.Reason) + assert.Equal(t, verdict.ReasonUnsupportedPartitionedParent, r.Statements[0].Reason) + assert.Empty(t, r.Statements[0].Backend) + assert.Empty(t, r.Statements[0].ExecSQL) + assert.Empty(t, r.Statements[0].Execution) +} + +// A refusal slice that does not line up with the planned statements means +// the shape check and the plan disagree about what the plan contains; no +// positional marking is safe, so the report is left as it was. +func TestRefuseUnsupportedCreateShapeRejectsLengthMismatch(t *testing.T) { + r := plan.Report{ + Disposition: router.DispositionExecute, + Statements: []plan.Statement{ + {Backend: router.BackendNative, Disposition: router.DispositionExecute, ExecSQL: []string{"CREATE TABLE app.t (id int)"}, Execution: planner.ExecutionAutocommit}, + {Backend: router.BackendNative, Disposition: router.DispositionExecute, ExecSQL: []string{"CREATE INDEX i ON app.t (id)"}, Execution: planner.ExecutionAutocommit}, + }, + } + err := plan.RefuseUnsupportedCreateShape(&r, []error{errors.New("refused")}) + + require.Error(t, err) + assert.Equal(t, router.DispositionExecute, r.Disposition) + for i := range r.Statements { + assert.Equal(t, router.DispositionExecute, r.Statements[i].Disposition, "statement %d", i+1) + assert.NotEmpty(t, r.Statements[i].ExecSQL, "statement %d", i+1) + } +} + func TestDiscloseGreenfieldExecutionUsesPlainSQLForExecutableStatements(t *testing.T) { tableExists := false concurrent := "CREATE INDEX CONCURRENTLY i ON s.t (c)"