From 6f3b20e2bd8604c8239290f8b5e4b78ecece8ef0 Mon Sep 17 00:00:00 2001 From: kiran01bm <17925757+Kiran01bm@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:26:25 +1000 Subject: [PATCH 1/4] =?UTF-8?q?executor:=20ExecuteCreate=20=E2=80=94=20the?= =?UTF-8?q?=20create=20path=20for=20a=20verified-absent=20target=20(#62)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the create-path executor: `ExecuteCreate` runs a validated desired schema (one CREATE TABLE plus its indexes) against a name proven absent, with an off-ladder privilege proof for greenfield creation. ## Why The declarative front door can diff a desired table into existence, but nothing below it could execute that creation under the engine's proof discipline: the sequence executor consumes a `PreflightedTable`, which by definition cannot exist for a table that does not. The create path needs its own proof pair — the target name is free (`AbsentTarget`, already landed) and the role may create in the schema — and an executor that re-verifies both at the point of use. This lands that executor, dormant until the front door routes to it. ## What - `executor.ExecuteCreate` / `ExecuteCreateWithProgress`: qualifies every desired statement into the proof's schema, re-parses and admits by shape and target (ST-7), orders the CREATE TABLE first, and runs each step as a brief bounded transaction under the existing lock-retry machinery. Failure returns the committed-prefix `SequenceReport` contract; the duplicate-name SQLSTATEs (42P07 for a relation, 42710 for a standalone type holding the name) map to the typed `ErrCreateCollision` so the caller re-diffs instead of assuming. - Indexes build plainly, never CONCURRENTLY: the table is born this run with no traffic to protect, a plain build on an empty table is fast, and it cannot leave an INVALID index behind a failure. - Refusals, all at admission before anything executes: `IF NOT EXISTS` (table or index — a name-only no-op proves nothing); `CREATE TABLE PARTITION OF`, `INHERITS`, `LIKE`, and `OF type` (each binds a secondary relation or type the qualification never touches, so the name resolves via search_path to an existing object the absence proof does not cover); concurrent index builds; and a name claimed twice within the desired set (`ErrDuplicateCreateName` — decidable at admission, never a mid-run failure with a committed prefix). - `preflight.CheckCreatePrivileges` → `CreationRole` proof: one catalog snapshot proving CONNECT + schema USAGE + CREATE, with each missing grant a typed `*PrivilegeError` whose grantee is the engine role itself. Off the ownership tier ladder deliberately — a greenfield table has no owner to be a member of; it is born owned by its creator (`TierCreateTable`). - `statement.Op` now carries `IfNotExists`, `Inherits`, `Like`, and `OfType` for CREATE TABLE; four new outcome codes (`create-collision`, `duplicate-create-name`, `partition-of-unsupported`, `unsupported-create-step`); docs updated (ST-7 enforcement list, SAFETY.md / tcb-model.md / review-checks proof types, engine-role.md off-ladder section, capabilities/limitations/README create-path boundaries). ## Before / after ``` Before: no execution path for a desired table that does not exist yet ParseDesired ──▶ DesiredSchema ──▶ (no executor consumes it) CheckTableAbsent ──▶ AbsentTarget ──▶ (no executor consumes it) After: the create path, proof-gated end to end CheckCreatePrivileges ──▶ CreationRole (may I create here?) CheckTableAbsent ─────────▶ AbsentTarget (is the name free?) │ ParseDesired ──▶ DesiredSchema ──┤ ▼ ExecuteCreate qualify + re-parse + admit (ST-7) refuse: PARTITION OF / INHERITS / LIKE / OF / IF NOT EXISTS / CONCURRENTLY / duplicate names │ ┌──────────────┼──────────────┐ ▼ ▼ ▼ CREATE TABLE CREATE INDEX CREATE INDEX ... (always 1st) (input order, plain builds, brief budgets) 42P07 / 42710 ──▶ ErrCreateCollision ──▶ caller re-diffs the live catalog failed step ──▶ committed prefix remains ──▶ rerun refuses ErrRelationExists ──▶ re-diff ``` --- .agents/checks/review.md | 2 +- README.md | 3 + SAFETY.md | 4 +- docs/capabilities.md | 2 +- docs/engine-role.md | 9 + docs/execution-model.md | 35 ++ docs/invariants.md | 5 +- docs/limitations.md | 1 + docs/schemabot-integration.md | 40 +- docs/tcb-model.md | 1 + pkg/executor/code.go | 56 ++- pkg/executor/code_test.go | 4 + pkg/executor/create.go | 308 +++++++++++++++ pkg/executor/create_integration_test.go | 419 +++++++++++++++++++++ pkg/executor/create_test.go | 59 +++ pkg/executor/docs_test.go | 24 ++ pkg/executor/native.go | 11 +- pkg/executor/optimistic.go | 17 +- pkg/preflight/create.go | 98 +++++ pkg/preflight/create_integration_test.go | 104 +++++ pkg/preflight/docs_test.go | 2 +- pkg/preflight/privileges.go | 53 ++- pkg/statement/implicit.go | 180 +++++++++ pkg/statement/implicit_integration_test.go | 77 ++++ pkg/statement/implicit_test.go | 118 ++++++ pkg/statement/ops.go | 34 +- pkg/statement/ops_test.go | 25 ++ 27 files changed, 1648 insertions(+), 43 deletions(-) create mode 100644 pkg/executor/create.go create mode 100644 pkg/executor/create_integration_test.go create mode 100644 pkg/executor/create_test.go create mode 100644 pkg/preflight/create.go create mode 100644 pkg/preflight/create_integration_test.go create mode 100644 pkg/statement/implicit.go create mode 100644 pkg/statement/implicit_integration_test.go create mode 100644 pkg/statement/implicit_test.go diff --git a/.agents/checks/review.md b/.agents/checks/review.md index a032ffa..af8fee7 100644 --- a/.agents/checks/review.md +++ b/.agents/checks/review.md @@ -16,7 +16,7 @@ the reviewer's distillation. - Core packages: every loop, queue, retry, and wait must be bounded. An unbounded anything in a core package is a review-blocking defect. - Dangerous APIs accept proof types (`statement.Classified`, `PreflightedTable`, - `AbsentTarget`, `VerifiedShadow`, `CleanWatermark`, `TableLock`) with package-private + `AbsentTarget`, `CreationRole`, `VerifiedShadow`, `CleanWatermark`, `TableLock`) with package-private constructors — never a raw string or bool that a caller could fabricate. Core code re-verifies its own preconditions; it never trusts that the planner or CLI checked. diff --git a/README.md b/README.md index 64cefcb..69431ba 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,9 @@ 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. - **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/SAFETY.md b/SAFETY.md index f58d66f..d2a7bc8 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -67,8 +67,8 @@ The short version — the full rules live in [docs/tcb-model.md](docs/tcb-model. - **Never trust callers.** Every dangerous operation re-verifies its preconditions, whoever the requester is (CLI, planner, orchestrator). The periphery may request; the core enforces. - **Domain types make illegal states unrepresentable.** Validating passages return proof types - with package-private constructors (today `preflight.PreflightedTable` and - `preflight.AbsentTarget`; later phases add + with package-private constructors (today `preflight.PreflightedTable`, + `preflight.AbsentTarget`, and `preflight.CreationRole`; later phases add `VerifiedShadow`, `CleanWatermark`, and `TableLock`); dangerous APIs accept only proof types — e.g. the planned cutover swap will accept only a `VerifiedShadow`. - **Put a limit on everything.** Every loop bounded, every queue bounded, every retry counted, diff --git a/docs/capabilities.md b/docs/capabilities.md index f12d86d..bc5fe79 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -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`) is in place; the executor create path and front-door admission build on it. `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 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 | ### Types and non-table objects diff --git a/docs/engine-role.md b/docs/engine-role.md index b3b977b..c92740a 100644 --- a/docs/engine-role.md +++ b/docs/engine-role.md @@ -55,6 +55,15 @@ same preflight: `wal_level = logical` (`rds.logical_replication = 1` on Aurora/R static parameter requiring a reboot), and free `max_replication_slots` / `max_wal_senders` headroom. +### Off-ladder: greenfield `CREATE TABLE` + +Creating a new table sits outside the ladder: the table does not exist yet, so there is no +owning role to be a member of — the table is born owned by the role that creates it. The +create path's preflight (`CheckCreatePrivileges`) therefore proves exactly `CONNECT` on the +database plus `USAGE` and `CREATE` on the target schema, deliberately not the Tier 1–3 +ownership membership. A missing grant is refused with the exact `GRANT` statement, whose +grantee is the engine role itself. + ## Provisioning For a target whose tables are owned by `app_owner` in schema `app`: diff --git a/docs/execution-model.md b/docs/execution-model.md index 034ebbf..d003406 100644 --- a/docs/execution-model.md +++ b/docs/execution-model.md @@ -21,6 +21,7 @@ and [suggest-report.md](suggest-report.md#caveats-caveats). - [The committed prefix](#the-committed-prefix) - [How a failure is reported](#how-a-failure-is-reported) - [Why the prefix is safe to leave](#why-the-prefix-is-safe-to-leave) +- [Outcome codes](#outcome-codes) ## Why there is no wrapping transaction @@ -241,3 +242,37 @@ statement — stopping at the first refusal or failure. Its result carries the plan, one verdict per attempted statement, and a detail naming exactly which planned statements committed and remain in effect: the committed prefix at the plan level, statements instead of steps. + +## Outcome codes + +`executor.Codes()` enumerates the closed vocabulary below, and +`executor.OutcomeCode` maps any executor error to its entry — the same code +that reaches the JSON verdict's `code` field. Adapters render three facts +per failure — the outcome code, the failing step's position +(`SequenceStepError.Step` of `.Total`), and the failing step's SQL — and +log the raw error, whose text interpolates server prose and is not a +branching surface. + +| Code | Meaning | +| --- | --- | +| `budget-lock-exceeded` | The lock was not granted within `lock_timeout`; nothing executed | +| `budget-statement-exceeded` | The statement ran past `statement_timeout` and was cancelled | +| `cancelled-externally` | The statement was cancelled from outside the executor before its budget elapsed | +| `invalid-index-own-leftover` | The failed build's own INVALID index remains; the [recovery runbook](invalid-index-recovery.md) applies | +| `invalid-index-preexisting` | An INVALID index under the requested name predates this run | +| `invalid-index-unproven` | An INVALID index may remain but the catalog state could not be proven | +| `empty-sequence` | The sequence had no steps to run | +| `unsupported-sequence-step` | A step is not a shape the sequence executor can run safely | +| `unsupported-partitioned-parent` | Partitioned-parent admission refusal | +| `not-concurrent-index-build` | The statement handed to the concurrent build executor is not a `CREATE INDEX CONCURRENTLY` | +| `unnamed-index` | The concurrent build does not name its index, so its outcome could not be verified | +| `unqualified-table` | The target table is not schema-qualified at the library boundary | +| `if-not-exists-unsupported` | `CREATE ... IF NOT EXISTS` cannot prove what its no-op would mean | +| `create-collision` | A name the create path needs is already taken on the server; re-diff the live catalog | +| `duplicate-create-name` | The desired set claims the same relation name twice; refused at admission | +| `partition-of-unsupported` | `CREATE TABLE PARTITION OF` locks the partitioned parent, which the absence proof does not cover | +| `unsupported-create-step` | A desired statement is not a shape the create path can run | +| `pool-too-small` | The pool cannot hold the build session and the verdict connection at once | +| `table-not-found` | The statement's qualified table does not exist | +| `invariant-violation` | A breach of the invariant registry; never a retry candidate | +| `execution-failed` | Fallback for a failure outside the typed set — an operational error to investigate, not a refusal to branch on | diff --git a/docs/invariants.md b/docs/invariants.md index ff2f0d7..b75308c 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -244,8 +244,9 @@ executes, any statement whose target table does not match the preflight proof it A proof for one table can never smuggle SQL against another, and a multi-statement string can never reach the database through the executor (pgx's simple protocol would happily run all of it). *Enforced:* `pkg/executor` (`ExecuteNative`; `RunSequence` admission re-proves every step's -target against the preflight proof before the first step executes), `pkg/statement` (proof -construction). +target against the preflight proof before the first step executes; `ExecuteCreate` re-proves +every desired statement's target against the absence proof the same way), `pkg/statement` +(proof construction). *Source:* adversarial review of the optimistic front door. ## Refusals and preflight (RF) diff --git a/docs/limitations.md b/docs/limitations.md index 92cae4e..202e0b1 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -29,6 +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. | | 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/schemabot-integration.md b/docs/schemabot-integration.md index b986f22..773191b 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -143,22 +143,46 @@ package. Landing this is one of: ### Routing the create path's refusals -The planned greenfield `CREATE TABLE` path opens with `preflight.CheckTableAbsent`, and its -proof has a rule the adapter must respect: an `AbsentTarget` is **minted inside the apply +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. +4. `executor.ExecuteCreate` — consume both proofs and run the set. + +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 at plan time proves nothing about -apply time; the executor re-verifies inside the session that runs the `CREATE`, the same way -ST-7 re-verifies a `PreflightedTable`. +the plan/apply boundary, or reused across retries. Absence or privilege at plan time proves +nothing about apply time; the executor re-verifies inside the session that runs the +`CREATE`, the same way ST-7 re-verifies a `PreflightedTable`. -Each refusal from the check maps to a different orchestrator action — route them, don't -retry them uniformly: +Each refusal from the preflight checks maps to a different orchestrator action — route +them, don't retry them uniformly: | Refusal | What it means | Orchestrator action | | --- | --- | --- | | `ErrRelationExists` / `ErrTypeExists` (grouped by `preflight.IsNameOccupied`) | The name is already taken — this is not a create, it's a change to something that exists | Route to the diff/alter path, not to a failure state | | `ErrSchemaNotFound` | The qualified schema does not exist on the target | Operator action (create the schema or fix the desired file); retrying cannot succeed | | `ErrNoCreationSchema` | Unqualified name and the role's `search_path` yields no creation schema | Caller configuration: schema-qualify the name or fix the role's `search_path` | -| Duplicate-name error from the `CREATE` itself | A concurrent writer won the race after a valid proof | Re-plan from scratch — the world changed; do not blindly retry the create | +| `*preflight.PrivilegeError` (`Tier == TierCreateTable`) | The role lacks `CREATE` on the schema (or `USAGE` reaching it); the error carries the exact missing grant | Operator action: provision the named `GRANT`, then retry | + +`ExecuteCreate`'s own refusals and failures carry the same routing discipline +([outcome codes](execution-model.md#outcome-codes)): + +| 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 | +| `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 | + +A failed create is not rolled back wholesale: each step committed in its own bounded +transaction, so the steps before the failure remain +([the committed prefix](execution-model.md#the-committed-prefix)). A rerun's absence check +then refuses with `ErrRelationExists`, and the gate stays closed until the declarative +front door re-diffs the live catalog and converges the remainder — the orchestrator never +assumes the failed run left nothing behind. ## Execution-mode verdicts and direct execution diff --git a/docs/tcb-model.md b/docs/tcb-model.md index ac54fe3..0daa056 100644 --- a/docs/tcb-model.md +++ b/docs/tcb-model.md @@ -92,6 +92,7 @@ to obtain the type is through the function that validates it. | `string` (user SQL) | `statement.ParseOne` / `statement.ParseOps`, then `planner.Classify` | `planner.Plan` / `planner.Decision` | CO-7 — classification consumes parsed operation descriptors | | table name | preflight | `PreflightedTable` (carries the proven facts: PK, no FKs/views, replica identity, headroom) | ST-6, RF-* | | table name (create target) | `preflight.CheckTableAbsent` | `AbsentTarget` (carries the resolved creation schema and the verified-free name; time-of-check — minted inside the apply session, never carried across a plan boundary, and re-verified at use the way ST-7 re-verifies `PreflightedTable`) | ST-6 for the create path | +| creating role's access (create target) | `preflight.CheckCreatePrivileges` | `CreationRole` (carries the connected role and the resolved creation schema whose CONNECT / USAGE / CREATE grants were verified; time-of-check and session-scoped, like `AbsentTarget` — a revoked grant after minting fails with the server's own error) | ST-6 for the create path | | shadow table | full checksum pass (planned) | `VerifiedShadow` — its constructor will be private to `pkg/checksum`; the planned `cutover.Swap` will accept **only** this type | CO-1 in the type system | | chunker low-watermark | all-checkers-clean pass (planned) | `CleanWatermark` — will be unobtainable in a pass that repaired anything | CO-2 | | — | planned table-lock acquisition | `TableLock` token, planned as a required parameter of every mutating operation | LK-1 | diff --git a/pkg/executor/code.go b/pkg/executor/code.go index 7780812..1bbe3a7 100644 --- a/pkg/executor/code.go +++ b/pkg/executor/code.go @@ -53,9 +53,23 @@ const ( // CodeUnqualifiedTable: the target table is not schema-qualified at // the library boundary. CodeUnqualifiedTable Code = "unqualified-table" - // CodeIfNotExistsUnsupported: CREATE INDEX CONCURRENTLY IF NOT EXISTS - // cannot prove what its no-op would mean. + // CodeIfNotExistsUnsupported: CREATE ... IF NOT EXISTS cannot prove + // what its no-op would mean. CodeIfNotExistsUnsupported Code = "if-not-exists-unsupported" + // CodeCreateCollision: a name the create path needs is already taken + // on the server; the caller re-diffs the live catalog rather than + // assuming the occupant's shape. + CodeCreateCollision Code = "create-collision" + // CodeDuplicateCreateName: the desired set claims the same relation + // name twice; the conflict is decidable at admission and refused + // before anything runs. + CodeDuplicateCreateName Code = "duplicate-create-name" + // CodePartitionOfUnsupported: CREATE TABLE PARTITION OF locks the + // partitioned parent, which the absence proof does not cover. + CodePartitionOfUnsupported Code = "partition-of-unsupported" + // CodeUnsupportedCreateStep: a desired statement is not a shape the + // create path can run. + CodeUnsupportedCreateStep Code = "unsupported-create-step" // CodePoolTooSmall: the pool cannot hold the build session and the // verdict connection at once. CodePoolTooSmall Code = "pool-too-small" @@ -71,6 +85,36 @@ const ( CodeExecutionFailed Code = "execution-failed" ) +// Codes returns the closed set of outcome codes. It is part of the report +// contract: adapters enumerate it to know every outcome they must render, +// and the docs test pins every code into the execution-model page so the +// documented vocabulary cannot drift from this one. +func Codes() []Code { + return []Code{ + CodeBudgetLockExceeded, + CodeBudgetStatementExceeded, + CodeCancelledExternally, + CodeInvalidIndexOwnLeftover, + CodeInvalidIndexPreexisting, + CodeInvalidIndexUnproven, + CodeEmptySequence, + CodeUnsupportedSequenceStep, + CodeUnsupportedPartitionedParent, + CodeNotConcurrentIndexBuild, + CodeUnnamedIndex, + CodeUnqualifiedTable, + CodeIfNotExistsUnsupported, + CodeCreateCollision, + CodeDuplicateCreateName, + CodePartitionOfUnsupported, + CodeUnsupportedCreateStep, + CodePoolTooSmall, + CodeTableNotFound, + CodeInvariantViolation, + CodeExecutionFailed, + } +} + // OutcomeCode maps an error returned by this package to its stable code. // A nil error has no outcome code and maps to the empty Code. A // *SequenceStepError carries its failed step's own cause, so it maps to @@ -119,6 +163,14 @@ func sentinelCode(err error) Code { return CodeUnqualifiedTable case errors.Is(err, ErrIfNotExistsUnsupported): return CodeIfNotExistsUnsupported + case errors.Is(err, ErrCreateCollision): + return CodeCreateCollision + case errors.Is(err, ErrDuplicateCreateName): + return CodeDuplicateCreateName + case errors.Is(err, ErrPartitionOfUnsupported): + return CodePartitionOfUnsupported + case errors.Is(err, ErrUnsupportedCreateStep): + return CodeUnsupportedCreateStep case errors.Is(err, ErrPoolTooSmall): return CodePoolTooSmall case errors.Is(err, ErrTableNotFound): diff --git a/pkg/executor/code_test.go b/pkg/executor/code_test.go index 6c497f1..84745ea 100644 --- a/pkg/executor/code_test.go +++ b/pkg/executor/code_test.go @@ -52,6 +52,10 @@ func TestOutcomeCodeMapsTypedOutcomes(t *testing.T) { {name: "unnamed index", err: executor.ErrUnnamedIndex, want: executor.CodeUnnamedIndex}, {name: "unqualified table", err: executor.ErrUnqualifiedTable, want: executor.CodeUnqualifiedTable}, {name: "if not exists", err: executor.ErrIfNotExistsUnsupported, want: executor.CodeIfNotExistsUnsupported}, + {name: "create collision", err: executor.ErrCreateCollision, want: executor.CodeCreateCollision}, + {name: "duplicate create name", err: executor.ErrDuplicateCreateName, want: executor.CodeDuplicateCreateName}, + {name: "partition of", err: executor.ErrPartitionOfUnsupported, want: executor.CodePartitionOfUnsupported}, + {name: "unsupported create step", err: executor.ErrUnsupportedCreateStep, want: executor.CodeUnsupportedCreateStep}, {name: "pool too small", err: executor.ErrPoolTooSmall, want: executor.CodePoolTooSmall}, {name: "table not found", err: executor.ErrTableNotFound, want: executor.CodeTableNotFound}, {name: "invariant violation", err: executor.ErrInvariantViolation, want: executor.CodeInvariantViolation}, diff --git a/pkg/executor/create.go b/pkg/executor/create.go new file mode 100644 index 0000000..7187d64 --- /dev/null +++ b/pkg/executor/create.go @@ -0,0 +1,308 @@ +// This file is the create-path executor: it runs a validated desired +// schema — one CREATE TABLE plus its indexes — against a name the caller +// proved absent. Every step is a brief bounded transactional run: the +// table is born this run and carries no traffic, so its indexes are built +// plainly rather than CONCURRENTLY — a plain build on an empty table is +// fast, and unlike CONCURRENTLY it cannot leave an INVALID index behind a +// failure. The executor never trusts the caller's classification (see +// SAFETY.md): each desired statement is qualified into the proof's schema, +// re-parsed by the real grammar, and admitted by shape and target before +// anything executes. +// +// The absence proof is time-of-check: nothing locks the name, so a +// concurrent create can still take it between the check and a step here. +// That loss surfaces as SQLSTATE 42P07 and is returned as the typed +// ErrCreateCollision — the caller re-diffs the live catalog rather than +// assuming what the collision left behind. A failed step ends the run +// immediately; the steps before it committed (each in its own bounded +// transaction) and remain, so a rerun's absence check refuses with +// ErrRelationExists and the declarative front door re-diffs and applies +// the remainder. + +package executor + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/block/pg-sprite/pkg/preflight" + "github.com/block/pg-sprite/pkg/progress" + "github.com/block/pg-sprite/pkg/statement" +) + +// Typed refusals and failures for the create path. Admission covers every +// desired statement before the first executes, so a creation this executor +// cannot finish is never started. +var ( + // ErrCreateCollision is returned when a step fails because its target + // name is already taken. For the table name that means a concurrent + // create won the race — the absence proof is time-of-check. Index + // names are never absence-checked, so a pre-existing occupant at an + // index name reports the same way. Either way the caller re-diffs the + // live catalog; nothing about the occupant's shape can be assumed. + ErrCreateCollision = errors.New("a name the create path needs is already taken") + // ErrDuplicateCreateName is returned when the desired set claims the + // same relation name twice — two indexes under one name, or an index + // named after the table. The conflict is decidable before anything + // runs, so admission refuses the whole set rather than letting a + // mid-run step fail after a prefix committed. + ErrDuplicateCreateName = errors.New("desired set claims the same relation name twice") + // ErrPartitionOfUnsupported is returned for CREATE TABLE ... PARTITION + // OF: attaching a partition takes a lock on the partitioned parent, + // an existing table the absence proof says nothing about. + ErrPartitionOfUnsupported = errors.New("CREATE TABLE PARTITION OF is not supported by the create path: attaching a partition locks the partitioned parent, which the absence proof does not cover") + // ErrUnsupportedCreateStep is returned when a desired statement is not + // a shape the create path can run: a plain CREATE TABLE or a plain + // CREATE INDEX on the new table. CONCURRENTLY is refused deliberately — + // the table is born this run with no traffic to protect, and a plain + // build cannot leave an INVALID index behind a failure. + ErrUnsupportedCreateStep = errors.New("statement is not a shape the create path can run") +) + +// The SQLSTATEs a create step raises when its target name is already +// taken. Postgres errors are matched by SQLSTATE, never by message text. +const ( + // sqlstateDuplicateTable: the occupant is a relation — any kind, an + // index included. + sqlstateDuplicateTable = "42P07" + // sqlstateDuplicateObject: the occupant is not a relation — a + // standalone type under the table's name raises it, because every + // table also mints a composite type of the same name. + sqlstateDuplicateObject = "42710" +) + +// ExecuteCreate runs the desired schema's statements against the +// verified-absent target: the CREATE TABLE first, then its indexes in +// input order, each step a bounded transactional run under the brief +// budgets, exactly like an optimistic attempt, with its search_path +// pinned to the proof's schema then public — the same policy the +// introspection read path sets — so the desired file's unqualified +// references resolve exactly as the diff resolved them. The pool must +// come from pkg/dbconn and must be the session the proofs were minted on: +// cr proves that session's role can create in the proof's schema, and +// like the absence proof it is time-of-check — a grant revoked after +// minting fails with the server's own error. Every desired statement is +// qualified into the proof's schema, re-parsed, and admitted by shape and +// target before the first step executes. On success every step committed +// and the report says what each did. On failure the run stops at the +// failing step and returns a typed *SequenceStepError; the committed +// prefix remains — a rerun's absence check then refuses with +// preflight.ErrRelationExists, and the caller re-diffs the live catalog +// to apply the remainder. retry bounds lock_timeout retries on each step, +// exactly as in ExecuteNative. +func ExecuteCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentTarget, cr preflight.CreationRole, ds statement.DesiredSchema, b Budget, retry RetryPolicy) (SequenceReport, error) { + return executeCreate(ctx, pool, at, cr, ds, b, retry, nil) +} + +// ExecuteCreateWithProgress runs the create path while updating tracker +// with the current step. The caller may poll concurrently. +func ExecuteCreateWithProgress(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentTarget, cr preflight.CreationRole, ds statement.DesiredSchema, b Budget, retry RetryPolicy, tracker *progress.Tracker) (rep SequenceReport, err error) { + if tracker == nil { + return rep, fmt.Errorf("%w: progress tracker is required", ErrInvariantViolation) + } + tracker.Start(len(ds.Statements()), progress.OperationAdmitting) + defer func() { tracker.Finish(err) }() + return executeCreate(ctx, pool, at, cr, ds, b, retry, tracker) +} + +func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentTarget, cr preflight.CreationRole, ds statement.DesiredSchema, b Budget, retry RetryPolicy, tracker *progress.Tracker) (SequenceReport, error) { + var rep SequenceReport + if err := b.validate(); err != nil { + return rep, err + } + if err := retry.validate(); err != nil { + return rep, err + } + // INV: ST-7 — the proofs are re-verified at the point of use. Zero + // values are forgeable by any package: only CheckTableAbsent mints an + // AbsentTarget with a table, only CheckCreatePrivileges mints a + // CreationRole with a schema, and only ParseDesired mints a + // DesiredSchema with a table. + if at.Schema() == "" || at.Table() == "" { + return rep, fmt.Errorf("%w: ST-7: absence proof carries no verified target", ErrInvariantViolation) + } + if cr.Schema() == "" { + return rep, fmt.Errorf("%w: ST-7: creation-privilege proof carries no verified schema", ErrInvariantViolation) + } + if cr.Schema() != at.Schema() { + return rep, fmt.Errorf("%w: ST-7: creation privileges were verified in %q but absence in %q", + ErrInvariantViolation, cr.Schema(), at.Schema()) + } + if ds.Table() == "" { + return rep, fmt.Errorf("%w: ST-7: desired schema carries no admitted CREATE TABLE", ErrInvariantViolation) + } + if ds.Table() != at.Table() { + return rep, fmt.Errorf("%w: ST-7: desired schema targets %q but absence was verified for %q", + ErrInvariantViolation, ds.Table(), at.Table()) + } + steps, err := admitCreateSteps(at, ds) + if err != nil { + return rep, err + } + for i, step := range steps { + start := time.Now() + if tracker != nil { + tracker.StartStep(i+1, progress.OperationBrief) + start = tracker.Now() + } + err := executeWithLockRetryObserved(ctx, retry, func(ctx context.Context) error { + return executeBoundedAttempt(ctx, pool, step, b, at.Schema()) + }, sleepContext, func(attempt int) { + if tracker != nil { + tracker.SetAttempt(attempt) + } + }) + if err != nil { + return rep, &SequenceStepError{Step: i + 1, Total: len(steps), Kind: StepBrief, SQL: step.SQL(), Err: asCreateCollision(err)} + } + rep.Steps = append(rep.Steps, StepReport{ + SQL: step.SQL(), + Kind: StepBrief, + Duration: elapsedSince(tracker, start), + }) + } + return rep, nil +} + +// 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 +// 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 +// anything runs rather than failing mid-run after a prefix committed. +// The claims are first choices: a set whose first choices collide is +// refused even where the server would sidestep with a numeric suffix, +// 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. +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)) + claimed := make(map[string]struct{}, len(desired)) + for i, raw := range desired { + st, names, err := admitCreateStep(at, raw.SQL()) + if err != nil { + return nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) + } + for _, name := range names { + if _, taken := claimed[name]; taken { + return nil, fmt.Errorf("desired statement %d of %d: %w: %q", i+1, len(desired), ErrDuplicateCreateName, name) + } + 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) + } + return append([]statement.Statement{createStep}, indexSteps...), 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 +// returns the pg_class names the step will claim — for a CREATE TABLE the +// table name plus the first-choice index names of its index-backed +// constraints, for a CREATE INDEX its explicit name, nothing when the +// server invents one. CREATE TABLE clauses that bind to a secondary +// relation or type — PARTITION OF, INHERITS, LIKE, OF — are refused: +// 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()) + if err != nil { + return statement.Statement{}, nil, err + } + st, err := statement.ParseOne(qualified) + if err != nil { + return statement.Statement{}, nil, err + } + ops, err := statement.ParseOps(qualified) + if err != nil { + return statement.Statement{}, nil, err + } + 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)) + } + 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.ImplicitIndexNames(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...) + 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} + } + default: + return statement.Statement{}, nil, 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 st, claims, nil +} + +// asCreateCollision maps the duplicate-name SQLSTATEs — 42P07 when a +// relation holds the name, 42710 when a standalone type does — to the +// typed collision refusal. Every other error passes through unchanged. +// The server's error names the occupant, so the wrap adds the +// classification, not the identifier. +func asCreateCollision(err error) error { + var pgErr *pgconn.PgError + if !errors.As(err, &pgErr) { + return err + } + if pgErr.Code != sqlstateDuplicateTable && pgErr.Code != sqlstateDuplicateObject { + return err + } + return fmt.Errorf("%w: %w", ErrCreateCollision, err) +} diff --git a/pkg/executor/create_integration_test.go b/pkg/executor/create_integration_test.go new file mode 100644 index 0000000..7fbdb74 --- /dev/null +++ b/pkg/executor/create_integration_test.go @@ -0,0 +1,419 @@ +package executor_test + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + "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/executor" + "github.com/block/pg-sprite/pkg/preflight" + "github.com/block/pg-sprite/pkg/statement" +) + +// createFixture is one schema on a real server with an absence proof and +// a creation-privilege proof minted for the named table — the inputs +// ExecuteCreate requires. +type createFixture struct { + pool *pgxpool.Pool + schema string + at preflight.AbsentTarget + cr preflight.CreationRole +} + +func newCreateFixture(t *testing.T, table string) createFixture { + t.Helper() + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + t.Cleanup(pool.Close) + schema := testutil.NewSchema(t, pool) + + at, err := preflight.CheckTableAbsent(t.Context(), pool, schema, table) + require.NoError(t, err) + cr, err := preflight.CheckCreatePrivileges(t.Context(), pool, schema) + require.NoError(t, err) + return createFixture{pool: pool, schema: schema, at: at, cr: cr} +} + +func desired(t *testing.T, sql string) statement.DesiredSchema { + t.Helper() + ds, err := statement.ParseDesired(sql) + require.NoError(t, err) + return ds +} + +// relationKind returns the pg_class relkind of schema.name, or "" when no +// relation owns the name — the catalog oracle for what a create run left. +func relationKind(t *testing.T, pool *pgxpool.Pool, schema, name string) string { + t.Helper() + var relkind *string + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT c.relkind::text + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 AND c.relname = $2`, + schema, name).Scan(&relkind)) + if relkind == nil { + return "" + } + return *relkind +} + +// relationExists reports whether any relation owns schema.name, for +// assertions where only presence matters. +func relationExists(t *testing.T, pool *pgxpool.Pool, schema, name string) bool { + t.Helper() + var exists bool + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT EXISTS ( + SELECT FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 AND c.relname = $2)`, + schema, name).Scan(&exists)) + return exists +} + +func TestExecuteCreateRunsTableAndIndexes(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, ` + CREATE TABLE t (id int PRIMARY KEY, name text); + CREATE INDEX t_name_idx ON t (name); + CREATE UNIQUE INDEX t_id_name_idx ON t (id, name); + `) + + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.NoError(t, err) + + assert.Equal(t, "r", relationKind(t, f.pool, f.schema, "t")) + assert.Equal(t, "i", relationKind(t, f.pool, f.schema, "t_name_idx")) + assert.Equal(t, "i", relationKind(t, f.pool, f.schema, "t_id_name_idx")) + + require.Len(t, rep.Steps, 3) + assert.Contains(t, rep.Steps[0].SQL, "CREATE TABLE") + for _, step := range rep.Steps { + assert.Equal(t, executor.StepBrief, step.Kind) + assert.GreaterOrEqual(t, step.Duration, time.Duration(0)) + } +} + +// A desired file may state its index before its table — declarative input +// carries no ordering contract — but an index cannot be built before its +// table exists, so the executor orders the CREATE TABLE first. +func TestExecuteCreateOrdersTableBeforeIndexes(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, ` + CREATE INDEX t_name_idx ON t (name); + CREATE TABLE t (id int, name text); + `) + + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.NoError(t, err) + + require.Len(t, rep.Steps, 2) + assert.Contains(t, rep.Steps[0].SQL, "CREATE TABLE") + assert.Equal(t, "i", relationKind(t, f.pool, f.schema, "t_name_idx")) +} + +// The absence proof is time-of-check: a create that takes the name after +// the check surfaces as the typed collision, and the caller re-diffs +// rather than assuming what the occupant looks like. +func TestExecuteCreateReportsCollisionAsTyped(t *testing.T) { + f := newCreateFixture(t, "t") + _, err := f.pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (other int)", f.schema)) + require.NoError(t, err) + + ds := desired(t, "CREATE TABLE t (id int)") + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.Error(t, err) + + var stepErr *executor.SequenceStepError + require.ErrorAs(t, err, &stepErr) + assert.Equal(t, 1, stepErr.Step) + assert.ErrorIs(t, err, executor.ErrCreateCollision) + assert.Equal(t, executor.CodeCreateCollision, executor.OutcomeCode(err)) + assert.Empty(t, rep.Steps) +} + +// A failed step ends the run; the steps before it committed and remain, +// and the report covers exactly that prefix so the caller can disclose +// what already happened. An index on a column the table does not have +// passes admission — admission checks shape and target, not column +// existence — and fails only when the server executes it. +func TestExecuteCreateFailedStepKeepsCommittedPrefix(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, ` + CREATE TABLE t (id int, name text); + CREATE INDEX t_id_idx ON t (id); + CREATE INDEX t_missing_idx ON t (missing); + `) + + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.Error(t, err) + + var stepErr *executor.SequenceStepError + require.ErrorAs(t, err, &stepErr) + assert.Equal(t, 3, stepErr.Step) + assert.Equal(t, 3, stepErr.Total) + // The server error is not a collision; it passes through untyped. + assert.NotErrorIs(t, err, executor.ErrCreateCollision) + assert.Equal(t, executor.CodeExecutionFailed, executor.OutcomeCode(err)) + + assert.True(t, relationExists(t, f.pool, f.schema, "t")) + assert.True(t, relationExists(t, f.pool, f.schema, "t_id_idx")) + require.Len(t, rep.Steps, 2) + + // The committed prefix is the rerun contract: the absence check now + // refuses, which is the declarative front door's signal to re-diff. + _, err = preflight.CheckTableAbsent(t.Context(), f.pool, f.schema, "t") + assert.ErrorIs(t, err, preflight.ErrRelationExists) +} + +// A name claimed twice within the desired set is decidable at admission, +// so the whole set refuses before the first step runs — never a mid-run +// failure with a committed prefix. +func TestExecuteCreateRefusesDuplicateNamesAtAdmission(t *testing.T) { + tests := []struct { + name string + sql string + }{ + { + name: "two indexes under one name", + sql: `CREATE TABLE t (id int, name text); + CREATE INDEX dup_idx ON t (id); + CREATE INDEX dup_idx ON t (name)`, + }, + { + name: "index named after the table", + sql: `CREATE TABLE t (id int); + CREATE INDEX t ON t (id)`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, tt.sql) + + _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrDuplicateCreateName) + assert.Equal(t, executor.CodeDuplicateCreateName, executor.OutcomeCode(err)) + assert.False(t, relationExists(t, f.pool, f.schema, "t"), + "admission covers the whole set before the first step executes") + }) + } +} + +// A standalone type occupying the table's name raises a different SQLSTATE +// than a relation would — every table also mints a composite type — and +// still surfaces as the typed collision. +func TestExecuteCreateReportsTypeCollisionAsTyped(t *testing.T) { + f := newCreateFixture(t, "t") + _, err := f.pool.Exec(t.Context(), fmt.Sprintf("CREATE TYPE %s.t AS ENUM ('a')", f.schema)) + require.NoError(t, err) + + ds := desired(t, "CREATE TABLE t (id int)") + _, err = executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrCreateCollision) + assert.Equal(t, executor.CodeCreateCollision, executor.OutcomeCode(err)) +} + +func TestExecuteCreateAdmissionRefusals(t *testing.T) { + tests := []struct { + name string + sql string + wantErr error + }{ + { + name: "if not exists on the table", + sql: "CREATE TABLE IF NOT EXISTS t (id int)", + wantErr: executor.ErrIfNotExistsUnsupported, + }, + { + name: "if not exists on an index", + sql: "CREATE TABLE t (id int); CREATE INDEX IF NOT EXISTS t_idx ON t (id)", + wantErr: executor.ErrIfNotExistsUnsupported, + }, + // INHERITS, LIKE, and OF bind to a secondary relation or type the + // qualification never touches: the name resolves via search_path + // to an existing object the absence proof says nothing about. + { + name: "inherits from an existing parent", + sql: "CREATE TABLE t (id int) INHERITS (parent)", + wantErr: executor.ErrUnsupportedCreateStep, + }, + { + name: "like an existing source table", + sql: "CREATE TABLE t (LIKE src INCLUDING ALL)", + wantErr: executor.ErrUnsupportedCreateStep, + }, + { + name: "of an existing composite type", + sql: "CREATE TABLE t OF ty", + wantErr: executor.ErrUnsupportedCreateStep, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, tt.sql) + + _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, tt.wantErr) + assert.False(t, relationExists(t, f.pool, f.schema, "t"), + "admission covers the whole set before the first step executes") + }) + } +} + +func TestExecuteCreateRefusesPartitionOf(t *testing.T) { + f := newCreateFixture(t, "t_part") + _, err := f.pool.Exec(t.Context(), + fmt.Sprintf("CREATE TABLE %s.parent (id int) PARTITION BY RANGE (id)", f.schema)) + require.NoError(t, err) + + ds := desired(t, "CREATE TABLE t_part PARTITION OF parent FOR VALUES FROM (1) TO (10)") + _, err = executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrPartitionOfUnsupported) + assert.Equal(t, executor.CodePartitionOfUnsupported, executor.OutcomeCode(err)) +} + +// A desired schema for one table can never run against a proof minted for +// another: the mismatch is an invariant breach, not a refusal. +func TestExecuteCreateRefusesProofTargetMismatch(t *testing.T) { + f := newCreateFixture(t, "other") + ds := desired(t, "CREATE TABLE t (id int)") + + _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrInvariantViolation) + assert.False(t, relationExists(t, f.pool, f.schema, "t")) +} + +// Create steps run with search_path pinned to the proof's schema then +// public — the same policy the introspection read path sets — so a +// desired file'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 steps would run under the session default and +// the target schema's type would be invisible (SQLSTATE 42704). +func TestExecuteCreateResolvesTypesInTargetSchema(t *testing.T) { + f := newCreateFixture(t, "t") + + // The type lives in the target schema and, under a unique name, in + // public too — resolution must pick the target schema's copy. + typeName := f.schema + "_mood" + _, err := f.pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TYPE %s.%s AS ENUM ('happy', 'sad')", f.schema, typeName)) + require.NoError(t, err) + _, err = f.pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TYPE public.%s AS ENUM ('decoy')", typeName)) + require.NoError(t, err) + t.Cleanup(func() { + _, err := f.pool.Exec(context.WithoutCancel(t.Context()), + fmt.Sprintf("DROP TYPE IF EXISTS public.%s", typeName)) + assert.NoError(t, err) + }) + + ds := desired(t, fmt.Sprintf("CREATE TABLE t (id int, m %s)", typeName)) + _, err = executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.NoError(t, err) + + var udtSchema string + require.NoError(t, f.pool.QueryRow(t.Context(), + `SELECT udt_schema FROM information_schema.columns + WHERE table_schema = $1 AND table_name = 't' AND column_name = 'm'`, + f.schema).Scan(&udtSchema)) + assert.Equal(t, f.schema, udtSchema, + "the column's type must resolve in the proof's schema, not public") +} + +// An explicit CREATE INDEX whose name is the first choice of an implicit +// constraint index is a decidable conflict: admission refuses the whole +// set before anything runs, rather than letting the server suffix its way +// around one name or fail mid-run after the table committed. +func TestExecuteCreateRefusesImplicitIndexNameCollision(t *testing.T) { + tests := []struct { + name string + sql string + }{ + { + name: "explicit index named after the primary key's index", + sql: `CREATE TABLE t (id int PRIMARY KEY); + CREATE INDEX t_pkey ON t (id);`, + }, + { + name: "explicit index named after a unique constraint's index", + sql: `CREATE TABLE t (a int, b int, UNIQUE (a, b)); + CREATE INDEX t_a_b_key ON t (a);`, + }, + { + name: "explicit index named after a named constraint", + sql: `CREATE TABLE t (id int, CONSTRAINT my_uni UNIQUE (id)); + CREATE INDEX my_uni ON t (id);`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, tt.sql) + + _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrDuplicateCreateName) + assert.Equal(t, executor.CodeDuplicateCreateName, executor.OutcomeCode(err)) + assert.False(t, relationExists(t, f.pool, f.schema, "t"), + "admission covers the whole set before the first step executes") + }) + } +} + +// A zero CreationRole is forgeable by any package: only +// CheckCreatePrivileges mints one with a schema, so the executor refuses +// it as an invariant breach before anything runs. +func TestExecuteCreateRefusesZeroCreationRole(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, "CREATE TABLE t (id int)") + + _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, preflight.CreationRole{}, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrInvariantViolation) + assert.False(t, relationExists(t, f.pool, f.schema, "t")) +} + +// A creation-privilege proof minted for one schema can never authorize a +// run whose absence proof names another: the mismatch is an invariant +// breach, not a refusal. +func TestExecuteCreateRefusesCreationRoleSchemaMismatch(t *testing.T) { + f := newCreateFixture(t, "t") + otherSchema := testutil.NewSchema(t, f.pool) + otherCR, err := preflight.CheckCreatePrivileges(t.Context(), f.pool, otherSchema) + require.NoError(t, err) + + ds := desired(t, "CREATE TABLE t (id int)") + _, err = executor.ExecuteCreate(t.Context(), f.pool, f.at, otherCR, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrInvariantViolation) + assert.False(t, relationExists(t, f.pool, f.schema, "t")) +} + +// Unnamed CREATE INDEX steps claim no name — the server invents one, +// suffixing around occupants — so two of them in one desired set are not +// a duplicate-name conflict. +func TestExecuteCreateAllowsMultipleUnnamedIndexes(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, ` + CREATE TABLE t (a int, b int); + CREATE INDEX ON t (a); + CREATE INDEX ON t (b); + `) + + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.NoError(t, err) + require.Len(t, rep.Steps, 3) + + var indexes int + require.NoError(t, f.pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_indexes WHERE schemaname = $1 AND tablename = 't'`, + f.schema).Scan(&indexes)) + assert.Equal(t, 2, indexes, "both server-named indexes exist") +} diff --git a/pkg/executor/create_test.go b/pkg/executor/create_test.go new file mode 100644 index 0000000..d126264 --- /dev/null +++ b/pkg/executor/create_test.go @@ -0,0 +1,59 @@ +package executor_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/preflight" + "github.com/block/pg-sprite/pkg/statement" +) + +// createBudget is generous for unit tests; admission refusals return +// before any database access. +var createBudget = executor.Budget{LockTimeout: time.Second, StatementTimeout: 2 * time.Second} + +func TestExecuteCreateRejectsUnboundedBudget(t *testing.T) { + ds, err := statement.ParseDesired("CREATE TABLE t (id int)") + require.NoError(t, err) + + _, err = executor.ExecuteCreate(t.Context(), nil, preflight.AbsentTarget{}, preflight.CreationRole{}, ds, + executor.Budget{LockTimeout: 0, StatementTimeout: time.Second}, executor.DefaultRetryPolicy()) + // The zero-value absence proof would also refuse (as an invariant + // violation); asserting on the budget wording proves the budget check + // fired first, since a valid proof needs a live database. + require.ErrorContains(t, err, "lock budget") +} + +// A zero-value AbsentTarget is constructible by any package; only +// CheckTableAbsent mints one with a verified target, so the executor +// refuses the forgery fail-closed. +func TestExecuteCreateRejectsZeroValueAbsenceProof(t *testing.T) { + ds, err := statement.ParseDesired("CREATE TABLE t (id int)") + require.NoError(t, err) + + _, err = executor.ExecuteCreate(t.Context(), nil, preflight.AbsentTarget{}, preflight.CreationRole{}, ds, + createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrInvariantViolation) +} + +// A zero-value DesiredSchema carries no admitted CREATE TABLE; only +// ParseDesired mints one, so the executor refuses the forgery fail-closed. +// The refusal fires even though the absence proof is also zero-valued: the +// absence check runs first and reports the same invariant class. +func TestExecuteCreateRejectsZeroValueDesiredSchema(t *testing.T) { + _, err := executor.ExecuteCreate(t.Context(), nil, preflight.AbsentTarget{}, preflight.CreationRole{}, statement.DesiredSchema{}, + createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrInvariantViolation) +} + +func TestExecuteCreateWithProgressRequiresTracker(t *testing.T) { + ds, err := statement.ParseDesired("CREATE TABLE t (id int)") + require.NoError(t, err) + + _, err = executor.ExecuteCreateWithProgress(t.Context(), nil, preflight.AbsentTarget{}, preflight.CreationRole{}, ds, + createBudget, executor.DefaultRetryPolicy(), nil) + require.ErrorIs(t, err, executor.ErrInvariantViolation) +} diff --git a/pkg/executor/docs_test.go b/pkg/executor/docs_test.go index 8c3bd13..2f359a5 100644 --- a/pkg/executor/docs_test.go +++ b/pkg/executor/docs_test.go @@ -25,3 +25,27 @@ func TestDocNamesEveryStepKind(t *testing.T) { "docs/execution-model.md does not name step kind %q", k) } } + +// Every outcome code automation can branch on must be named in the +// execution model: a Code added to the vocabulary without the doc naming +// it fails here. +func TestDocNamesEveryOutcomeCode(t *testing.T) { + raw, err := os.ReadFile(executionModelDoc) + require.NoError(t, err) + doc := string(raw) + for _, c := range executor.Codes() { + assert.Contains(t, doc, fmt.Sprintf("`%s`", c), + "docs/execution-model.md does not name outcome code %q", c) + } +} + +// The closed set has no duplicates: a code pasted twice would silently +// shadow a missing entry. +func TestCodesAreUnique(t *testing.T) { + seen := make(map[executor.Code]struct{}) + for _, c := range executor.Codes() { + _, dup := seen[c] + assert.False(t, dup, "duplicate outcome code %q", c) + seen[c] = struct{}{} + } +} diff --git a/pkg/executor/native.go b/pkg/executor/native.go index 357f164..782cef8 100644 --- a/pkg/executor/native.go +++ b/pkg/executor/native.go @@ -45,11 +45,12 @@ var ( // identical to the build session's — an unqualified name could resolve // to a different table and turn the verdict into a false clean. ErrUnqualifiedTable = errors.New("concurrent index build must schema-qualify its table") - // ErrIfNotExistsUnsupported is returned for CREATE INDEX CONCURRENTLY - // IF NOT EXISTS. The clause checks only the name: it succeeds as a - // no-op while an invalid or unrelated index owns that name, so the - // executor could report success over an index it cannot vouch for. - ErrIfNotExistsUnsupported = errors.New("CREATE INDEX CONCURRENTLY IF NOT EXISTS is not supported: a name-only no-op cannot prove the existing index is valid or even the requested one") + // ErrIfNotExistsUnsupported is returned for any CREATE ... IF NOT + // EXISTS. The clause checks only the name: it succeeds as a no-op + // while an unrelated relation — or, for a concurrent build, an + // invalid index — owns that name, so an executor could report + // success over a relation it cannot vouch for. + ErrIfNotExistsUnsupported = errors.New("IF NOT EXISTS is not supported: a name-only no-op cannot prove the existing relation is the requested one, or even valid") // ErrPreexistingInvalidIndex is returned when an invalid index with the // requested name already exists in the target schema — on any table. // The executor cannot prove who owns that entry — an in-progress diff --git a/pkg/executor/optimistic.go b/pkg/executor/optimistic.go index 575756a..94cb5f3 100644 --- a/pkg/executor/optimistic.go +++ b/pkg/executor/optimistic.go @@ -22,6 +22,7 @@ import ( "strconv" "time" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" @@ -218,6 +219,17 @@ 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 +// same policy the introspection read path sets — so a statement's +// unqualified references (a column's type, an expression's function) +// resolve exactly as the diff resolved them, never via the session's +// ambient search_path. +func executeBoundedAttempt(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, b Budget, searchPathSchema string) error { tx, err := pool.Begin(ctx) if err != nil { return fmt.Errorf("begin optimistic attempt: %w", err) @@ -233,9 +245,12 @@ func executeNativeAttempt(ctx context.Context, pool *pgxpool.Pool, st statement. // INV: LK-2 — budgets are applied inside this transaction regardless of // the session defaults, so the attempt cannot outlive them even on a // misconfigured pool. A bare integer is milliseconds to PostgreSQL; - // SET LOCAL cannot use bind parameters. + // SET LOCAL cannot use bind parameters, and identifiers are sanitized. setBudgets := "SET LOCAL lock_timeout = " + strconv.FormatInt(b.LockTimeout.Milliseconds(), 10) + "; SET LOCAL statement_timeout = " + strconv.FormatInt(b.StatementTimeout.Milliseconds(), 10) + if searchPathSchema != "" { + setBudgets += "; SET LOCAL search_path = " + pgx.Identifier{searchPathSchema}.Sanitize() + ", public" + } if _, err := tx.Exec(ctx, setBudgets); err != nil { return fmt.Errorf("set attempt budgets: %w", err) } diff --git a/pkg/preflight/create.go b/pkg/preflight/create.go new file mode 100644 index 0000000..ab1f50c --- /dev/null +++ b/pkg/preflight/create.go @@ -0,0 +1,98 @@ +// This file is the create path's privilege check. A greenfield CREATE +// TABLE has no owner to be a member of — the table is born owned by the +// role that creates it — so the check proves the off-ladder TierCreateTable +// facts (CONNECT on the database, USAGE and CREATE on the schema) instead +// of walking the ownership tier ladder in privileges.go, which states +// facts about an existing table. + +package preflight + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// CreationRole proves the connected role holds every access a greenfield +// CREATE TABLE in the schema needs: CONNECT on the database, USAGE and +// CREATE on the schema. It can only be constructed by +// CheckCreatePrivileges in this package. The schema it carries is always +// resolved — an unqualified check records the session's creation schema. +// +// Like AbsentTarget, the proof is time-of-check and session-scoped: a +// grant can be revoked between the check and the CREATE TABLE, in which +// case the create fails with the server's own insufficient-privilege +// error rather than a typed refusal. +type CreationRole struct { + role string + schema string +} + +// Role returns the connected role the checks ran as — the role a created +// table would be owned by. +func (c CreationRole) Role() string { return c.role } + +// Schema returns the resolved schema the access was verified in. +func (c CreationRole) Schema() string { return c.schema } + +// CheckCreatePrivileges verifies the connected role can create a table in +// the schema (the session's creation schema, current_schema(), when schema +// is empty): CONNECT on the database, USAGE and CREATE on the schema. A +// missing grant is a *PrivilegeError naming the exact statement that would +// satisfy it — the grantee is the connected role itself, because a table +// that does not exist yet has no owning role to inherit from. On success +// it returns the CreationRole proof. +func CheckCreatePrivileges(ctx context.Context, pool *pgxpool.Pool, schema string) (CreationRole, error) { + // One catalog snapshot gathers every fact the check consults, so the + // facts cannot disagree about when they looked. The LEFT JOIN turns + // "schema missing" into a false exists column instead of an absent + // row, and COALESCE keeps the privilege probes NULL-safe on that + // branch. + const q = ` + SELECT s.nspname, + n.nspname IS NOT NULL, + current_user::text, + current_database()::text, + has_database_privilege(current_user, current_database(), 'CONNECT'), + COALESCE(has_schema_privilege(current_user, n.nspname, 'USAGE'), false), + COALESCE(has_schema_privilege(current_user, n.nspname, 'CREATE'), false) + FROM (SELECT CASE WHEN $1 = '' THEN current_schema() ELSE $1 END AS nspname) s + LEFT JOIN pg_namespace n ON n.nspname = s.nspname` + var targetSchema *string + var schemaExists, canConnect, schemaUsage, schemaCreate bool + var role, database string + if err := pool.QueryRow(ctx, q, schema).Scan( + &targetSchema, &schemaExists, &role, &database, + &canConnect, &schemaUsage, &schemaCreate); err != nil { + return CreationRole{}, fmt.Errorf("gather create access facts for schema %q: %w", schema, err) + } + if targetSchema == nil { + // Only an unqualified check can land here: current_schema() is + // NULL when the search_path names no usable schema, so there is + // no schema to check creation access in. + return CreationRole{}, fmt.Errorf("resolve creation schema: %w", ErrNoCreationSchema) + } + if !schemaExists { + return CreationRole{}, fmt.Errorf("%w: schema %s does not exist", ErrSchemaNotFound, *targetSchema) + } + // INV: ST-6 — each missing grant is a typed refusal carrying the exact + // provisioning statement; the proof is only minted when every fact + // holds. + if !canConnect { + return CreationRole{}, connectRefusal(role, database) + } + if !schemaUsage { + return CreationRole{}, schemaUsageRefusal(role, *targetSchema) + } + if !schemaCreate { + return CreationRole{}, &PrivilegeError{ + Tier: TierCreateTable, + Check: fmt.Sprintf("has_schema_privilege(%s, %s, 'CREATE')", role, *targetSchema), + Grant: fmt.Sprintf("GRANT CREATE ON SCHEMA %s TO %s", + pgx.Identifier{*targetSchema}.Sanitize(), pgx.Identifier{role}.Sanitize()), + } + } + return CreationRole{role: role, schema: *targetSchema}, nil +} diff --git a/pkg/preflight/create_integration_test.go b/pkg/preflight/create_integration_test.go new file mode 100644 index 0000000..3cb81be --- /dev/null +++ b/pkg/preflight/create_integration_test.go @@ -0,0 +1,104 @@ +package preflight_test + +import ( + "fmt" + "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/preflight" +) + +// The off-ladder create check, walked grant by grant: each missing access +// is a typed refusal naming the exact provisioning statement whose grantee +// is the engine role itself, and applying exactly that statement unlocks +// the next rung. +func TestCheckCreatePrivilegesWalksTheGrants(t *testing.T) { + serverURL := testutil.StartPostgres(t) + admin, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: serverURL}) + require.NoError(t, err) + t.Cleanup(admin.Close) + schema := testutil.NewSchema(t, admin) + + const password = "create-test-password" + role := testutil.NewRole(t, admin, "LOGIN PASSWORD '"+password+"'") + engine := connectAs(t, serverURL, role, password) + ctx := t.Context() + + // No USAGE on the schema yet. + _, err = preflight.CheckCreatePrivileges(ctx, engine, schema) + var privErr *preflight.PrivilegeError + require.ErrorAs(t, err, &privErr) + assert.Equal(t, preflight.TierConnect, privErr.Tier) + assert.Equal(t, fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", + pgx.Identifier{schema}.Sanitize(), pgx.Identifier{role}.Sanitize()), privErr.Grant) + + _, err = admin.Exec(ctx, privErr.Grant) + require.NoError(t, err) + + // USAGE held, CREATE still missing: the off-ladder tier, and the + // grantee is the connected role — no owner exists to inherit from. + _, err = preflight.CheckCreatePrivileges(ctx, engine, schema) + require.ErrorAs(t, err, &privErr) + assert.Equal(t, preflight.TierCreateTable, privErr.Tier) + assert.Equal(t, fmt.Sprintf("GRANT CREATE ON SCHEMA %s TO %s", + pgx.Identifier{schema}.Sanitize(), pgx.Identifier{role}.Sanitize()), privErr.Grant) + assert.Empty(t, privErr.Hint, "the grant speaks for itself: its grantee is the role the check names") + + _, err = admin.Exec(ctx, privErr.Grant) + require.NoError(t, err) + + proof, err := preflight.CheckCreatePrivileges(ctx, engine, schema) + require.NoError(t, err) + assert.Equal(t, role, proof.Role()) + assert.Equal(t, schema, proof.Schema()) +} + +func TestCheckCreatePrivilegesRefusesMissingSchema(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + t.Cleanup(pool.Close) + + _, err = preflight.CheckCreatePrivileges(t.Context(), pool, "no_such_schema") + assert.ErrorIs(t, err, preflight.ErrSchemaNotFound) +} + +// An empty schema resolves the session's creation schema — the schema an +// unqualified CREATE TABLE would land in — and the proof carries it. +func TestCheckCreatePrivilegesResolvesUnqualifiedSchema(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + t.Cleanup(pool.Close) + + var creationSchema string + require.NoError(t, pool.QueryRow(t.Context(), "SELECT current_schema()").Scan(&creationSchema)) + + proof, err := preflight.CheckCreatePrivileges(t.Context(), pool, "") + require.NoError(t, err) + assert.Equal(t, creationSchema, proof.Schema()) +} + +// A session whose search_path names no schema has no creation target for +// an unqualified check; the check fails rather than guessing a schema. +func TestCheckCreatePrivilegesRefusesEmptySearchPath(t *testing.T) { + serverURL := testutil.StartPostgres(t) + admin, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: serverURL}) + require.NoError(t, err) + t.Cleanup(admin.Close) + + const password = "create-test-password" + role := testutil.NewRole(t, admin, "LOGIN PASSWORD '"+password+"'") + _, err = admin.Exec(t.Context(), fmt.Sprintf("ALTER ROLE %s SET search_path = ''", + pgx.Identifier{role}.Sanitize())) + require.NoError(t, err) + + engine := connectAs(t, serverURL, role, password) + _, err = preflight.CheckCreatePrivileges(t.Context(), engine, "") + assert.ErrorIs(t, err, preflight.ErrNoCreationSchema) + assert.NotErrorIs(t, err, preflight.ErrSchemaNotFound, + "an unresolvable search_path is not a missing schema — there is no schema name to report missing") +} diff --git a/pkg/preflight/docs_test.go b/pkg/preflight/docs_test.go index b676067..e5c7e54 100644 --- a/pkg/preflight/docs_test.go +++ b/pkg/preflight/docs_test.go @@ -22,7 +22,7 @@ var proofTypeDocs = []string{ // document: a new proof type added without updating all three lists fails // here. Extend the slice when a new proof type lands. func TestDocsListEveryProofType(t *testing.T) { - proofTypes := []string{"PreflightedTable", "AbsentTarget"} + proofTypes := []string{"PreflightedTable", "AbsentTarget", "CreationRole"} for _, doc := range proofTypeDocs { raw, err := os.ReadFile(doc) require.NoError(t, err) diff --git a/pkg/preflight/privileges.go b/pkg/preflight/privileges.go index 68284ef..3ac8583 100644 --- a/pkg/preflight/privileges.go +++ b/pkg/preflight/privileges.go @@ -40,6 +40,14 @@ const ( // TierCopyAndSwap covers shadow-object creation: membership usable // with SET ROLE, so shadow objects are born with the correct owner. TierCopyAndSwap + // TierCreateTable covers greenfield CREATE TABLE and sits off the + // ladder above: a table that does not exist yet has no owner to be a + // member of, so the create path proves CONNECT on the database plus + // USAGE and CREATE on the schema — deliberately not the ownership + // membership the ALTER tiers require. It is checked by + // CheckCreatePrivileges, never by CheckPrivileges, whose ladder walks + // facts about an existing table. + TierCreateTable ) // String names the tier's capability for refusal messages. @@ -53,6 +61,8 @@ func (t Tier) String() string { return "index builds" case TierCopyAndSwap: return "copy-and-swap" + case TierCreateTable: + return "create a new table" default: return fmt.Sprintf("unknown tier %d", int(t)) } @@ -249,16 +259,33 @@ func unresolvedTargetCause(ctx context.Context, pool *pgxpool.Pool, schema, tabl return fmt.Errorf("resolve schema %s: %w", schema, err) } if !usage { - return &PrivilegeError{ - Tier: TierConnect, - Check: fmt.Sprintf("has_schema_privilege(%s, %s, 'USAGE')", role, schema), - Grant: fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", - pgx.Identifier{schema}.Sanitize(), pgx.Identifier{role}.Sanitize()), - } + return schemaUsageRefusal(role, schema) } return fmt.Errorf("%w: %s", ErrTableNotFound, qualifiedName(schema, table)) } +// connectRefusal is the typed refusal for a role that cannot connect to +// the database, carrying the exact provisioning statement. +func connectRefusal(role, database string) *PrivilegeError { + return &PrivilegeError{ + Tier: TierConnect, + Check: fmt.Sprintf("has_database_privilege(%s, %s, 'CONNECT')", role, database), + Grant: fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", + pgx.Identifier{database}.Sanitize(), pgx.Identifier{role}.Sanitize()), + } +} + +// schemaUsageRefusal is the typed refusal for a role without USAGE on the +// schema, carrying the exact provisioning statement. +func schemaUsageRefusal(role, schema string) *PrivilegeError { + return &PrivilegeError{ + Tier: TierConnect, + Check: fmt.Sprintf("has_schema_privilege(%s, %s, 'USAGE')", role, schema), + Grant: fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", + pgx.Identifier{schema}.Sanitize(), pgx.Identifier{role}.Sanitize()), + } +} + // checkTierLadder walks the contract's tiers bottom-up to the requirement // and returns the first missing access as a typed refusal. Bottom-up order // makes the refusal actionable: the operator fixes the foundational grant @@ -270,20 +297,10 @@ func unresolvedTargetCause(ctx context.Context, pool *pgxpool.Pool, schema, tabl // mid-change server error. func checkTierLadder(ctx context.Context, pool *pgxpool.Pool, f accessFacts, tier Tier) error { if !f.canConnect { - return &PrivilegeError{ - Tier: TierConnect, - Check: fmt.Sprintf("has_database_privilege(%s, %s, 'CONNECT')", f.role, f.database), - Grant: fmt.Sprintf("GRANT CONNECT ON DATABASE %s TO %s", - pgx.Identifier{f.database}.Sanitize(), pgx.Identifier{f.role}.Sanitize()), - } + return connectRefusal(f.role, f.database) } if !f.schemaUsage { - return &PrivilegeError{ - Tier: TierConnect, - Check: fmt.Sprintf("has_schema_privilege(%s, %s, 'USAGE')", f.role, f.schema), - Grant: fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", - pgx.Identifier{f.schema}.Sanitize(), pgx.Identifier{f.role}.Sanitize()), - } + return schemaUsageRefusal(f.role, f.schema) } if tier < TierAlterInPlace { return nil diff --git a/pkg/statement/implicit.go b/pkg/statement/implicit.go new file mode 100644 index 0000000..0957923 --- /dev/null +++ b/pkg/statement/implicit.go @@ -0,0 +1,180 @@ +// This file predicts the index names PostgreSQL invents for a CREATE +// TABLE's index-backed constraints. The create path's admission gate +// claims every relation name a desired set will occupy, and an implicit +// constraint index occupies one just as an explicit CREATE INDEX does — +// a set whose explicit index name collides with a constraint's index +// would otherwise pass admission and fail mid-run after the table +// committed. The prediction mirrors the server's first choice +// (makeObjectName in the PostgreSQL sources): when that first choice is +// already occupied on the server, PostgreSQL appends a numeric suffix +// instead, so a predicted name is where the server *starts*, not a +// guarantee of the final catalog name — exactly the right meaning for a +// duplicate-claim check inside one desired set. + +package statement + +import ( + "errors" + "fmt" + "strings" + "unicode/utf8" + + pganalyze "github.com/pganalyze/pg_query_go/v6" + pgquery "github.com/wasilibs/go-pgquery" +) + +// ErrNotCreateTable is returned when the statement handed to +// ImplicitIndexNames is not a single CREATE TABLE. +var ErrNotCreateTable = errors.New("statement is not a CREATE TABLE") + +// nameDataLen is PostgreSQL's NAMEDATALEN - 1: the byte budget an +// identifier is truncated to. +const nameDataLen = 63 + +// ImplicitIndexNames returns the first-choice index names PostgreSQL will +// use for the index-backed constraints of one CREATE TABLE statement — +// PRIMARY KEY, UNIQUE, and EXCLUDE, in their column-inline and +// table-constraint forms. A named constraint's index takes the constraint +// name verbatim; an unnamed one takes the server's generated name +// (`_pkey`, `
__key`, `
__excl`, truncated +// to the identifier byte budget the way the server truncates). Names are +// returned in definition order and are not de-duplicated: two constraints +// whose first choices coincide both appear, so a claim map sees the +// conflict. +func ImplicitIndexNames(sql string) ([]string, error) { + tree, err := pgquery.Parse(sql) + if err != nil { + return nil, fmt.Errorf("parse statement: %w", err) + } + if n := len(tree.GetStmts()); n != 1 { + return nil, fmt.Errorf("%w: got %d", ErrNotOneStatement, n) + } + create := tree.GetStmts()[0].GetStmt().GetCreateStmt() + if create == nil { + return nil, ErrNotCreateTable + } + table := create.GetRelation().GetRelname() + var names []string + for _, elt := range create.GetTableElts() { + if con := elt.GetConstraint(); con != nil { + if name, ok := constraintIndexName(table, con); ok { + names = append(names, name) + } + continue + } + col := elt.GetColumnDef() + if col == nil { + continue + } + for _, c := range col.GetConstraints() { + con := c.GetConstraint() + if con == nil { + continue + } + if name, ok := inlineConstraintIndexName(table, col.GetColname(), con); ok { + names = append(names, name) + } + } + } + return names, nil +} + +// constraintIndexName returns the index name a table-level constraint will +// claim, or ok=false when the constraint builds no index. +func constraintIndexName(table string, con *pganalyze.Constraint) (string, bool) { + if name := con.GetConname(); name != "" && constraintBuildsIndex(con) { + return name, true + } + switch con.GetContype() { + case pganalyze.ConstrType_CONSTR_PRIMARY: + return makeObjectName(table, "", "pkey"), true + case pganalyze.ConstrType_CONSTR_UNIQUE: + return makeObjectName(table, strings.Join(constraintKeys(con), "_"), "key"), true + case pganalyze.ConstrType_CONSTR_EXCLUSION: + return makeObjectName(table, strings.Join(exclusionKeys(con), "_"), "excl"), true + default: + return "", false + } +} + +// inlineConstraintIndexName returns the index name a column-inline +// constraint will claim, or ok=false when the constraint builds no index. +// An inline PRIMARY KEY names its index after the table alone, exactly as +// the table-constraint form does; an inline UNIQUE names it after the one +// column it covers. +func inlineConstraintIndexName(table, column string, con *pganalyze.Constraint) (string, bool) { + if name := con.GetConname(); name != "" && constraintBuildsIndex(con) { + return name, true + } + switch con.GetContype() { + case pganalyze.ConstrType_CONSTR_PRIMARY: + return makeObjectName(table, "", "pkey"), true + case pganalyze.ConstrType_CONSTR_UNIQUE: + return makeObjectName(table, column, "key"), true + default: + return "", false + } +} + +// constraintKeys returns the plain key column names of a PRIMARY KEY or +// UNIQUE table constraint. +func constraintKeys(con *pganalyze.Constraint) []string { + keys := make([]string, 0, len(con.GetKeys())) + for _, k := range con.GetKeys() { + keys = append(keys, k.GetString_().GetSval()) + } + return keys +} + +// exclusionKeys returns the name contribution of each EXCLUDE element: the +// column name for a plain column, the literal "expr" for an expression — +// the same substitution the server makes when it builds the name. +func exclusionKeys(con *pganalyze.Constraint) []string { + keys := make([]string, 0, len(con.GetExclusions())) + for _, ex := range con.GetExclusions() { + elem := ex.GetList().GetItems()[0].GetIndexElem() + if name := elem.GetName(); name != "" { + keys = append(keys, name) + continue + } + keys = append(keys, "expr") + } + return keys +} + +// makeObjectName mirrors PostgreSQL's makeObjectName: join name1, an +// optional name2, and the label with underscores, shrinking the longer of +// name1/name2 one byte at a time until the whole fits the identifier byte +// budget, never splitting a multibyte character. +func makeObjectName(name1, name2, label string) string { + overhead := len(label) + 1 + if name2 != "" { + overhead++ + } + avail := nameDataLen - overhead + n1, n2 := len(name1), len(name2) + for n1+n2 > avail { + if n1 > n2 { + n1-- + } else { + n2-- + } + } + name1 = clipToRuneBoundary(name1, n1) + if name2 == "" { + return name1 + "_" + label + } + return name1 + "_" + clipToRuneBoundary(name2, n2) + "_" + label +} + +// clipToRuneBoundary truncates s to at most n bytes, backing off to the +// nearest rune boundary so a multibyte character is never split. +func clipToRuneBoundary(s string, n int) string { + if len(s) <= n { + return s + } + for n > 0 && !utf8.RuneStart(s[n]) { + n-- + } + return s[:n] +} diff --git a/pkg/statement/implicit_integration_test.go b/pkg/statement/implicit_integration_test.go new file mode 100644 index 0000000..722a7a4 --- /dev/null +++ b/pkg/statement/implicit_integration_test.go @@ -0,0 +1,77 @@ +package statement_test + +import ( + "fmt" + "strings" + "testing" + + "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/statement" +) + +// Two-oracle check (TM): for each representative CREATE TABLE, the names +// ImplicitIndexNames predicts must be exactly the index names the real +// server mints when it runs the same statement into an empty schema. The +// prediction is the server's first choice, and an empty schema guarantees +// the first choice is what the catalog records. +func TestImplicitIndexNamesMatchServer(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + t.Cleanup(pool.Close) + + longTable := strings.Repeat("a", 60) + tests := []struct { + name string + sql string + }{ + {name: "unnamed inline primary key", sql: "CREATE TABLE t (id int PRIMARY KEY)"}, + {name: "unnamed table-constraint primary key", sql: "CREATE TABLE t (id int, PRIMARY KEY (id))"}, + {name: "multi-column unique table constraint", sql: "CREATE TABLE t (a int, b int, UNIQUE (a, b))"}, + {name: "inline unique column", sql: "CREATE TABLE t (id int, email text UNIQUE)"}, + {name: "named unique constraint", sql: "CREATE TABLE t (id int, CONSTRAINT my_uni UNIQUE (id))"}, + {name: "primary key and unique together", sql: "CREATE TABLE t (id int PRIMARY KEY, a int, b int, UNIQUE (a, b))"}, + {name: "long table name truncates the generated name", sql: fmt.Sprintf("CREATE TABLE %s (id int PRIMARY KEY)", longTable)}, + {name: "btree exclusion constraint", sql: "CREATE TABLE t (c int, EXCLUDE USING btree (c WITH =))"}, + {name: "exclusion constraint over an expression", sql: "CREATE TABLE t (c int, EXCLUDE USING btree ((c + 1) WITH =))"}, + {name: "no index-backed constraints", sql: "CREATE TABLE t (id int, note text)"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + predicted, err := statement.ImplicitIndexNames(tt.sql) + require.NoError(t, err) + + schema := testutil.NewSchema(t, pool) + tx, err := pool.Begin(t.Context()) + require.NoError(t, err) + defer func() { assert.NoError(t, tx.Commit(t.Context())) }() + _, err = tx.Exec(t.Context(), "SET LOCAL search_path = "+schema) + require.NoError(t, err) + _, err = tx.Exec(t.Context(), tt.sql) + require.NoError(t, err) + + rows, err := tx.Query(t.Context(), + `SELECT ic.relname + FROM pg_index i + JOIN pg_class c ON c.oid = i.indrelid + JOIN pg_class ic ON ic.oid = i.indexrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 + ORDER BY ic.oid`, schema) + require.NoError(t, err) + var actual []string + for rows.Next() { + var name string + require.NoError(t, rows.Scan(&name)) + actual = append(actual, name) + } + require.NoError(t, rows.Err()) + + assert.ElementsMatch(t, predicted, actual, + "predicted first-choice names must match the names the server minted") + }) + } +} diff --git a/pkg/statement/implicit_test.go b/pkg/statement/implicit_test.go new file mode 100644 index 0000000..e42fc31 --- /dev/null +++ b/pkg/statement/implicit_test.go @@ -0,0 +1,118 @@ +package statement_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/statement" +) + +func TestImplicitIndexNames(t *testing.T) { + tests := []struct { + name string + sql string + want []string + }{ + { + name: "no index-backed constraints", + sql: "CREATE TABLE t (id int, name text, CHECK (id > 0))", + want: nil, + }, + { + name: "inline primary key", + sql: "CREATE TABLE t (id int PRIMARY KEY)", + want: []string{"t_pkey"}, + }, + { + name: "table primary key", + sql: "CREATE TABLE t (id int, PRIMARY KEY (id))", + want: []string{"t_pkey"}, + }, + { + name: "named table primary key", + sql: "CREATE TABLE t (id int, CONSTRAINT my_pk PRIMARY KEY (id))", + want: []string{"my_pk"}, + }, + { + name: "inline unique", + sql: "CREATE TABLE t (email text UNIQUE)", + want: []string{"t_email_key"}, + }, + { + name: "named inline unique", + sql: "CREATE TABLE t (email text CONSTRAINT email_uq UNIQUE)", + want: []string{"email_uq"}, + }, + { + name: "multi-column table unique", + sql: "CREATE TABLE t (a int, b int, UNIQUE (a, b))", + want: []string{"t_a_b_key"}, + }, + { + name: "exclude on a plain column", + sql: "CREATE TABLE t (id int, EXCLUDE USING btree (id WITH =))", + want: []string{"t_id_excl"}, + }, + { + name: "exclude on an expression", + sql: "CREATE TABLE t (id int, EXCLUDE USING btree ((id * 2) WITH =))", + want: []string{"t_expr_excl"}, + }, + { + name: "mixed constraints in definition order", + sql: "CREATE TABLE t (id int PRIMARY KEY, email text UNIQUE, a int, b int, CONSTRAINT ab_uq UNIQUE (a, b))", + want: []string{"t_pkey", "t_email_key", "ab_uq"}, + }, + { + name: "qualified table uses the bare relation name", + sql: `CREATE TABLE "s"."t" (id int PRIMARY KEY)`, + want: []string{"t_pkey"}, + }, + { + name: "identical first choices are both returned", + sql: "CREATE TABLE t (a int, UNIQUE (a), CONSTRAINT t_a_key UNIQUE (a))", + want: []string{"t_a_key", "t_a_key"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := statement.ImplicitIndexNames(tt.sql) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// The generated name is truncated to the identifier byte budget the way +// the server truncates: the longer of table and column contributions +// shrinks first, and the label always survives whole. +func TestImplicitIndexNamesTruncatesLikeTheServer(t *testing.T) { + table := strings.Repeat("t", 70) + got, err := statement.ImplicitIndexNames("CREATE TABLE " + table + " (id int PRIMARY KEY)") + require.NoError(t, err) + require.Len(t, got, 1) + // NAMEDATALEN-1 = 63: 58 bytes of table + "_pkey". + assert.Equal(t, strings.Repeat("t", 58)+"_pkey", got[0]) + assert.LessOrEqual(t, len(got[0]), 63) + + column := strings.Repeat("c", 70) + got, err = statement.ImplicitIndexNames("CREATE TABLE t (" + column + " int UNIQUE)") + require.NoError(t, err) + require.Len(t, got, 1) + // 63 - len("_key") - len("t_") = 57 bytes of column survive. + assert.Equal(t, "t_"+strings.Repeat("c", 57)+"_key", got[0]) + assert.LessOrEqual(t, len(got[0]), 63) +} + +func TestImplicitIndexNamesRefusesNonCreateTable(t *testing.T) { + _, err := statement.ImplicitIndexNames("CREATE INDEX i ON t (id)") + require.ErrorIs(t, err, statement.ErrNotCreateTable) +} + +func TestImplicitIndexNamesRefusesMultipleStatements(t *testing.T) { + _, err := statement.ImplicitIndexNames("CREATE TABLE t (id int); CREATE TABLE u (id int)") + require.ErrorIs(t, err, statement.ErrNotOneStatement) +} diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index b18bcca..38b3d9b 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -98,7 +98,8 @@ type Op struct { Concurrent bool // Unique is true for CREATE UNIQUE INDEX. Unique bool - // IfNotExists is true for CREATE INDEX IF NOT EXISTS. + // IfNotExists is true for CREATE TABLE IF NOT EXISTS and + // CREATE INDEX IF NOT EXISTS. IfNotExists bool // GeneratedStored is true for ADD COLUMN ... GENERATED ... STORED. GeneratedStored bool @@ -111,6 +112,19 @@ type Op struct { // PartitionOf is true for CREATE TABLE ... PARTITION OF, which locks // the partitioned parent, not just the new relation. PartitionOf bool + // Inherits is true for CREATE TABLE ... INHERITS, which locks each + // named parent — an existing relation, resolved via search_path when + // unqualified. Disjoint from PartitionOf: the grammar carries the + // partitioned parent in the same clause, but only PARTITION OF sets a + // partition bound. + Inherits bool + // Like is true for a CREATE TABLE with a LIKE clause, which reads an + // existing source table — resolved via search_path when unqualified. + Like bool + // OfType is true for CREATE TABLE ... OF type, which binds the table + // to an existing composite type — resolved via search_path when + // unqualified. + OfType bool // Default is the DEFAULT shape for OpAddColumn. Default DefaultKind // NewType is the target type for OpAlterColumnType and the column type @@ -185,6 +199,17 @@ func (o Op) Describe() string { } } +// hasLikeClause reports whether any table element is a LIKE clause, which +// copies column definitions from an existing source table. +func hasLikeClause(create *pganalyze.CreateStmt) bool { + for _, elt := range create.GetTableElts() { + if elt.GetTableLikeClause() != nil { + return true + } + } + return false +} + // dropIndexNames renders the dropped index names for the operation label: // each object's qualified name, comma-separated when one statement drops // several. The name identifies which structure the plan discards, so a @@ -242,9 +267,14 @@ func ParseOps(sql string) ([]Op, error) { } return ops, nil case node.GetCreateStmt() != nil: + create := node.GetCreateStmt() return []Op{{ Kind: OpCreateTable, - PartitionOf: node.GetCreateStmt().GetPartbound() != nil, + PartitionOf: create.GetPartbound() != nil, + Inherits: create.GetPartbound() == nil && len(create.GetInhRelations()) > 0, + Like: hasLikeClause(create), + OfType: create.GetOfTypename() != nil, + IfNotExists: create.GetIfNotExists(), }}, nil case node.GetIndexStmt() != nil: idx := node.GetIndexStmt() diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go index 2138c41..b995be0 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -255,6 +255,31 @@ func TestParseOpsShapes(t *testing.T) { sql: "CREATE TABLE t (id int PRIMARY KEY)", want: statement.Op{Kind: statement.OpCreateTable}, }, + { + name: "create table if not exists", + sql: "CREATE TABLE IF NOT EXISTS t (id int)", + want: statement.Op{Kind: statement.OpCreateTable, IfNotExists: true}, + }, + { + name: "create table partition of", + sql: "CREATE TABLE t PARTITION OF parent FOR VALUES FROM (1) TO (10)", + want: statement.Op{Kind: statement.OpCreateTable, PartitionOf: true}, + }, + { + name: "create table inherits", + sql: "CREATE TABLE t (id int) INHERITS (parent)", + want: statement.Op{Kind: statement.OpCreateTable, Inherits: true}, + }, + { + name: "create table like", + sql: "CREATE TABLE t (LIKE src INCLUDING ALL)", + want: statement.Op{Kind: statement.OpCreateTable, Like: true}, + }, + { + name: "create table of type", + sql: "CREATE TABLE t OF ty", + want: statement.Op{Kind: statement.OpCreateTable, OfType: true}, + }, { name: "unrecognized statement", sql: "VACUUM FULL t", From ab8ca3a32dce5747eeb43635d82717c109610140 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 28 Aug 2026 19:16:37 +1000 Subject: [PATCH 2/4] 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 3/4] 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 4/4] 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