diff --git a/docs/capabilities.md b/docs/capabilities.md index fc49a2e..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: 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 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 d04c743..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 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, 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 545bbe1..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 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 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 e4da940..30650a0 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, 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 | @@ -167,7 +167,8 @@ 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, constraint-index, and sequence name the set claims, then run the set. For the shapes the create path admits, the plan's `exec_sql` is the statement as written โ€” the plain `CREATE TABLE` and index builds this path runs, with each build's decision @@ -176,10 +177,13 @@ born in that run. `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 @@ -206,7 +210,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 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, 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 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 c5240ef..695d1ac 100644 --- a/pkg/executor/create.go +++ b/pkg/executor/create.go @@ -9,13 +9,17 @@ // 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 +// 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 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 // the remainder. @@ -25,6 +29,7 @@ import ( "context" "errors" "fmt" + "slices" "time" "github.com/jackc/pgx/v5/pgconn" @@ -39,12 +44,16 @@ 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. + // A catalog probe after admission checks the desired file's explicit + // 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 // same relation name twice โ€” two indexes under one name, or an index @@ -88,13 +97,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) } @@ -140,10 +154,26 @@ 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() }) + 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) + } + 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() if tracker != nil { @@ -174,46 +204,52 @@ 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, // 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 + // Order does not matter: the catalog probe is set membership and picks + // the reported occupant itself. + names := make([]string, 0, len(claimed)) + for name := range claimed { + names = append(names, name) + } + return steps, names, nil } // admitCreateStep qualifies one desired statement into the proof's schema, // re-parses it by the real grammar, and admits it by shape and target. It // returns the pg_class names the step will claim โ€” for a CREATE TABLE the -// table name plus the first-choice index names of its index-backed -// constraints, for a CREATE INDEX its explicit name, nothing when the +// 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 @@ -256,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 5910769..4c16d8e 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" "sync" "testing" @@ -208,6 +209,67 @@ 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 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 +// 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 96d6d5a..3f7ba70 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -257,8 +257,16 @@ 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 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) { 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..0842f7f 100644 --- a/pkg/migrate/desired_integration_test.go +++ b/pkg/migrate/desired_integration_test.go @@ -208,26 +208,60 @@ 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.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.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("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.CodeCreateCollision), res.Verdicts[1].Code) + 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(), diff --git a/pkg/preflight/absent.go b/pkg/preflight/absent.go index 4b74dca..b0f9315 100644 --- a/pkg/preflight/absent.go +++ b/pkg/preflight/absent.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" ) @@ -145,3 +146,37 @@ func CheckTableAbsent(ctx context.Context, pool *pgxpool.Pool, schema, table str } return AbsentTarget{schema: *targetSchema, table: table}, nil } + +// 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 + 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, names).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..0d1c4a9 100644 --- a/pkg/preflight/absent_integration_test.go +++ b/pkg/preflight/absent_integration_test.go @@ -25,6 +25,33 @@ 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) + at, err := preflight.CheckTableAbsent(t.Context(), pool, schema, "new_table") + require.NoError(t, err) + + 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)) + 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, 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, at, []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) { 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) }