From 85e125a554ceb40d02ce8a21ce6f12a622a57625 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 3 Sep 2026 12:12:06 +1000 Subject: [PATCH 1/3] feat(executor): prove claimed index names free before the create runs An index or constraint-index name the desired set claims that the catalog already holds now refuses the whole set before the CREATE TABLE commits, instead of failing on the index step and leaving the table behind. --- docs/capabilities.md | 2 +- docs/cli-output-examples.md | 2 +- docs/limitations.md | 2 +- docs/schemabot-integration.md | 9 ++++- pkg/executor/create.go | 51 +++++++++++++++--------- pkg/executor/create_integration_test.go | 17 ++++++++ pkg/migrate/desired.go | 10 ++++- pkg/migrate/desired_integration_test.go | 34 ++++++++-------- pkg/preflight/absent.go | 32 +++++++++++++++ pkg/preflight/absent_integration_test.go | 24 +++++++++++ 10 files changed, 141 insertions(+), 42 deletions(-) diff --git a/docs/capabilities.md b/docs/capabilities.md index fc49a2e..edf8415 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -198,7 +198,7 @@ Status legend: โœ… T1 (supported today) ยท ๐ŸŸก T2 (planned; typed refusal today | Unlogged tables | ๐ŸŸก | native, planned flow | Yes | Typed refusal: persistence is not modeled, converging it (`SET LOGGED`) is a full rewrite, and rendering the table as plain `CREATE TABLE` would silently change crash-safety | | Explicit column collations | ๐ŸŸก | native, planned flow | Yes | Typed refusal: dropping a `COLLATE` clause from a rendered baseline silently changes sort order and index semantics; a collation delta cannot converge without a rewrite | | Columns whose default uses a sequence the column does not own | ๐ŸŸก | native, planned flow | Yes | Typed refusal: in a desired-state model that sequence exists only inside the scratch transaction, so no derived plan can reference it. Column-owned (`serial`-style) sequences are fine | -| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | native, as-is | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: the absence preflight (`CheckTableAbsent`) verifies the name is free, `CheckCreatePrivileges` verifies the role can create in the schema, and the executor runs the `CREATE TABLE` and the index builds as brief bounded steps under the engine's `lock_timeout` / `statement_timeout` budgets. An occupied name is a typed `create-collision` refusal; `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | +| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | native, as-is | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every deterministic index/constraint-index name is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | ### Types and non-table objects diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index d04c743..7a8fedc 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -87,7 +87,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `backend-unavailable` | The change routes to an execution strategy this build does not implement (copy-and-swap). | | `destructive-change` | The desired-state plan discards live structure โ€” a dropped column, constraint, index, or `NOT NULL` โ€” and desired-state execution runs no destructive statement; run the drop deliberately instead ([execution model](execution-model.md)). | | `plan-fingerprint-mismatch` | The plan recomputed at execution time does not carry the pinned fingerprint: the plan a reviewer approved is not the plan that would execute, so nothing runs ([execution model](execution-model.md)). | -| `create-collision` | The greenfield create plan's target name is already occupied โ€” a relation or standalone type took it after the plan was derived. Nothing runs; re-derive the plan against the live catalog and review what it says now. | +| `create-collision` | The greenfield create plan's table name or a claimed index/constraint-index name is occupied. Nothing runs; re-derive the plan against the live catalog and review what it says now. Catalog absence checks handle existing occupants; duplicate-name SQLSTATEs remain the race backstop. | ## Migrate diff --git a/docs/limitations.md b/docs/limitations.md index 545bbe1..8f0cd15 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -44,7 +44,7 @@ composition of the model boundaries above with those gates. At a glance: | Desired-file edit | Outcome today | | --- | --- | -| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the name is free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and the index builds as brief bounded steps. An occupied name (a relation or standalone type) is a typed `create-collision` refusal; `PARTITION OF` and `IF NOT EXISTS` are typed refusals before anything runs. | +| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the table name and every deterministic index/constraint-index name are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. An occupied claimed name is a typed `create-collision` refusal before anything runs; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF` and `IF NOT EXISTS` are also typed refusals before execution. | | Add a column | Converges. Runs as a bounded attempt of the submitted form, so the table-size guard applies (below). | | Widen a column type (`varchar(50)` โ†’ `varchar(255)`) | Converges โ€” the same bounded attempt, under the same size guard. | | Add an index | Converges via `CREATE INDEX CONCURRENTLY`. Not size-guarded: long online work on a large table is the pattern's purpose. | diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 48e6403..16cbba6 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -150,7 +150,7 @@ it to one of four things: | Outcome | Routing class | | --- | --- | | Executed | The table and its indexes exist; a rerun converges to an empty plan | -| `create-collision` refusal | **Re-plan**: re-diff the live catalog โ€” something now owns the name; never blindly retry | +| `create-collision` refusal | **Re-plan**: re-diff the live catalog โ€” the table name or a claimed index/constraint-index name is occupied; never blindly retry | | `insufficient-privileges` refusal (`*preflight.PrivilegeError`, `Tier == TierCreateTable`) | **Operator provisioning action**: the role needs the exact `GRANT` the error carries โ€” not a desired-file fix, and not retryable until granted | | Admission refusal (`unsupported-statement`) | **Author action**: the desired file states a shape the create path refuses; retrying unchanged cannot succeed | @@ -201,7 +201,12 @@ them, don't retry them uniformly: | `ErrPartitionOfUnsupported` (`partition-of-unsupported`) | `PARTITION OF` binds to a live parent the absence proof does not cover | Fix the desired file; out of the create path's scope | | `ErrIfNotExistsUnsupported` (`if-not-exists-unsupported`) | `CREATE ... IF NOT EXISTS` succeeds as a name-only no-op over a relation it cannot vouch for โ€” the opposite of the absence proof's fail-closed contract; refused at admission, nothing ran | Fix the desired file: state the plain `CREATE`; the absence check owns collision handling | | `ErrUnsupportedCreateStep` (`unsupported-create-step`) | A desired statement is not a shape the create path can run | Fix the desired file | -| `ErrCreateCollision` (`create-collision`) | A concurrent writer took a needed name after a valid proof | Re-diff the live catalog and re-plan โ€” the world changed; never blindly retry the create | +| `ErrCreateCollision` (`create-collision`) | A claimed index or constraint-index name was already occupied, or a concurrent writer took a needed name after the absence checks | Re-diff the live catalog and re-plan โ€” the world changed; never blindly retry the create | + +Before the first step, `ExecuteCreate` checks all deterministic index and +constraint-index names against `pg_class` in one schema-scoped catalog snapshot; +`CheckTableAbsent` separately covers the table relation and composite type. The +duplicate-name SQLSTATE mapping remains the race backstop after those checks. A failed create is not rolled back wholesale: each step committed in its own bounded transaction, so the steps before the failure remain diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 5e416e7..27cd372 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -9,13 +9,13 @@ // 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 +// The executor proves every deterministic table, index, and constraint-index +// name free before execution. The proof is time-of-check: nothing locks the +// names, so a concurrent create can still take one before its step. That +// loss surfaces through SQLSTATE 42P07 or 42710 as ErrCreateCollision โ€” the +// caller re-diffs 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) remain, so a rerun's absence check refuses with // ErrRelationExists and the declarative front door re-diffs and applies // the remainder. @@ -25,6 +25,7 @@ import ( "context" "errors" "fmt" + "slices" "time" "github.com/jackc/pgx/v5/pgconn" @@ -39,12 +40,11 @@ import ( // desired statement before the first executes, so a creation this executor // cannot finish is never started. var ( - // ErrCreateCollision is returned when a step fails because its target - // name is already taken. For the table name that means a concurrent - // create won the race โ€” the absence proof is time-of-check. Index - // names are never absence-checked, so a pre-existing occupant at an - // index name reports the same way. Either way the caller re-diffs the - // live catalog; nothing about the occupant's shape can be assumed. + // ErrCreateCollision is returned when a claimed name is already taken. + // Admission checks the table's index and constraint-index names before + // execution; duplicate-name SQLSTATEs remain the time-of-check race + // backstop. The caller re-diffs the live catalog without assuming the + // occupant's shape. 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 @@ -140,10 +140,18 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT 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) + steps, claimed, err := admitCreateSteps(at, ds) if err != nil { return rep, err } + // Every deterministic relation name the desired set will claim is + // proved free before the first step executes, so an occupied index name + // refuses the whole set instead of failing after the table committed. + // CheckTableAbsent already covers the table name and its composite type. + claimed = slices.DeleteFunc(claimed, func(name string) bool { return name == at.Table() }) + if err := preflight.CheckNamesAbsent(ctx, pool, at.Schema(), claimed); err != nil { + return rep, fmt.Errorf("%w: the desired file claims a name the catalog already holds: %w", ErrCreateCollision, err) + } for i, step := range steps { start := time.Now() if tracker != nil { @@ -183,30 +191,35 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // because a deterministic name the file states beats one the server // invents. A step whose name the server invents outright (an unnamed // index) claims nothing decidable and is exempt. -func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([]statement.Statement, error) { +func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([]statement.Statement, []string, error) { desired := ds.Statements() // INV: ST-8 โ€” a DesiredSchema proof guarantees a CREATE TABLE ordered // first; a set that does not lead with one means the proof was forged // or mutated. if len(desired) == 0 || desired[0].Kind() != statement.KindCreateTable { - return nil, fmt.Errorf("%w: ST-8: desired schema does not lead with a CREATE TABLE", ErrInvariantViolation) + return nil, nil, fmt.Errorf("%w: ST-8: desired schema does not lead with a CREATE TABLE", ErrInvariantViolation) } steps := make([]statement.Statement, 0, len(desired)) claimed := make(map[string]struct{}, len(desired)) for i, raw := range desired { st, names, err := admitCreateStep(at, raw.SQL()) if err != nil { - return nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) + return nil, nil, fmt.Errorf("desired statement %d of %d: %w", i+1, len(desired), err) } for _, name := range names { if _, taken := claimed[name]; taken { - return nil, fmt.Errorf("desired statement %d of %d: %w: %q", i+1, len(desired), ErrDuplicateCreateName, name) + return nil, nil, fmt.Errorf("desired statement %d of %d: %w: %q", i+1, len(desired), ErrDuplicateCreateName, name) } claimed[name] = struct{}{} } steps = append(steps, st) } - return steps, nil + ordered := make([]string, 0, len(claimed)) + for name := range claimed { + ordered = append(ordered, name) + } + slices.Sort(ordered) + return steps, ordered, nil } // admitCreateStep qualifies one desired statement into the proof's schema, diff --git a/pkg/executor/create_integration_test.go b/pkg/executor/create_integration_test.go index 7fbdb74..5bd98e2 100644 --- a/pkg/executor/create_integration_test.go +++ b/pkg/executor/create_integration_test.go @@ -140,6 +140,23 @@ func TestExecuteCreateReportsCollisionAsTyped(t *testing.T) { assert.Empty(t, rep.Steps) } +// The first-choice name of an index-backed constraint is part of the +// desired set's contract. An unrelated catalog occupant must refuse the +// whole set rather than make PostgreSQL suffix the constraint name. +func TestExecuteCreateRefusesOccupiedImplicitIndexNameBeforeExecution(t *testing.T) { + f := newCreateFixture(t, "t") + _, err := f.pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v int)", f.schema)) + require.NoError(t, err) + _, err = f.pool.Exec(t.Context(), fmt.Sprintf("CREATE INDEX t_pkey ON %s.other (v)", f.schema)) + require.NoError(t, err) + + ds := desired(t, "CREATE TABLE t (id int PRIMARY KEY, v text)") + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrCreateCollision) + assert.Empty(t, rep.Steps) + assert.False(t, relationExists(t, f.pool, f.schema, "t"), "the catalog preflight runs before every step") +} + // A failed step ends the run; the steps before it committed and remain, // and the report covers exactly that prefix so the caller can disclose // what already happened. An index on a column the table does not have diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index 96d6d5a..a4fb5ca 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -257,8 +257,14 @@ func runCreate(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, repo } var stepErr *executor.SequenceStepError if !errors.As(execErr, &stepErr) { - // No step error means nothing started: the executor refused the - // set at admission, from the statements' shapes alone. + // No step error means nothing started: the executor refused the set + // during admission or its claimed-name catalog preflight. + if errors.Is(execErr, executor.ErrCreateCollision) { + result.Outcome = verdict.OutcomeRefused + result.Reason = verdict.ReasonCreateCollision + result.Detail = fmt.Sprintf("the create path refused the plan because the catalog already holds a name it claims (%v); re-derive the plan and review what it says now; nothing was executed", execErr) + return result, nil + } if isCreateAdmissionRefusal(execErr) { result.Outcome = verdict.OutcomeRefused result.Reason = verdict.ReasonUnsupportedStatement diff --git a/pkg/migrate/desired_integration_test.go b/pkg/migrate/desired_integration_test.go index 1a60dfd..ad30a61 100644 --- a/pkg/migrate/desired_integration_test.go +++ b/pkg/migrate/desired_integration_test.go @@ -12,7 +12,6 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/diffplan" - "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/migrate" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" @@ -208,32 +207,35 @@ CREATE INDEX t_v_idx ON t (v);` assert.False(t, exists, "the refused plan must not create the partition") }) - t.Run("a failed index build keeps the created table and discloses the prefix", func(t *testing.T) { - // The desired index's name is already taken by an index on another - // table, so the create path commits the CREATE TABLE and stops on - // the index step โ€” committed-prefix semantics, disclosed by the - // verdicts, with the collision's stable code on the failed one. + t.Run("refuses an occupied constraint-index name and preserves convergence", func(t *testing.T) { schema := testutil.NewSchema(t, pool) - _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v text)", schema)) + _, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v int)", schema)) require.NoError(t, err) - _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE INDEX t_v_idx ON %s.other (v)", schema)) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE INDEX t_pkey ON %s.other (v)", schema)) require.NoError(t, err) res, err := migrate.RunDesired(t.Context(), pool, migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, desiredSQL)}, runOptions()) - require.Error(t, err, "a mid-plan execution failure returns the failed result with the error") - assert.Equal(t, verdict.OutcomeFailed, res.Outcome) - require.Len(t, res.Verdicts, 2, "the committed create and the failed index build") - assert.Equal(t, verdict.OutcomeExecuted, res.Verdicts[0].Outcome) - assert.Contains(t, res.Verdicts[0].Statement, "CREATE TABLE") - assert.Equal(t, verdict.OutcomeFailed, res.Verdicts[1].Outcome) - assert.Equal(t, string(executor.CodeCreateCollision), res.Verdicts[1].Code) + require.NoError(t, err, "a catalog collision is a refusal, not an execution failure") + assert.Equal(t, verdict.OutcomeRefused, res.Outcome) + assert.Equal(t, verdict.ReasonCreateCollision, res.Reason) + assert.Empty(t, res.Verdicts, "no create step ran") var exists bool require.NoError(t, pool.QueryRow(t.Context(), `SELECT EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema = $1 AND table_name = 't')`, schema).Scan(&exists)) - assert.True(t, exists, "the committed CREATE TABLE stays committed") + assert.False(t, exists, "the refused set must not create the table") + + clean := migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, + "CREATE TABLE clean (id int PRIMARY KEY, v text)")} + res, err = migrate.RunDesired(t.Context(), pool, clean, runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, res.Outcome) + res, err = migrate.RunDesired(t.Context(), pool, clean, runOptions()) + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeExecuted, res.Outcome) + assert.Empty(t, res.Plan.Statements, "a clean greenfield create converges on its second run") }) t.Run("refuses a destructive plan and drops nothing", func(t *testing.T) { diff --git a/pkg/preflight/absent.go b/pkg/preflight/absent.go index 4b74dca..604de65 100644 --- a/pkg/preflight/absent.go +++ b/pkg/preflight/absent.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "slices" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" ) @@ -145,3 +147,33 @@ func CheckTableAbsent(ctx context.Context, pool *pgxpool.Pool, schema, table str } return AbsentTarget{schema: *targetSchema, table: table}, nil } + +// CheckNamesAbsent verifies that no relation in schema occupies any of +// names. It reads pg_class in one catalog snapshot and reports the first +// occupied name in lexical order, regardless of input order. It does not +// probe pg_type: this check protects index and constraint-index names, +// which do not create types; [CheckTableAbsent] separately protects the +// CREATE TABLE name and its composite type. +func CheckNamesAbsent(ctx context.Context, pool *pgxpool.Pool, schema string, names []string) error { + if len(names) == 0 { + return nil + } + ordered := slices.Clone(names) + slices.Sort(ordered) + const q = ` + SELECT c.relname, c.relkind::text + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 AND c.relname = ANY($2) + ORDER BY c.relname + LIMIT 1` + var name, relkind string + err := pool.QueryRow(ctx, q, schema, ordered).Scan(&name, &relkind) + if errors.Is(err, pgx.ErrNoRows) { + return nil + } + if err != nil { + return fmt.Errorf("check claimed names are absent in schema %q: %w", schema, err) + } + return fmt.Errorf("%w: %s has relkind %q", ErrRelationExists, qualifiedName(schema, name), relkind) +} diff --git a/pkg/preflight/absent_integration_test.go b/pkg/preflight/absent_integration_test.go index 8def32c..7506c9a 100644 --- a/pkg/preflight/absent_integration_test.go +++ b/pkg/preflight/absent_integration_test.go @@ -25,6 +25,30 @@ func TestCheckTableAbsentProvesFreeName(t *testing.T) { assert.Equal(t, "brand_new", at.Table()) } +func TestCheckNamesAbsent(t *testing.T) { + 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) + otherSchema := testutil.NewSchema(t, pool) + + require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, schema, nil)) + require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, schema, []string{"free_name"})) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v int)", schema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE INDEX t_pkey ON %s.other (v)", schema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t_pkey (v int)", otherSchema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other_schema_only (v int)", otherSchema)) + require.NoError(t, err) + + err = preflight.CheckNamesAbsent(t.Context(), pool, schema, []string{"z_free", "t_pkey"}) + assert.ErrorIs(t, err, preflight.ErrRelationExists) + assert.Contains(t, err.Error(), "t_pkey") + require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, schema, []string{"other_schema_only"})) +} + // An unqualified check resolves the schema an unqualified CREATE TABLE // would land in, so the proof names the exact creation target. func TestCheckTableAbsentResolvesUnqualifiedName(t *testing.T) { From 2fc6a04cad3d8f36b4c79b6972324d5d1c302b03 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 3 Sep 2026 14:39:35 +1000 Subject: [PATCH 2/3] fix(executor): route claimed-name probe failures as operational errors Only an occupied name is a create-collision; a probe that itself fails (cancelled context, dropped connection) proves nothing about the catalog and must not be relabelled a refusal. Also restores the migrate-level committed-prefix coverage the probe made unreachable, states the occupant remedy on the refusal, and narrows the docs to the names the desired file actually states (an unnamed CREATE INDEX claims nothing). --- docs/capabilities.md | 2 +- docs/cli-output-examples.md | 2 +- docs/limitations.md | 2 +- docs/schemabot-integration.md | 24 ++++++---- pkg/executor/create.go | 58 +++++++++++++++++-------- pkg/executor/create_integration_test.go | 20 +++++++++ pkg/migrate/desired.go | 4 +- pkg/migrate/desired_integration_test.go | 32 ++++++++++++++ pkg/preflight/absent.go | 17 ++++---- 9 files changed, 121 insertions(+), 40 deletions(-) diff --git a/docs/capabilities.md b/docs/capabilities.md index edf8415..a02ff36 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -198,7 +198,7 @@ Status legend: โœ… T1 (supported today) ยท ๐ŸŸก T2 (planned; typed refusal today | Unlogged tables | ๐ŸŸก | native, planned flow | Yes | Typed refusal: persistence is not modeled, converging it (`SET LOGGED`) is a full rewrite, and rendering the table as plain `CREATE TABLE` would silently change crash-safety | | Explicit column collations | ๐ŸŸก | native, planned flow | Yes | Typed refusal: dropping a `COLLATE` clause from a rendered baseline silently changes sort order and index semantics; a collation delta cannot converge without a rewrite | | Columns whose default uses a sequence the column does not own | ๐ŸŸก | native, planned flow | Yes | Typed refusal: in a desired-state model that sequence exists only inside the scratch transaction, so no derived plan can reference it. Column-owned (`serial`-style) sequences are fine | -| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | native, as-is | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every deterministic index/constraint-index name is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | +| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | native, as-is | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every index name the desired file states (explicit `CREATE INDEX` names and first-choice constraint-index names; an unnamed `CREATE INDEX ON t (v)` claims nothing) is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution โ€” drop or rename the occupant, or name the constraint's index explicitly; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | ### Types and non-table objects diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 7a8fedc..851f0f2 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -87,7 +87,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `backend-unavailable` | The change routes to an execution strategy this build does not implement (copy-and-swap). | | `destructive-change` | The desired-state plan discards live structure โ€” a dropped column, constraint, index, or `NOT NULL` โ€” and desired-state execution runs no destructive statement; run the drop deliberately instead ([execution model](execution-model.md)). | | `plan-fingerprint-mismatch` | The plan recomputed at execution time does not carry the pinned fingerprint: the plan a reviewer approved is not the plan that would execute, so nothing runs ([execution model](execution-model.md)). | -| `create-collision` | The greenfield create plan's table name or a claimed index/constraint-index name is occupied. Nothing runs; re-derive the plan against the live catalog and review what it says now. Catalog absence checks handle existing occupants; duplicate-name SQLSTATEs remain the race backstop. | +| `create-collision` | The greenfield create plan's table name or a claimed index/constraint-index name is occupied. Nothing runs; re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant or name the constraint's index explicitly in the desired file โ€” re-planning alone reproduces the refusal. Catalog absence checks handle existing occupants; duplicate-name SQLSTATEs remain the race backstop. | ## Migrate diff --git a/docs/limitations.md b/docs/limitations.md index 8f0cd15..41840ad 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -44,7 +44,7 @@ composition of the model boundaries above with those gates. At a glance: | Desired-file edit | Outcome today | | --- | --- | -| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the table name and every deterministic index/constraint-index name are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. An occupied claimed name is a typed `create-collision` refusal before anything runs; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF` and `IF NOT EXISTS` are also typed refusals before execution. | +| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the table name and every index name the desired file states (explicit `CREATE INDEX` names and first-choice constraint-index names; an unnamed `CREATE INDEX ON t (v)` claims nothing) are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. An occupied claimed name is a typed `create-collision` refusal before anything runs โ€” drop or rename the occupant, or name the constraint's index explicitly; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF` and `IF NOT EXISTS` are also typed refusals before execution. | | Add a column | Converges. Runs as a bounded attempt of the submitted form, so the table-size guard applies (below). | | Widen a column type (`varchar(50)` โ†’ `varchar(255)`) | Converges โ€” the same bounded attempt, under the same size guard. | | Add an index | Converges via `CREATE INDEX CONCURRENTLY`. Not size-guarded: long online work on a large table is the pattern's purpose. | diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 16cbba6..9fe0ce8 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -150,7 +150,7 @@ it to one of four things: | Outcome | Routing class | | --- | --- | | Executed | The table and its indexes exist; a rerun converges to an empty plan | -| `create-collision` refusal | **Re-plan**: re-diff the live catalog โ€” the table name or a claimed index/constraint-index name is occupied; never blindly retry | +| `create-collision` refusal | **Re-plan, then fix the occupant**: the table name or a claimed index/constraint-index name is occupied. Re-diff the live catalog to see what holds it; re-planning alone reproduces the refusal โ€” drop or rename the occupant, or name the constraint's index explicitly in the desired file. Never blindly retry | | `insufficient-privileges` refusal (`*preflight.PrivilegeError`, `Tier == TierCreateTable`) | **Operator provisioning action**: the role needs the exact `GRANT` the error carries โ€” not a desired-file fix, and not retryable until granted | | Admission refusal (`unsupported-statement`) | **Author action**: the desired file states a shape the create path refuses; retrying unchanged cannot succeed | @@ -167,14 +167,18 @@ The greenfield `CREATE TABLE` path is a fixed call order, all inside the apply s `CONCURRENTLY`, qualified names). 2. `preflight.CheckTableAbsent` โ€” mint the `AbsentTarget` proof for the table name. 3. `preflight.CheckCreatePrivileges` โ€” mint the `CreationRole` proof for the target schema. -4. `executor.ExecuteCreate` โ€” consume both proofs and run the set. +4. `executor.ExecuteCreate` โ€” consume both proofs, admit the set, probe `pg_class` for every + index and constraint-index name the set claims, then run the set. `migrate.RunDesired` runs this sequence itself when the plan is greenfield โ€” the adapter does not assemble it and must not mint either proof separately (a proof minted outside the -executing session proves nothing about it). The order decides which refusal wins when both -preflights would fail: absence is checked first, so an occupied name refuses as -`create-collision` even when the role also lacks `CREATE` โ€” the collision is the more -actionable message (the change is not a create at all) and absence is the cheaper check. +executing session proves nothing about it). The order decides which refusal wins when more +than one check would fail. The table name is checked first, so an occupied **table** name +refuses as `create-collision` even when the role also lacks `CREATE` โ€” the collision is the +more actionable message (the change is not a create at all) and absence is the cheaper +check. Claimed **index** names are probed last, inside `ExecuteCreate`: an occupied index +name with a role lacking `CREATE` surfaces as `insufficient-privileges`, and the index +collision appears only once the grant is in place. 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 @@ -201,10 +205,12 @@ them, don't retry them uniformly: | `ErrPartitionOfUnsupported` (`partition-of-unsupported`) | `PARTITION OF` binds to a live parent the absence proof does not cover | Fix the desired file; out of the create path's scope | | `ErrIfNotExistsUnsupported` (`if-not-exists-unsupported`) | `CREATE ... IF NOT EXISTS` succeeds as a name-only no-op over a relation it cannot vouch for โ€” the opposite of the absence proof's fail-closed contract; refused at admission, nothing ran | Fix the desired file: state the plain `CREATE`; the absence check owns collision handling | | `ErrUnsupportedCreateStep` (`unsupported-create-step`) | A desired statement is not a shape the create path can run | Fix the desired file | -| `ErrCreateCollision` (`create-collision`) | A claimed index or constraint-index name was already occupied, or a concurrent writer took a needed name after the absence checks | Re-diff the live catalog and re-plan โ€” the world changed; never blindly retry the create | +| `ErrCreateCollision` (`create-collision`) | A claimed index or constraint-index name was already occupied, or a concurrent writer took a needed name after the absence checks | Re-diff the live catalog to see what holds the name, then drop or rename the occupant or name the constraint's index explicitly in the desired file โ€” re-planning alone reproduces the refusal; never blindly retry the create | -Before the first step, `ExecuteCreate` checks all deterministic index and -constraint-index names against `pg_class` in one schema-scoped catalog snapshot; +Before the first step, `ExecuteCreate` probes `pg_class` in one schema-scoped catalog +snapshot for every index name the desired file states โ€” explicit `CREATE INDEX` names and +the first-choice name of each implicit constraint index. An unnamed `CREATE INDEX ON t (v)` +claims nothing: the server invents its name and the probe has nothing to check. `CheckTableAbsent` separately covers the table relation and composite type. The duplicate-name SQLSTATE mapping remains the race backstop after those checks. diff --git a/pkg/executor/create.go b/pkg/executor/create.go index 27cd372..fe83d5d 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -9,8 +9,11 @@ // re-parsed by the real grammar, and admitted by shape and target before // anything executes. // -// The executor proves every deterministic table, index, and constraint-index -// name free before execution. The proof is time-of-check: nothing locks the +// Before the first step the executor probes pg_class for every name the +// desired file states โ€” explicit index names and the first-choice names of +// index-backed constraints โ€” so an occupied name refuses the whole set +// rather than failing after the table committed; the table name itself is +// the caller's absence proof. The proof is time-of-check: nothing locks the // names, so a concurrent create can still take one before its step. That // loss surfaces through SQLSTATE 42P07 or 42710 as ErrCreateCollision โ€” the // caller re-diffs rather than assuming what the collision left behind. A @@ -41,10 +44,13 @@ import ( // cannot finish is never started. var ( // ErrCreateCollision is returned when a claimed name is already taken. - // Admission checks the table's index and constraint-index names before - // execution; duplicate-name SQLSTATEs remain the time-of-check race - // backstop. The caller re-diffs the live catalog without assuming the - // occupant's shape. + // A catalog probe after admission checks the desired file's explicit + // index names and first-choice constraint-index names before the first + // step executes, refusing the whole set with no *SequenceStepError; + // duplicate-name SQLSTATEs remain the time-of-check race backstop and + // arrive wrapped in one. The remedy is on the desired file's side โ€” drop + // or rename the occupant, or name the constraint's index explicitly โ€” + // then re-diff; a re-plan alone reproduces the refusal. 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 @@ -88,13 +94,18 @@ const ( // 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. +// target, and every index and constraint-index name the file states is +// probed free in pg_class, before the first step executes. A refusal from +// either check returns with an empty report and no *SequenceStepError: +// an occupied name is ErrCreateCollision, an inadmissible shape one of the +// admission sentinels, and a probe that could not complete (a cancelled +// context, a lost connection) is that error, wrapped โ€” not a collision. +// 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) } @@ -149,8 +160,16 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // refuses the whole set instead of failing after the table committed. // CheckTableAbsent already covers the table name and its composite type. claimed = slices.DeleteFunc(claimed, func(name string) bool { return name == at.Table() }) - if err := preflight.CheckNamesAbsent(ctx, pool, at.Schema(), claimed); err != nil { - return rep, fmt.Errorf("%w: the desired file claims a name the catalog already holds: %w", ErrCreateCollision, err) + err = preflight.CheckNamesAbsent(ctx, pool, at.Schema(), claimed) + if preflight.IsNameOccupied(err) { + return rep, fmt.Errorf("%w: the desired file claims a name the catalog already holds: %w", + ErrCreateCollision, err) + } + if err != nil { + // The probe itself failed โ€” a cancelled context, a dropped + // connection โ€” which says nothing about whether the names are + // free; it is an operational failure, not a collision. + return rep, fmt.Errorf("verify claimed names are absent in %s: %w", at.Schema(), err) } for i, step := range steps { start := time.Now() @@ -214,12 +233,13 @@ func admitCreateSteps(at preflight.AbsentTarget, ds statement.DesiredSchema) ([] } steps = append(steps, st) } - ordered := make([]string, 0, len(claimed)) + // Order does not matter: the catalog probe is set membership and picks + // the reported occupant itself. + names := make([]string, 0, len(claimed)) for name := range claimed { - ordered = append(ordered, name) + names = append(names, name) } - slices.Sort(ordered) - return steps, ordered, nil + return steps, names, nil } // admitCreateStep qualifies one desired statement into the proof's schema, diff --git a/pkg/executor/create_integration_test.go b/pkg/executor/create_integration_test.go index 5bd98e2..95c5f80 100644 --- a/pkg/executor/create_integration_test.go +++ b/pkg/executor/create_integration_test.go @@ -2,6 +2,7 @@ package executor_test import ( "context" + "errors" "fmt" "testing" "time" @@ -157,6 +158,25 @@ func TestExecuteCreateRefusesOccupiedImplicitIndexNameBeforeExecution(t *testing assert.False(t, relationExists(t, f.pool, f.schema, "t"), "the catalog preflight runs before every step") } +// A claimed-name probe that cannot complete says nothing about whether the +// names are free: it is the caller's operational failure, never a +// collision, so the caller retries rather than being told a free name is +// taken. +func TestExecuteCreateProbeFailureIsNotACollision(t *testing.T) { + f := newCreateFixture(t, "t") + ds := desired(t, "CREATE TABLE t (id int PRIMARY KEY, v text)") + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + rep, err := executor.ExecuteCreate(ctx, f.pool, f.at, f.cr, ds, createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, context.Canceled) + assert.NotErrorIs(t, err, executor.ErrCreateCollision) + var stepErr *executor.SequenceStepError + assert.False(t, errors.As(err, &stepErr), "nothing started, so there is no step to blame") + assert.Empty(t, rep.Steps) + assert.False(t, relationExists(t, f.pool, f.schema, "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. An index on a column the table does not have diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index a4fb5ca..b119407 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -262,7 +262,9 @@ func runCreate(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, repo if errors.Is(execErr, executor.ErrCreateCollision) { result.Outcome = verdict.OutcomeRefused result.Reason = verdict.ReasonCreateCollision - result.Detail = fmt.Sprintf("the create path refused the plan because the catalog already holds a name it claims (%v); re-derive the plan and review what it says now; nothing was executed", execErr) + result.Detail = fmt.Sprintf("the create path refused the plan because the catalog already holds a name it claims (%v); "+ + "re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant "+ + "or name the constraint's index explicitly in the desired file; nothing was executed", execErr) return result, nil } if isCreateAdmissionRefusal(execErr) { diff --git a/pkg/migrate/desired_integration_test.go b/pkg/migrate/desired_integration_test.go index ad30a61..0842f7f 100644 --- a/pkg/migrate/desired_integration_test.go +++ b/pkg/migrate/desired_integration_test.go @@ -12,6 +12,7 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" "github.com/block/pg-sprite/pkg/diffplan" + "github.com/block/pg-sprite/pkg/executor" "github.com/block/pg-sprite/pkg/migrate" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" @@ -238,6 +239,37 @@ CREATE INDEX t_v_idx ON t (v);` assert.Empty(t, res.Plan.Statements, "a clean greenfield create converges on its second run") }) + t.Run("a failed index build keeps the created table and discloses the prefix", func(t *testing.T) { + // An index on a column the table does not have passes admission + // (shape and target) and the claimed-name probe (the name is free), + // so the create path commits the CREATE TABLE and fails on the + // index step โ€” committed-prefix semantics, disclosed by the + // verdicts: the executed create, then the failed build with the + // failing plan statement named. + schema := testutil.NewSchema(t, pool) + res, err := migrate.RunDesired(t.Context(), pool, + migrate.DesiredRequest{Schema: schema, Desired: parseDesired(t, ` + CREATE TABLE t (id int PRIMARY KEY, v text); + CREATE INDEX t_missing_idx ON t (missing);`)}, runOptions()) + require.Error(t, err, "a mid-plan execution failure returns the failed result with the error") + assert.Equal(t, verdict.OutcomeFailed, res.Outcome) + require.Len(t, res.Verdicts, 2, "the committed create and the failed index build") + assert.Equal(t, verdict.OutcomeExecuted, res.Verdicts[0].Outcome) + assert.Contains(t, res.Verdicts[0].Statement, "CREATE TABLE") + assert.Equal(t, verdict.OutcomeFailed, res.Verdicts[1].Outcome) + assert.Equal(t, string(executor.CodeExecutionFailed), res.Verdicts[1].Code) + assert.Contains(t, res.Verdicts[1].Statement, "t_missing_idx", + "the failed verdict names the plan statement that failed, not the one before it") + assert.Contains(t, res.Detail, "planned statement 2 of 2 failed", + "the disclosure counts the committed prefix") + + var exists bool + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT EXISTS (SELECT 1 FROM information_schema.tables + WHERE table_schema = $1 AND table_name = 't')`, schema).Scan(&exists)) + assert.True(t, exists, "the committed CREATE TABLE stays committed") + }) + t.Run("refuses a destructive plan and drops nothing", func(t *testing.T) { schema := testutil.NewSchema(t, pool) _, err := pool.Exec(t.Context(), fmt.Sprintf( diff --git a/pkg/preflight/absent.go b/pkg/preflight/absent.go index 604de65..2d0caae 100644 --- a/pkg/preflight/absent.go +++ b/pkg/preflight/absent.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "slices" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" @@ -150,16 +149,18 @@ func CheckTableAbsent(ctx context.Context, pool *pgxpool.Pool, schema, table str // CheckNamesAbsent verifies that no relation in schema occupies any of // names. It reads pg_class in one catalog snapshot and reports the first -// occupied name in lexical order, regardless of input order. It does not -// probe pg_type: this check protects index and constraint-index names, -// which do not create types; [CheckTableAbsent] separately protects the -// CREATE TABLE name and its composite type. +// occupied name in lexical order, regardless of input order โ€” the query's +// ORDER BY decides, so the caller need not sort. It does not probe +// pg_type: this check protects index and constraint-index names, which do +// not create types; [CheckTableAbsent] separately protects the CREATE +// TABLE name and its composite type. schema must be the resolved, +// non-empty schema the names would land in โ€” the one an [AbsentTarget] +// carries; a schema that does not exist holds no relations and reports +// every name free. func CheckNamesAbsent(ctx context.Context, pool *pgxpool.Pool, schema string, names []string) error { if len(names) == 0 { return nil } - ordered := slices.Clone(names) - slices.Sort(ordered) const q = ` SELECT c.relname, c.relkind::text FROM pg_class c @@ -168,7 +169,7 @@ func CheckNamesAbsent(ctx context.Context, pool *pgxpool.Pool, schema string, na ORDER BY c.relname LIMIT 1` var name, relkind string - err := pool.QueryRow(ctx, q, schema, ordered).Scan(&name, &relkind) + err := pool.QueryRow(ctx, q, schema, names).Scan(&name, &relkind) if errors.Is(err, pgx.ErrNoRows) { return nil } From c72ac530f9f4b4eeb1e707d0b72ad1f9d8600402 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 4 Sep 2026 10:14:12 +1000 Subject: [PATCH 3/3] fix(executor): claim column sequence names and bind the probe to the proof A serial or identity column owns a sequence whose first-choice name is part of the desired file's contract; an occupant made the create succeed with a suffixed name, the defect class this path exists to refuse. CheckNamesAbsent takes the AbsentTarget so an empty or unresolved schema can no longer report every name free. Docs qualify the SQLSTATE backstop: it covers explicit names only; server-chosen names have none. --- docs/capabilities.md | 2 +- docs/cli-output-examples.md | 2 +- docs/limitations.md | 2 +- docs/schemabot-integration.md | 21 +++++++---- pkg/executor/create.go | 39 ++++++++++--------- pkg/executor/create_integration_test.go | 25 ++++++++++++ pkg/migrate/desired.go | 4 +- pkg/preflight/absent.go | 24 ++++++------ pkg/preflight/absent_integration_test.go | 11 ++++-- pkg/statement/implicit.go | 44 ++++++++++++++++------ pkg/statement/implicit_integration_test.go | 18 ++++----- pkg/statement/implicit_test.go | 39 ++++++++++++++----- 12 files changed, 155 insertions(+), 76 deletions(-) diff --git a/docs/capabilities.md b/docs/capabilities.md index a02ff36..706ecf7 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -198,7 +198,7 @@ Status legend: โœ… T1 (supported today) ยท ๐ŸŸก T2 (planned; typed refusal today | Unlogged tables | ๐ŸŸก | native, planned flow | Yes | Typed refusal: persistence is not modeled, converging it (`SET LOGGED`) is a full rewrite, and rendering the table as plain `CREATE TABLE` would silently change crash-safety | | Explicit column collations | ๐ŸŸก | native, planned flow | Yes | Typed refusal: dropping a `COLLATE` clause from a rendered baseline silently changes sort order and index semantics; a collation delta cannot converge without a rewrite | | Columns whose default uses a sequence the column does not own | ๐ŸŸก | native, planned flow | Yes | Typed refusal: in a desired-state model that sequence exists only inside the scratch transaction, so no derived plan can reference it. Column-owned (`serial`-style) sequences are fine | -| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | native, as-is | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every index name the desired file states (explicit `CREATE INDEX` names and first-choice constraint-index names; an unnamed `CREATE INDEX ON t (v)` claims nothing) is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution โ€” drop or rename the occupant, or name the constraint's index explicitly; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | +| Greenfield `CREATE TABLE` apply (the table does not exist yet โ€” a fresh database or a new table in a live one) | โœ… | native, as-is | Yes โ€” a `REFERENCES` clause would take a brief `SHARE ROW EXCLUSIVE` on each **referenced** live table, but desired files refuse foreign keys today, so no live table is locked | Desired-state execution creates the table: `CheckTableAbsent` verifies the table relation and composite-type name are free, the executor verifies every relation name the desired file states (explicit index names and first-choice constraint-index and column-sequence names) is free in the schema, and `CheckCreatePrivileges` verifies the role can create there. It then runs the `CREATE TABLE` and index builds as brief bounded steps under the engine's budgets. An occupied claimed name is a typed `create-collision` refusal before execution โ€” drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. `PARTITION OF`, `INHERITS`, `LIKE`, `OF`, `IF NOT EXISTS`, and in-set duplicate names are typed refusals before anything runs, while `REFERENCES` and `CONCURRENTLY` are refused upstream at desired-file parse and re-checked at admission as defense in depth | ### Types and non-table objects diff --git a/docs/cli-output-examples.md b/docs/cli-output-examples.md index 851f0f2..3ac4dc2 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -87,7 +87,7 @@ in `detail`. The set is closed and pinned by test (`verdict.Reasons()`). | `backend-unavailable` | The change routes to an execution strategy this build does not implement (copy-and-swap). | | `destructive-change` | The desired-state plan discards live structure โ€” a dropped column, constraint, index, or `NOT NULL` โ€” and desired-state execution runs no destructive statement; run the drop deliberately instead ([execution model](execution-model.md)). | | `plan-fingerprint-mismatch` | The plan recomputed at execution time does not carry the pinned fingerprint: the plan a reviewer approved is not the plan that would execute, so nothing runs ([execution model](execution-model.md)). | -| `create-collision` | The greenfield create plan's table name or a claimed index/constraint-index name is occupied. Nothing runs; re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant or name the constraint's index explicitly in the desired file โ€” re-planning alone reproduces the refusal. Catalog absence checks handle existing occupants; duplicate-name SQLSTATEs remain the race backstop. | +| `create-collision` | The greenfield create plan's table name or a claimed index, constraint-index, or sequence name is occupied. Nothing runs; re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column โ€” re-planning alone reproduces the refusal. Catalog absence checks handle existing occupants. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. | ## Migrate diff --git a/docs/limitations.md b/docs/limitations.md index 41840ad..2971310 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -44,7 +44,7 @@ composition of the model boundaries above with those gates. At a glance: | Desired-file edit | Outcome today | | --- | --- | -| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the table name and every index name the desired file states (explicit `CREATE INDEX` names and first-choice constraint-index names; an unnamed `CREATE INDEX ON t (v)` claims nothing) are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. An occupied claimed name is a typed `create-collision` refusal before anything runs โ€” drop or rename the occupant, or name the constraint's index explicitly; duplicate-name SQLSTATEs remain the race backstop. `PARTITION OF` and `IF NOT EXISTS` are also typed refusals before execution. | +| A desired file whose table does not exist yet | Converges โ€” the greenfield create path verifies the table name and every relation name the desired file states (explicit index names and first-choice constraint-index and column-sequence names) are free and the role holds `CREATE` on the schema, then runs the `CREATE TABLE` and index builds as brief bounded steps. Names the server invents rather than names the desired file states are outside this coverage. An occupied claimed name is a typed `create-collision` refusal before anything runs โ€” drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Duplicate-name SQLSTATEs backstop races for explicit names; for server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it. `PARTITION OF` and `IF NOT EXISTS` are also typed refusals before execution. | | Add a column | Converges. Runs as a bounded attempt of the submitted form, so the table-size guard applies (below). | | Widen a column type (`varchar(50)` โ†’ `varchar(255)`) | Converges โ€” the same bounded attempt, under the same size guard. | | Add an index | Converges via `CREATE INDEX CONCURRENTLY`. Not size-guarded: long online work on a large table is the pattern's purpose. | diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 9fe0ce8..70cd800 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -150,7 +150,7 @@ it to one of four things: | Outcome | Routing class | | --- | --- | | Executed | The table and its indexes exist; a rerun converges to an empty plan | -| `create-collision` refusal | **Re-plan, then fix the occupant**: the table name or a claimed index/constraint-index name is occupied. Re-diff the live catalog to see what holds it; re-planning alone reproduces the refusal โ€” drop or rename the occupant, or name the constraint's index explicitly in the desired file. Never blindly retry | +| `create-collision` refusal | **Re-plan, then fix the occupant**: the table name or a claimed index, constraint-index, or sequence name is occupied. Re-diff the live catalog to see what holds it; re-planning alone reproduces the refusal โ€” drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column. Never blindly retry | | `insufficient-privileges` refusal (`*preflight.PrivilegeError`, `Tier == TierCreateTable`) | **Operator provisioning action**: the role needs the exact `GRANT` the error carries โ€” not a desired-file fix, and not retryable until granted | | Admission refusal (`unsupported-statement`) | **Author action**: the desired file states a shape the create path refuses; retrying unchanged cannot succeed | @@ -168,7 +168,7 @@ The greenfield `CREATE TABLE` path is a fixed call order, all inside the apply s 2. `preflight.CheckTableAbsent` โ€” mint the `AbsentTarget` proof for the table name. 3. `preflight.CheckCreatePrivileges` โ€” mint the `CreationRole` proof for the target schema. 4. `executor.ExecuteCreate` โ€” consume both proofs, admit the set, probe `pg_class` for every - index and constraint-index name the set claims, then run the set. + index, constraint-index, and sequence name the set claims, then run the set. `migrate.RunDesired` runs this sequence itself when the plan is greenfield โ€” the adapter does not assemble it and must not mint either proof separately (a proof minted outside the @@ -205,14 +205,19 @@ them, don't retry them uniformly: | `ErrPartitionOfUnsupported` (`partition-of-unsupported`) | `PARTITION OF` binds to a live parent the absence proof does not cover | Fix the desired file; out of the create path's scope | | `ErrIfNotExistsUnsupported` (`if-not-exists-unsupported`) | `CREATE ... IF NOT EXISTS` succeeds as a name-only no-op over a relation it cannot vouch for โ€” the opposite of the absence proof's fail-closed contract; refused at admission, nothing ran | Fix the desired file: state the plain `CREATE`; the absence check owns collision handling | | `ErrUnsupportedCreateStep` (`unsupported-create-step`) | A desired statement is not a shape the create path can run | Fix the desired file | -| `ErrCreateCollision` (`create-collision`) | A claimed index or constraint-index name was already occupied, or a concurrent writer took a needed name after the absence checks | Re-diff the live catalog to see what holds the name, then drop or rename the occupant or name the constraint's index explicitly in the desired file โ€” re-planning alone reproduces the refusal; never blindly retry the create | +| `ErrCreateCollision` (`create-collision`) | A claimed index, constraint-index, or sequence name was already occupied, or a concurrent writer took a needed name after the absence checks | Re-diff the live catalog to see what holds the name, then drop or rename the occupant, name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column โ€” re-planning alone reproduces the refusal; never blindly retry the create | Before the first step, `ExecuteCreate` probes `pg_class` in one schema-scoped catalog -snapshot for every index name the desired file states โ€” explicit `CREATE INDEX` names and -the first-choice name of each implicit constraint index. An unnamed `CREATE INDEX ON t (v)` -claims nothing: the server invents its name and the probe has nothing to check. -`CheckTableAbsent` separately covers the table relation and composite type. The -duplicate-name SQLSTATE mapping remains the race backstop after those checks. +snapshot for every relation name the desired file states โ€” explicit `CREATE INDEX` names +and the first-choice names of implicit constraint indexes and column-owned sequences. Names +the server invents rather than names the desired file states, such as an unnamed `CREATE +INDEX ON t (v)`, are outside this coverage because the probe has nothing to check. +`CheckTableAbsent` separately covers the table relation and composite type. Duplicate-name +SQLSTATEs backstop races for explicit names. For server-chosen names, the probe narrows the +race to the time-of-check window, but nothing catches a name taken inside it. + +A `create-collision` can identify a name the table needs โ€” an index, constraint index, or +sequence โ€” rather than the table name itself. A failed create is not rolled back wholesale: each step committed in its own bounded transaction, so the steps before the failure remain diff --git a/pkg/executor/create.go b/pkg/executor/create.go index fe83d5d..7ce3533 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -11,12 +11,13 @@ // // Before the first step the executor probes pg_class for every name the // desired file states โ€” explicit index names and the first-choice names of -// index-backed constraints โ€” so an occupied name refuses the whole set -// rather than failing after the table committed; the table name itself is -// the caller's absence proof. The proof is time-of-check: nothing locks the -// names, so a concurrent create can still take one before its step. That -// loss surfaces through SQLSTATE 42P07 or 42710 as ErrCreateCollision โ€” the -// caller re-diffs rather than assuming what the collision left behind. A +// index-backed constraints and column-owned sequences โ€” so an occupied +// name refuses the whole set rather than failing after the table committed; +// the table name itself is the caller's absence proof. The proof is +// time-of-check: nothing locks the names, so a concurrent create can still +// take one before its step. Duplicate-name SQLSTATEs backstop races for +// explicit names. For server-chosen names, the probe narrows the race to +// the time-of-check window, but nothing catches a name taken inside it. A // failed step ends the run immediately; the steps before it committed // (each in its own bounded transaction) remain, so a rerun's absence check refuses with // ErrRelationExists and the declarative front door re-diffs and applies @@ -45,11 +46,13 @@ import ( var ( // ErrCreateCollision is returned when a claimed name is already taken. // A catalog probe after admission checks the desired file's explicit - // index names and first-choice constraint-index names before the first - // step executes, refusing the whole set with no *SequenceStepError; - // duplicate-name SQLSTATEs remain the time-of-check race backstop and - // arrive wrapped in one. The remedy is on the desired file's side โ€” drop - // or rename the occupant, or name the constraint's index explicitly โ€” + // index names and first-choice constraint-index and sequence names before + // the first step executes, refusing the whole set with no *SequenceStepError; + // duplicate-name SQLSTATEs backstop time-of-check races for explicit + // names and arrive wrapped in one. Server-chosen names have no such + // backstop. The remedy is on the desired file's side โ€” drop or rename the + // occupant, name a constraint's index explicitly, or for a sequence use + // an explicitly named sequence or a non-serial column โ€” // then re-diff; a re-plan alone reproduces the refusal. ErrCreateCollision = errors.New("a name the create path needs is already taken") // ErrDuplicateCreateName is returned when the desired set claims the @@ -160,7 +163,7 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // refuses the whole set instead of failing after the table committed. // CheckTableAbsent already covers the table name and its composite type. claimed = slices.DeleteFunc(claimed, func(name string) bool { return name == at.Table() }) - err = preflight.CheckNamesAbsent(ctx, pool, at.Schema(), claimed) + err = preflight.CheckNamesAbsent(ctx, pool, at, claimed) if preflight.IsNameOccupied(err) { return rep, fmt.Errorf("%w: the desired file claims a name the catalog already holds: %w", ErrCreateCollision, err) @@ -201,9 +204,9 @@ func executeCreate(ctx context.Context, pool *pgxpool.Pool, at preflight.AbsentT // arrive in execution order โ€” the CREATE TABLE first, the indexes in input // order after it, ordered once by statement.ParseDesired โ€” and the steps // keep that order. Every step claims the names it will occupy in the -// same pg_class namespace โ€” the table plus the first-choice index names -// of its index-backed constraints, or an explicit index name โ€” so a name -// claimed twice within the set โ€” decidable here โ€” is refused before +// same pg_class namespace โ€” the table plus the first-choice relation names +// of its constraints and column-owned sequences, or an explicit index name. +// 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, @@ -245,8 +248,8 @@ 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 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 +// table name plus the first-choice relation names of its constraints and +// column-owned sequences, for a CREATE INDEX its explicit name, nothing when the // 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 @@ -289,7 +292,7 @@ func admitCreateStep(at preflight.AbsentTarget, sql string) (statement.Statement if op.IfNotExists { return statement.Statement{}, nil, ErrIfNotExistsUnsupported } - implicit, err := statement.ImplicitIndexNames(qualified) + implicit, err := statement.ImplicitRelationNames(qualified) if err != nil { // ParseOne already admitted this SQL as a CREATE TABLE, so a // refusal here means the two parse boundaries disagree. diff --git a/pkg/executor/create_integration_test.go b/pkg/executor/create_integration_test.go index 95c5f80..aa13231 100644 --- a/pkg/executor/create_integration_test.go +++ b/pkg/executor/create_integration_test.go @@ -158,6 +158,31 @@ func TestExecuteCreateRefusesOccupiedImplicitIndexNameBeforeExecution(t *testing assert.False(t, relationExists(t, f.pool, f.schema, "t"), "the catalog preflight runs before every step") } +// A column-owned sequence's first-choice name is part of the desired set's +// contract. An occupant must refuse the whole set rather than make +// PostgreSQL silently suffix the sequence name. +func TestExecuteCreateRefusesOccupiedImplicitSequenceNameBeforeExecution(t *testing.T) { + tests := []struct { + name string + sql string + }{ + {name: "serial", sql: "CREATE TABLE t (id serial PRIMARY KEY)"}, + {name: "identity", sql: "CREATE TABLE t (id bigint GENERATED BY DEFAULT AS IDENTITY)"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newCreateFixture(t, "t") + _, err := f.pool.Exec(t.Context(), fmt.Sprintf("CREATE SEQUENCE %s.t_id_seq", f.schema)) + require.NoError(t, err) + + rep, err := executor.ExecuteCreate(t.Context(), f.pool, f.at, f.cr, desired(t, tt.sql), createBudget, executor.DefaultRetryPolicy()) + require.ErrorIs(t, err, executor.ErrCreateCollision) + assert.Empty(t, rep.Steps) + assert.False(t, relationExists(t, f.pool, f.schema, "t"), "the catalog preflight runs before every step") + }) + } +} + // A claimed-name probe that cannot complete says nothing about whether the // names are free: it is the caller's operational failure, never a // collision, so the caller retries rather than being told a free name is diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index b119407..3f7ba70 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -263,8 +263,8 @@ func runCreate(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, repo result.Outcome = verdict.OutcomeRefused result.Reason = verdict.ReasonCreateCollision result.Detail = fmt.Sprintf("the create path refused the plan because the catalog already holds a name it claims (%v); "+ - "re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant "+ - "or name the constraint's index explicitly in the desired file; nothing was executed", execErr) + "re-derive the plan against the live catalog to see what holds the name, then drop or rename the occupant, "+ + "name a constraint's index explicitly, or for a sequence use an explicitly named sequence or a non-serial column; nothing was executed", execErr) return result, nil } if isCreateAdmissionRefusal(execErr) { diff --git a/pkg/preflight/absent.go b/pkg/preflight/absent.go index 2d0caae..b0f9315 100644 --- a/pkg/preflight/absent.go +++ b/pkg/preflight/absent.go @@ -147,20 +147,22 @@ func CheckTableAbsent(ctx context.Context, pool *pgxpool.Pool, schema, table str return AbsentTarget{schema: *targetSchema, table: table}, nil } -// CheckNamesAbsent verifies that no relation in schema occupies any of -// names. It reads pg_class in one catalog snapshot and reports the first -// occupied name in lexical order, regardless of input order โ€” the query's -// ORDER BY decides, so the caller need not sort. It does not probe -// pg_type: this check protects index and constraint-index names, which do -// not create types; [CheckTableAbsent] separately protects the CREATE -// TABLE name and its composite type. schema must be the resolved, -// non-empty schema the names would land in โ€” the one an [AbsentTarget] -// carries; a schema that does not exist holds no relations and reports -// every name free. -func CheckNamesAbsent(ctx context.Context, pool *pgxpool.Pool, schema string, names []string) error { +// CheckNamesAbsent verifies that no relation in the proved target's schema +// occupies any of names. It reads pg_class in one catalog snapshot and +// reports the first occupied name in lexical order, regardless of input +// order โ€” the query's ORDER BY decides, so the caller need not sort. It does not probe +// pg_type: this check protects index, constraint-index, and sequence names; +// [CheckTableAbsent] separately protects the CREATE TABLE name and its +// composite type. The [AbsentTarget] binds the check to the resolved, +// existing schema where those names would land. +func CheckNamesAbsent(ctx context.Context, pool *pgxpool.Pool, at AbsentTarget, names []string) error { if len(names) == 0 { return nil } + schema := at.Schema() + if schema == "" || at.Table() == "" { + return fmt.Errorf("check claimed names: absence proof carries no verified target") + } const q = ` SELECT c.relname, c.relkind::text FROM pg_class c diff --git a/pkg/preflight/absent_integration_test.go b/pkg/preflight/absent_integration_test.go index 7506c9a..0d1c4a9 100644 --- a/pkg/preflight/absent_integration_test.go +++ b/pkg/preflight/absent_integration_test.go @@ -31,9 +31,12 @@ func TestCheckNamesAbsent(t *testing.T) { t.Cleanup(pool.Close) schema := testutil.NewSchema(t, pool) otherSchema := testutil.NewSchema(t, pool) + at, err := preflight.CheckTableAbsent(t.Context(), pool, schema, "new_table") + require.NoError(t, err) - require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, schema, nil)) - require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, schema, []string{"free_name"})) + require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, at, nil)) + require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, at, []string{"free_name"})) + require.Error(t, preflight.CheckNamesAbsent(t.Context(), pool, preflight.AbsentTarget{}, []string{"free_name"})) _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other (v int)", schema)) require.NoError(t, err) _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE INDEX t_pkey ON %s.other (v)", schema)) @@ -43,10 +46,10 @@ func TestCheckNamesAbsent(t *testing.T) { _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.other_schema_only (v int)", otherSchema)) require.NoError(t, err) - err = preflight.CheckNamesAbsent(t.Context(), pool, schema, []string{"z_free", "t_pkey"}) + err = preflight.CheckNamesAbsent(t.Context(), pool, at, []string{"z_free", "t_pkey"}) assert.ErrorIs(t, err, preflight.ErrRelationExists) assert.Contains(t, err.Error(), "t_pkey") - require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, schema, []string{"other_schema_only"})) + require.NoError(t, preflight.CheckNamesAbsent(t.Context(), pool, at, []string{"other_schema_only"})) } // An unqualified check resolves the schema an unqualified CREATE TABLE diff --git a/pkg/statement/implicit.go b/pkg/statement/implicit.go index 0957923..62e5ffc 100644 --- a/pkg/statement/implicit.go +++ b/pkg/statement/implicit.go @@ -1,7 +1,8 @@ -// 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 โ€” +// This file predicts the relation names PostgreSQL invents for a CREATE +// TABLE's index-backed constraints and column-owned sequences. 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 @@ -24,24 +25,25 @@ import ( ) // ErrNotCreateTable is returned when the statement handed to -// ImplicitIndexNames is not a single CREATE TABLE. +// ImplicitRelationNames 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 +// ImplicitRelationNames returns the first-choice relation names PostgreSQL +// will use for one CREATE TABLE statement: indexes for PRIMARY KEY, UNIQUE, +// and EXCLUDE constraints, and sequences for serial and identity columns. +// 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 +// to the identifier byte budget the way the server truncates). A sequence +// takes `
__seq` under the same truncation rules. 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) { +func ImplicitRelationNames(sql string) ([]string, error) { tree, err := pgquery.Parse(sql) if err != nil { return nil, fmt.Errorf("parse statement: %w", err) @@ -66,6 +68,9 @@ func ImplicitIndexNames(sql string) ([]string, error) { if col == nil { continue } + if columnOwnsSequence(col) { + names = append(names, makeObjectName(table, col.GetColname(), "seq")) + } for _, c := range col.GetConstraints() { con := c.GetConstraint() if con == nil { @@ -79,6 +84,21 @@ func ImplicitIndexNames(sql string) ([]string, error) { return names, nil } +// columnOwnsSequence reports whether the column definition creates a +// sequence whose name is chosen from the table and column names. +func columnOwnsSequence(col *pganalyze.ColumnDef) bool { + typeName, _ := typeRef(col.GetTypeName()) + if isSerialType(typeName) { + return true + } + for _, node := range col.GetConstraints() { + if node.GetConstraint().GetContype() == pganalyze.ConstrType_CONSTR_IDENTITY { + return true + } + } + return false +} + // 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) { diff --git a/pkg/statement/implicit_integration_test.go b/pkg/statement/implicit_integration_test.go index 722a7a4..3ce0a12 100644 --- a/pkg/statement/implicit_integration_test.go +++ b/pkg/statement/implicit_integration_test.go @@ -14,11 +14,11 @@ import ( ) // Two-oracle check (TM): for each representative CREATE TABLE, the names -// ImplicitIndexNames predicts must be exactly the index names the real +// ImplicitRelationNames predicts must be exactly the index and sequence 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) { +func TestImplicitRelationNamesMatchServer(t *testing.T) { pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) require.NoError(t, err) t.Cleanup(pool.Close) @@ -37,11 +37,13 @@ func TestImplicitIndexNamesMatchServer(t *testing.T) { {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: "serial sequence", sql: "CREATE TABLE t (id serial PRIMARY KEY)"}, + {name: "identity sequence", sql: "CREATE TABLE t (id bigint GENERATED ALWAYS AS IDENTITY)"}, {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) + predicted, err := statement.ImplicitRelationNames(tt.sql) require.NoError(t, err) schema := testutil.NewSchema(t, pool) @@ -54,13 +56,11 @@ func TestImplicitIndexNamesMatchServer(t *testing.T) { 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 + `SELECT c.relname + FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE n.nspname = $1 - ORDER BY ic.oid`, schema) + WHERE n.nspname = $1 AND c.relkind IN ('i', 'S') + ORDER BY c.oid`, schema) require.NoError(t, err) var actual []string for rows.Next() { diff --git a/pkg/statement/implicit_test.go b/pkg/statement/implicit_test.go index e42fc31..e80ca4a 100644 --- a/pkg/statement/implicit_test.go +++ b/pkg/statement/implicit_test.go @@ -10,7 +10,7 @@ import ( "github.com/block/pg-sprite/pkg/statement" ) -func TestImplicitIndexNames(t *testing.T) { +func TestImplicitRelationNames(t *testing.T) { tests := []struct { name string sql string @@ -66,6 +66,21 @@ func TestImplicitIndexNames(t *testing.T) { 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: "serial column sequence", + sql: "CREATE TABLE t (id serial PRIMARY KEY)", + want: []string{"t_id_seq", "t_pkey"}, + }, + { + name: "identity column sequence", + sql: "CREATE TABLE t (id bigint GENERATED BY DEFAULT AS IDENTITY)", + want: []string{"t_id_seq"}, + }, + { + name: "always identity column sequence", + sql: "CREATE TABLE t (id bigint GENERATED ALWAYS AS IDENTITY)", + want: []string{"t_id_seq"}, + }, { name: "qualified table uses the bare relation name", sql: `CREATE TABLE "s"."t" (id int PRIMARY KEY)`, @@ -79,7 +94,7 @@ func TestImplicitIndexNames(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := statement.ImplicitIndexNames(tt.sql) + got, err := statement.ImplicitRelationNames(tt.sql) require.NoError(t, err) assert.Equal(t, tt.want, got) }) @@ -89,9 +104,9 @@ func TestImplicitIndexNames(t *testing.T) { // 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) { +func TestImplicitRelationNamesTruncatesLikeTheServer(t *testing.T) { table := strings.Repeat("t", 70) - got, err := statement.ImplicitIndexNames("CREATE TABLE " + table + " (id int PRIMARY KEY)") + got, err := statement.ImplicitRelationNames("CREATE TABLE " + table + " (id int PRIMARY KEY)") require.NoError(t, err) require.Len(t, got, 1) // NAMEDATALEN-1 = 63: 58 bytes of table + "_pkey". @@ -99,20 +114,26 @@ func TestImplicitIndexNamesTruncatesLikeTheServer(t *testing.T) { assert.LessOrEqual(t, len(got[0]), 63) column := strings.Repeat("c", 70) - got, err = statement.ImplicitIndexNames("CREATE TABLE t (" + column + " int UNIQUE)") + got, err = statement.ImplicitRelationNames("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) + + got, err = statement.ImplicitRelationNames("CREATE TABLE " + table + " (" + column + " serial)") + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, strings.Repeat("t", 29)+"_"+strings.Repeat("c", 29)+"_seq", got[0]) + assert.LessOrEqual(t, len(got[0]), 63) } -func TestImplicitIndexNamesRefusesNonCreateTable(t *testing.T) { - _, err := statement.ImplicitIndexNames("CREATE INDEX i ON t (id)") +func TestImplicitRelationNamesRefusesNonCreateTable(t *testing.T) { + _, err := statement.ImplicitRelationNames("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)") +func TestImplicitRelationNamesRefusesMultipleStatements(t *testing.T) { + _, err := statement.ImplicitRelationNames("CREATE TABLE t (id int); CREATE TABLE u (id int)") require.ErrorIs(t, err, statement.ErrNotOneStatement) }