From 7020e6517bbdeba521609ebd26495dd0c2ef34e5 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 28 Aug 2026 17:45:19 +1000 Subject: [PATCH 1/3] =?UTF-8?q?feat(executor):=20ExecuteCreate=20=E2=80=94?= =?UTF-8?q?=20the=20create=20path=20for=20a=20verified-absent=20target?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runs a validated desired schema (CREATE TABLE plus indexes, table always first) against an AbsentTarget proof: every statement is qualified into the proof's schema, re-parsed, and admitted by shape and target before the first step executes (ST-7). Indexes build plainly, not CONCURRENTLY — the table is born this run with no traffic, and a plain build cannot leave an INVALID index. SQLSTATE 42P07 maps to the typed ErrCreateCollision so the caller re-diffs rather than assumes. Adds the off-ladder CheckCreatePrivileges/CreationRole proof: a greenfield table has no owner to be a member of, so the create path proves CONNECT + schema USAGE + CREATE, deliberately not the ownership tier ladder. Dormant until the declarative front door routes to it. --- SAFETY.md | 4 +- docs/engine-role.md | 9 + docs/invariants.md | 5 +- pkg/executor/code.go | 20 +- pkg/executor/code_test.go | 3 + pkg/executor/create.go | 232 +++++++++++++++++++++++ pkg/executor/create_integration_test.go | 221 +++++++++++++++++++++ pkg/executor/create_test.go | 56 ++++++ pkg/executor/native.go | 11 +- pkg/preflight/create.go | 108 +++++++++++ pkg/preflight/create_integration_test.go | 104 ++++++++++ pkg/preflight/privileges.go | 10 + pkg/statement/ops.go | 4 +- pkg/statement/ops_test.go | 5 + 14 files changed, 780 insertions(+), 12 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 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/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/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/pkg/executor/code.go b/pkg/executor/code.go index 7780812..23b1c11 100644 --- a/pkg/executor/code.go +++ b/pkg/executor/code.go @@ -53,9 +53,19 @@ 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: the create path's target name was taken + // between the absence check and execution; the caller re-diffs the + // live catalog rather than assuming the occupant's shape. + CodeCreateCollision Code = "create-collision" + // 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" @@ -119,6 +129,12 @@ 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, 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..c562311 100644 --- a/pkg/executor/code_test.go +++ b/pkg/executor/code_test.go @@ -52,6 +52,9 @@ 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: "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..9feb516 --- /dev/null +++ b/pkg/executor/create.go @@ -0,0 +1,232 @@ +// 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 — the absence proof is time-of-check, and a + // concurrent create won the race between the check and this run. The + // caller re-diffs the live catalog; nothing about the occupant's shape + // can be assumed. + ErrCreateCollision = errors.New("a relation already exists at a name the create path verified absent") + // 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") +) + +// sqlstateDuplicateTable is raised when a CREATE's target name is already +// taken — for any relation kind, an index included. Postgres errors are +// matched by SQLSTATE, never by message text. +const sqlstateDuplicateTable = "42P07" + +// 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. The pool must come from +// pkg/dbconn. 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, ds statement.DesiredSchema, b Budget, retry RetryPolicy) (SequenceReport, error) { + return executeCreate(ctx, pool, at, 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, 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, ds, b, retry, tracker) +} + +func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentTarget, 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, and only ParseDesired mints a + // DesiredSchema with one. + if at.Schema() == "" || at.Table() == "" { + return rep, fmt.Errorf("%w: ST-7: absence proof carries no verified target", ErrInvariantViolation) + } + 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 executeNativeAttempt(ctx, pool, step, b) + }, 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. +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)) + for i, raw := range desired { + st, err := admitCreateStep(at, raw.SQL()) + if err != nil { + return nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) + } + 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. +func admitCreateStep(at preflight.AbsentTarget, sql string) (statement.Statement, error) { + qualified, err := statement.Qualify(sql, at.Schema()) + if err != nil { + return statement.Statement{}, err + } + st, err := statement.ParseOne(qualified) + if err != nil { + return statement.Statement{}, err + } + ops, err := statement.ParseOps(qualified) + if err != nil { + return statement.Statement{}, 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{}, fmt.Errorf("%w: statement carries %d operations", ErrUnsupportedCreateStep, len(ops)) + } + op := ops[0] + switch st.Kind() { + case statement.KindCreateTable: + if op.PartitionOf { + return statement.Statement{}, ErrPartitionOfUnsupported + } + if op.IfNotExists { + return statement.Statement{}, ErrIfNotExistsUnsupported + } + case statement.KindCreateIndex: + if op.Concurrent { + return statement.Statement{}, fmt.Errorf("%w: a concurrent build is refused on a table born this run", ErrUnsupportedCreateStep) + } + if op.IfNotExists { + return statement.Statement{}, ErrIfNotExistsUnsupported + } + default: + return statement.Statement{}, 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{}, 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, nil +} + +// asCreateCollision maps SQLSTATE 42P07 — raised when a CREATE's target +// name is taken, whether by a table or an index — to the typed collision +// refusal. Every other error passes through unchanged. The server's error +// names the occupied relation, so the wrap adds the classification, not +// the identifier. +func asCreateCollision(err error) error { + var pgErr *pgconn.PgError + if !errors.As(err, &pgErr) || pgErr.Code != sqlstateDuplicateTable { + 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..93da44f --- /dev/null +++ b/pkg/executor/create_integration_test.go @@ -0,0 +1,221 @@ +package executor_test + +import ( + "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 +// minted for the named table — the inputs ExecuteCreate requires. +type createFixture struct { + pool *pgxpool.Pool + schema string + at preflight.AbsentTarget +} + +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) + return createFixture{pool: pool, schema: schema, at: at} +} + +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, 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, 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, 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. +func TestExecuteCreateFailedStepKeepsCommittedPrefix(t *testing.T) { + f := newCreateFixture(t, "t") + // Two indexes under one name: the second build fails with the + // duplicate-name SQLSTATE after the table and first index committed. + ds := desired(t, ` + CREATE TABLE t (id int, name text); + CREATE INDEX dup_idx ON t (id); + CREATE INDEX dup_idx ON t (name); + `) + + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, 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) + assert.ErrorIs(t, err, executor.ErrCreateCollision) + + assert.True(t, relationExists(t, f.pool, f.schema, "t")) + assert.True(t, relationExists(t, f.pool, f.schema, "dup_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) +} + +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, + }, + } + 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, 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, 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, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrInvariantViolation) + assert.False(t, relationExists(t, f.pool, f.schema, "t")) +} diff --git a/pkg/executor/create_test.go b/pkg/executor/create_test.go new file mode 100644 index 0000000..9f025d8 --- /dev/null +++ b/pkg/executor/create_test.go @@ -0,0 +1,56 @@ +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{}, ds, + executor.Budget{LockTimeout: 0, StatementTimeout: time.Second}, executor.DefaultRetryPolicy()) + require.Error(t, err) +} + +// 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{}, 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{}, 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{}, ds, + createBudget, executor.DefaultRetryPolicy(), nil) + require.ErrorIs(t, err, executor.ErrInvariantViolation) +} 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/preflight/create.go b/pkg/preflight/create.go new file mode 100644 index 0000000..d13bfe6 --- /dev/null +++ b/pkg/preflight/create.go @@ -0,0 +1,108 @@ +// 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{}, &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()), + } + } + if !schemaUsage { + return CreationRole{}, &PrivilegeError{ + Tier: TierConnect, + Check: fmt.Sprintf("has_schema_privilege(%s, %s, 'USAGE')", role, *targetSchema), + Grant: fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", + pgx.Identifier{*targetSchema}.Sanitize(), pgx.Identifier{role}.Sanitize()), + } + } + 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/privileges.go b/pkg/preflight/privileges.go index 68284ef..a9958f3 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)) } diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index b18bcca..6b4e9e6 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 @@ -245,6 +246,7 @@ func ParseOps(sql string) ([]Op, error) { return []Op{{ Kind: OpCreateTable, PartitionOf: node.GetCreateStmt().GetPartbound() != nil, + IfNotExists: node.GetCreateStmt().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..f62989d 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -255,6 +255,11 @@ 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: "unrecognized statement", sql: "VACUUM FULL t", From e95a7550dc1154bb2905f7226f058e8e3e70c4e9 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 28 Aug 2026 18:45:18 +1000 Subject: [PATCH 2/3] executor: harden create-path admission and collision mapping Qualify rewrites only the target name, so INHERITS/LIKE/OF bind their secondary relation or type via search_path to existing objects the absence proof does not cover - refuse them at admission. Also refuse in-set duplicate names before anything runs (decidable at admission, previously a mid-run failure with a committed prefix), and map SQLSTATE 42710 - a standalone type holding the table's name - to the typed collision alongside 42P07. --- .agents/checks/review.md | 2 +- README.md | 3 + docs/capabilities.md | 2 +- docs/limitations.md | 1 + docs/tcb-model.md | 1 + pkg/executor/code.go | 12 ++- pkg/executor/code_test.go | 1 + pkg/executor/create.go | 106 +++++++++++++++++------- pkg/executor/create_integration_test.go | 82 ++++++++++++++++-- pkg/executor/create_test.go | 5 +- pkg/preflight/create.go | 14 +--- pkg/preflight/docs_test.go | 2 +- pkg/preflight/privileges.go | 43 ++++++---- pkg/statement/ops.go | 32 ++++++- pkg/statement/ops_test.go | 20 +++++ 15 files changed, 250 insertions(+), 76 deletions(-) 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/docs/capabilities.md b/docs/capabilities.md index f12d86d..d60cdf4 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`, `IF NOT EXISTS`, and `CONCURRENTLY` are typed refusals at admission) 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/limitations.md b/docs/limitations.md index 92cae4e..9bd2d2f 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`, `OF`, and `REFERENCES` (refused upstream at desired-file parse), plus `IF NOT EXISTS` and `CONCURRENTLY`. | | 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/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 23b1c11..c0a5f77 100644 --- a/pkg/executor/code.go +++ b/pkg/executor/code.go @@ -56,10 +56,14 @@ const ( // CodeIfNotExistsUnsupported: CREATE ... IF NOT EXISTS cannot prove // what its no-op would mean. CodeIfNotExistsUnsupported Code = "if-not-exists-unsupported" - // CodeCreateCollision: the create path's target name was taken - // between the absence check and execution; the caller re-diffs the - // live catalog rather than assuming the occupant's shape. + // 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" @@ -131,6 +135,8 @@ func sentinelCode(err error) Code { 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): diff --git a/pkg/executor/code_test.go b/pkg/executor/code_test.go index c562311..84745ea 100644 --- a/pkg/executor/code_test.go +++ b/pkg/executor/code_test.go @@ -53,6 +53,7 @@ func TestOutcomeCodeMapsTypedOutcomes(t *testing.T) { {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}, diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 9feb516..09b27d1 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -40,11 +40,18 @@ import ( // cannot finish is never started. var ( // ErrCreateCollision is returned when a step fails because its target - // name is already taken — the absence proof is time-of-check, and a - // concurrent create won the race between the check and this run. The - // caller re-diffs the live catalog; nothing about the occupant's shape - // can be assumed. - ErrCreateCollision = errors.New("a relation already exists at a name the create path verified absent") + // 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. @@ -57,10 +64,17 @@ var ( ErrUnsupportedCreateStep = errors.New("statement is not a shape the create path can run") ) -// sqlstateDuplicateTable is raised when a CREATE's target name is already -// taken — for any relation kind, an index included. Postgres errors are -// matched by SQLSTATE, never by message text. -const sqlstateDuplicateTable = "42P07" +// 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 @@ -144,17 +158,28 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // 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. +// input order after it. Every step claims a name in the same pg_class +// namespace, so a name claimed twice within the set — decidable here — +// is refused before anything runs rather than failing mid-run after a +// prefix committed. A step whose name the server invents (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, err := admitCreateStep(at, raw.SQL()) + st, name, err := admitCreateStep(at, raw.SQL()) if err != nil { return nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) } + if name != "" { + 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 @@ -171,61 +196,82 @@ func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([] } // admitCreateStep qualifies one desired statement into the proof's schema, -// re-parses it by the real grammar, and admits it by shape and target. -func admitCreateStep(at preflight.AbsentTarget, sql string) (statement.Statement, error) { +// re-parses it by the real grammar, and admits it by shape and target. It +// returns the pg_class name the step will claim — the table name, or the +// index name, empty 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{}, err + return statement.Statement{}, "", err } st, err := statement.ParseOne(qualified) if err != nil { - return statement.Statement{}, err + return statement.Statement{}, "", err } ops, err := statement.ParseOps(qualified) if err != nil { - return statement.Statement{}, err + return statement.Statement{}, "", 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{}, fmt.Errorf("%w: statement carries %d operations", ErrUnsupportedCreateStep, len(ops)) + return statement.Statement{}, "", 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{}, ErrPartitionOfUnsupported + return statement.Statement{}, "", ErrPartitionOfUnsupported + } + if op.Inherits { + return statement.Statement{}, "", fmt.Errorf("%w: INHERITS binds to an existing parent the absence proof does not cover", ErrUnsupportedCreateStep) + } + if op.Like { + return statement.Statement{}, "", fmt.Errorf("%w: LIKE reads an existing source table the absence proof does not cover", ErrUnsupportedCreateStep) + } + if op.OfType { + return statement.Statement{}, "", fmt.Errorf("%w: OF binds to an existing composite type the absence proof does not cover", ErrUnsupportedCreateStep) } if op.IfNotExists { - return statement.Statement{}, ErrIfNotExistsUnsupported + return statement.Statement{}, "", ErrIfNotExistsUnsupported } + claims = st.Table() case statement.KindCreateIndex: if op.Concurrent { - return statement.Statement{}, fmt.Errorf("%w: a concurrent build is refused on a table born this run", ErrUnsupportedCreateStep) + return statement.Statement{}, "", fmt.Errorf("%w: a concurrent build is refused on a table born this run", ErrUnsupportedCreateStep) } if op.IfNotExists { - return statement.Statement{}, ErrIfNotExistsUnsupported + return statement.Statement{}, "", ErrIfNotExistsUnsupported } + claims = op.Name default: - return statement.Statement{}, fmt.Errorf("%w: kind %q", ErrUnsupportedCreateStep, st.Kind()) + return statement.Statement{}, "", 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{}, fmt.Errorf("%w: ST-7: statement targets %q but absence was verified for %q", + return statement.Statement{}, "", 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, nil + return st, claims, nil } -// asCreateCollision maps SQLSTATE 42P07 — raised when a CREATE's target -// name is taken, whether by a table or an index — to the typed collision -// refusal. Every other error passes through unchanged. The server's error -// names the occupied relation, so the wrap adds the classification, not -// the identifier. +// 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) || pgErr.Code != sqlstateDuplicateTable { + 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 index 93da44f..f68158d 100644 --- a/pkg/executor/create_integration_test.go +++ b/pkg/executor/create_integration_test.go @@ -137,15 +137,15 @@ func TestExecuteCreateReportsCollisionAsTyped(t *testing.T) { // 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. +// 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") - // Two indexes under one name: the second build fails with the - // duplicate-name SQLSTATE after the table and first index committed. ds := desired(t, ` CREATE TABLE t (id int, name text); - CREATE INDEX dup_idx ON t (id); - CREATE INDEX dup_idx ON t (name); + 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, ds, createBudget, executor.DefaultRetryPolicy()) @@ -155,10 +155,12 @@ func TestExecuteCreateFailedStepKeepsCommittedPrefix(t *testing.T) { require.ErrorAs(t, err, &stepErr) assert.Equal(t, 3, stepErr.Step) assert.Equal(t, 3, stepErr.Total) - assert.ErrorIs(t, err, executor.ErrCreateCollision) + // 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, "dup_idx")) + 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 @@ -167,6 +169,54 @@ func TestExecuteCreateFailedStepKeepsCommittedPrefix(t *testing.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, 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, 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 @@ -183,6 +233,24 @@ func TestExecuteCreateAdmissionRefusals(t *testing.T) { 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) { diff --git a/pkg/executor/create_test.go b/pkg/executor/create_test.go index 9f025d8..9a5bb67 100644 --- a/pkg/executor/create_test.go +++ b/pkg/executor/create_test.go @@ -21,7 +21,10 @@ func TestExecuteCreateRejectsUnboundedBudget(t *testing.T) { _, err = executor.ExecuteCreate(t.Context(), nil, preflight.AbsentTarget{}, ds, executor.Budget{LockTimeout: 0, StatementTimeout: time.Second}, executor.DefaultRetryPolicy()) - require.Error(t, err) + // 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 diff --git a/pkg/preflight/create.go b/pkg/preflight/create.go index d13bfe6..ab1f50c 100644 --- a/pkg/preflight/create.go +++ b/pkg/preflight/create.go @@ -81,20 +81,10 @@ func CheckCreatePrivileges(ctx context.Context, pool *pgxpool.Pool, schema strin // provisioning statement; the proof is only minted when every fact // holds. if !canConnect { - return CreationRole{}, &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()), - } + return CreationRole{}, connectRefusal(role, database) } if !schemaUsage { - return CreationRole{}, &PrivilegeError{ - Tier: TierConnect, - Check: fmt.Sprintf("has_schema_privilege(%s, %s, 'USAGE')", role, *targetSchema), - Grant: fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s", - pgx.Identifier{*targetSchema}.Sanitize(), pgx.Identifier{role}.Sanitize()), - } + return CreationRole{}, schemaUsageRefusal(role, *targetSchema) } if !schemaCreate { return CreationRole{}, &PrivilegeError{ 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 a9958f3..3ac8583 100644 --- a/pkg/preflight/privileges.go +++ b/pkg/preflight/privileges.go @@ -259,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 @@ -280,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/ops.go b/pkg/statement/ops.go index 6b4e9e6..38b3d9b 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -112,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 @@ -186,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 @@ -243,10 +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, - IfNotExists: node.GetCreateStmt().GetIfNotExists(), + 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 f62989d..b995be0 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -260,6 +260,26 @@ func TestParseOpsShapes(t *testing.T) { 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 fafd2765071a7784f9e9c6493dae0415e40533a5 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 28 Aug 2026 20:22:48 +1000 Subject: [PATCH 3/3] executor: address adversarial review of the create path Pin create-step search_path to the proof's schema so unqualified type references resolve as the diff resolved them; seed first-choice implicit constraint-index names into the admission claim map (two-oracle-tested against the server's makeObjectName); consume the CreationRole proof in ExecuteCreate with ST-7 re-verification; pin the unnamed-index exemption; re-attribute the CONCURRENTLY refusal to desired-file parse; document the outcome-code vocabulary (pinned by a docs test via executor.Codes) and the create path's refusal routing for orchestrators. --- docs/capabilities.md | 2 +- docs/execution-model.md | 35 ++++ docs/limitations.md | 2 +- docs/schemabot-integration.md | 40 ++++- pkg/executor/code.go | 30 ++++ pkg/executor/create.go | 122 ++++++++------ pkg/executor/create_integration_test.go | 154 ++++++++++++++++-- pkg/executor/create_test.go | 8 +- pkg/executor/docs_test.go | 24 +++ pkg/executor/optimistic.go | 17 +- pkg/statement/implicit.go | 180 +++++++++++++++++++++ pkg/statement/implicit_integration_test.go | 77 +++++++++ pkg/statement/implicit_test.go | 118 ++++++++++++++ 13 files changed, 736 insertions(+), 73 deletions(-) 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/docs/capabilities.md b/docs/capabilities.md index d60cdf4..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`), the creation-privilege preflight (`CheckCreatePrivileges`), and the executor create path (`ExecuteCreate` — plain `CREATE TABLE` plus plain index builds; `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and `CONCURRENTLY` are typed refusals at admission) 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 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/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/limitations.md b/docs/limitations.md index 9bd2d2f..202e0b1 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`, `OF`, and `REFERENCES` (refused upstream at desired-file parse), plus `IF NOT EXISTS` and `CONCURRENTLY`. | +| 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/pkg/executor/code.go b/pkg/executor/code.go index c0a5f77..1bbe3a7 100644 --- a/pkg/executor/code.go +++ b/pkg/executor/code.go @@ -85,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 diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 09b27d1..7187d64 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -79,31 +79,38 @@ const ( // 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. The pool must come from -// pkg/dbconn. 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, ds statement.DesiredSchema, b Budget, retry RetryPolicy) (SequenceReport, error) { - return executeCreate(ctx, pool, at, ds, b, retry, nil) +// 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, ds statement.DesiredSchema, b Budget, retry RetryPolicy, tracker *progress.Tracker) (rep SequenceReport, err error) { +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, ds, b, retry, tracker) + return executeCreate(ctx, pool, at, cr, ds, b, retry, tracker) } -func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentTarget, ds statement.DesiredSchema, b Budget, retry RetryPolicy, tracker *progress.Tracker) (SequenceReport, error) { +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 @@ -113,11 +120,19 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT } // 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, and only ParseDesired mints a - // DesiredSchema with one. + // 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) } @@ -136,7 +151,7 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT start = tracker.Now() } err := executeWithLockRetryObserved(ctx, retry, func(ctx context.Context) error { - return executeNativeAttempt(ctx, pool, step, b) + return executeBoundedAttempt(ctx, pool, step, b, at.Schema()) }, sleepContext, func(attempt int) { if tracker != nil { tracker.SetAttempt(attempt) @@ -158,10 +173,15 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // 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 a name in the same pg_class -// namespace, so a name claimed twice within the set — decidable here — -// is refused before anything runs rather than failing mid-run after a -// prefix committed. A step whose name the server invents (an unnamed +// 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() @@ -170,11 +190,11 @@ func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([] indexSteps := make([]statement.Statement, 0, len(desired)) claimed := make(map[string]struct{}, len(desired)) for i, raw := range desired { - st, name, err := admitCreateStep(at, raw.SQL()) + 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) } - if name != "" { + 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) } @@ -197,65 +217,75 @@ func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([] // 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 name the step will claim — the table name, or the -// index name, empty 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) { +// 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{}, "", err + return statement.Statement{}, nil, err } st, err := statement.ParseOne(qualified) if err != nil { - return statement.Statement{}, "", err + return statement.Statement{}, nil, err } ops, err := statement.ParseOps(qualified) if err != nil { - return statement.Statement{}, "", err + 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{}, "", fmt.Errorf("%w: statement carries %d operations", ErrUnsupportedCreateStep, len(ops)) + return statement.Statement{}, nil, fmt.Errorf("%w: statement carries %d operations", ErrUnsupportedCreateStep, len(ops)) } op := ops[0] - var claims string + var claims []string switch st.Kind() { case statement.KindCreateTable: if op.PartitionOf { - return statement.Statement{}, "", ErrPartitionOfUnsupported + return statement.Statement{}, nil, ErrPartitionOfUnsupported } if op.Inherits { - return statement.Statement{}, "", fmt.Errorf("%w: INHERITS binds to an existing parent the absence proof does not cover", ErrUnsupportedCreateStep) + 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{}, "", fmt.Errorf("%w: LIKE reads an existing source table the absence proof does not cover", ErrUnsupportedCreateStep) + 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{}, "", fmt.Errorf("%w: OF binds to an existing composite type the absence proof does not cover", ErrUnsupportedCreateStep) + 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{}, "", ErrIfNotExistsUnsupported + return statement.Statement{}, nil, ErrIfNotExistsUnsupported } - claims = st.Table() + 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{}, "", fmt.Errorf("%w: a concurrent build is refused on a table born this run", ErrUnsupportedCreateStep) + 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{}, "", ErrIfNotExistsUnsupported + return statement.Statement{}, nil, ErrIfNotExistsUnsupported + } + if op.Name != "" { + claims = []string{op.Name} } - claims = op.Name default: - return statement.Statement{}, "", fmt.Errorf("%w: kind %q", ErrUnsupportedCreateStep, st.Kind()) + 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{}, "", fmt.Errorf("%w: ST-7: statement targets %q but absence was verified for %q", + 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 diff --git a/pkg/executor/create_integration_test.go b/pkg/executor/create_integration_test.go index f68158d..7fbdb74 100644 --- a/pkg/executor/create_integration_test.go +++ b/pkg/executor/create_integration_test.go @@ -1,6 +1,7 @@ package executor_test import ( + "context" "fmt" "testing" "time" @@ -16,12 +17,14 @@ import ( "github.com/block/pg-sprite/pkg/statement" ) -// createFixture is one schema on a real server with an absence proof -// minted for the named table — the inputs ExecuteCreate requires. +// 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 { @@ -33,7 +36,9 @@ func newCreateFixture(t *testing.T, table string) createFixture { at, err := preflight.CheckTableAbsent(t.Context(), pool, schema, table) require.NoError(t, err) - return createFixture{pool: pool, schema: schema, at: at} + 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 { @@ -82,7 +87,7 @@ func TestExecuteCreateRunsTableAndIndexes(t *testing.T) { CREATE UNIQUE INDEX t_id_name_idx ON t (id, name); `) - rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + 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")) @@ -107,7 +112,7 @@ func TestExecuteCreateOrdersTableBeforeIndexes(t *testing.T) { CREATE TABLE t (id int, name text); `) - rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + 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) @@ -124,7 +129,7 @@ func TestExecuteCreateReportsCollisionAsTyped(t *testing.T) { require.NoError(t, err) ds := desired(t, "CREATE TABLE t (id int)") - rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) require.Error(t, err) var stepErr *executor.SequenceStepError @@ -148,7 +153,7 @@ func TestExecuteCreateFailedStepKeepsCommittedPrefix(t *testing.T) { CREATE INDEX t_missing_idx ON t (missing); `) - rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) require.Error(t, err) var stepErr *executor.SequenceStepError @@ -194,7 +199,7 @@ func TestExecuteCreateRefusesDuplicateNamesAtAdmission(t *testing.T) { f := newCreateFixture(t, "t") ds := desired(t, tt.sql) - _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + _, 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"), @@ -212,7 +217,7 @@ func TestExecuteCreateReportsTypeCollisionAsTyped(t *testing.T) { require.NoError(t, err) ds := desired(t, "CREATE TABLE t (id int)") - _, err = executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + _, 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)) } @@ -257,7 +262,7 @@ func TestExecuteCreateAdmissionRefusals(t *testing.T) { f := newCreateFixture(t, "t") ds := desired(t, tt.sql) - _, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, ds, createBudget, executor.DefaultRetryPolicy()) + _, 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") @@ -272,7 +277,7 @@ func TestExecuteCreateRefusesPartitionOf(t *testing.T) { 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, ds, createBudget, executor.DefaultRetryPolicy()) + _, 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)) } @@ -283,7 +288,132 @@ 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, ds, createBudget, executor.DefaultRetryPolicy()) + _, 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 index 9a5bb67..d126264 100644 --- a/pkg/executor/create_test.go +++ b/pkg/executor/create_test.go @@ -19,7 +19,7 @@ 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{}, ds, + _, 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 @@ -34,7 +34,7 @@ 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{}, ds, + _, err = executor.ExecuteCreate(t.Context(), nil, preflight.AbsentTarget{}, preflight.CreationRole{}, ds, createBudget, executor.DefaultRetryPolicy()) require.ErrorIs(t, err, executor.ErrInvariantViolation) } @@ -44,7 +44,7 @@ func TestExecuteCreateRejectsZeroValueAbsenceProof(t *testing.T) { // 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{}, statement.DesiredSchema{}, + _, err := executor.ExecuteCreate(t.Context(), nil, preflight.AbsentTarget{}, preflight.CreationRole{}, statement.DesiredSchema{}, createBudget, executor.DefaultRetryPolicy()) require.ErrorIs(t, err, executor.ErrInvariantViolation) } @@ -53,7 +53,7 @@ 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{}, ds, + _, 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/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/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) +}