feat(executor): prove claimed index names free before the create runs

[Kiran01bm]

The create path now probes pg_class for every index name the desired set states — explicit CREATE INDEX names and the first-choice name of each PRIMARY KEY/UNIQUE constraint index — before the first step runs, refusing an occupied name as a typed create-collision with nothing executed.

Why

CheckTableAbsent proved only the table name free, and the two index-name cases behaved differently when an unrelated relation already held the name:

• Explicit CREATE INDEX t_v_idx ON t (v): the CREATE TABLE committed, then the index step failed with a duplicate-name SQLSTATE. The rerun saw a table that exists with a shape the desired file does not state, and the reviewer got a mid-run failure rather than a refusal on a clean catalog.
• Implicit PRIMARY KEY / UNIQUE: the create succeeded — PostgreSQL sidesteps the occupied first-choice name with a numeric suffix (t_pkey1) inside the CREATE TABLE. The catalog then holds a constraint index whose name is not the one the desired file implies, and a later re-diff of the same file cannot tell that index apart from a stray.

This PR treats the first-choice name as part of the desired file's contract: both cases now refuse before anything runs. For the implicit case that is a policy change, not a bug fix — a desired file that used to converge with a suffixed index name is now refused until the occupant is dropped or renamed, or the constraint's index is named explicitly.

What

• preflight.CheckNamesAbsent(ctx, pool, schema, names): one schema-scoped pg_class probe over the claimed names, wrapping ErrRelationExists. It deliberately does not probe pg_type — index names create no types; the table's composite type is already covered by CheckTableAbsent. The schema must be resolved and non-empty; ORDER BY relname decides which occupant is reported.
• executeCreate runs the probe over the claimed names (minus the table) before the step loop. Only preflight.IsNameOccupied hits become ErrCreateCollision; a probe that itself fails (cancelled context, dropped connection) is returned as an operational error, not a collision.
• migrate.runCreate maps a pre-execution ErrCreateCollision to a refused verdict with create-collision whose detail names the remedy, so callers see a refusal, not a failed apply.
• An unnamed CREATE INDEX ON t (v) claims nothing: the server invents its name and the probe has nothing to check. Docs state this boundary rather than claiming total coverage.
• Duplicate-name SQLSTATEs stay in place as the time-of-check race backstop.

Before / after

Before                                        After
explicit CREATE INDEX t_v_idx, t_v_idx taken  CheckTableAbsent(t)              ok
  CREATE TABLE t        committed             CheckCreatePrivileges            ok
  CREATE INDEX t_v_idx  42P07 -> failed       CheckNamesAbsent(t_pkey, t_v_idx)
  table t left behind, rerun re-diffs           -> refused create-collision
                                                 nothing executed, catalog unchanged
implicit PRIMARY KEY, t_pkey taken              detail: drop/rename the occupant,
  CREATE TABLE t        committed as t_pkey1     or name the constraint's index
  desired file's implied name never exists

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 2fc6a04c.

Verdict: the probe is correct, fail-closed on its own failure, and the right shape — nothing blocks. I attacked the claim "every name the desired file states is proved free", and it holds for indexes and constraint indexes: ImplicitIndexNames covers table-level and inline PRIMARY KEY/UNIQUE, EXCLUDE, named constraints, and the server's expr substitution. Three findings, all proved against a real PostgreSQL 16: one unenforced precondition in a core-package export, and two places where the stated boundary is narrower than the real one.

Findings

1. CheckNamesAbsent reports every name free when schema is empty or does not exist, and nothing enforces otherwise. The godoc states the precondition ("must be the resolved, non-empty schema … the one an AbsentTarget carries") but the body only filters n.nspname = $1, so a wrong schema yields pgx.ErrNoRows → nil → all-clear. Proved: with <schema>.occupied existing, CheckNamesAbsent(ctx, pool, "", []string{"occupied"}) returns nil, and so does "no_such_schema". Today's only caller passes at.Schema() so it can't trip, but this is an exported function in a core package whose failure mode is a safety check that passes, and SAFETY.md is explicit on both halves — "never trust callers … the core enforces", and dangerous APIs take proof types, "never a raw string … a caller could fabricate." The proof type already exists and the caller already holds it: taking preflight.AbsentTarget instead of schema string makes the illegal state unrepresentable and costs one signature line.

2. "Duplicate-name SQLSTATEs remain the race backstop" is true for explicit index names and false for exactly the case that motivated this PR. The implicit constraint index does not raise 42P07/42710 — PostgreSQL picks a non-conflicting name inside the CREATE TABLE, which is the silent-suffix behavior the Why section documents. Proved: with t_pkey occupied, CREATE TABLE t (id int PRIMARY KEY) succeeds and the resulting index is t_pkey1. So in the time-of-check window the implicit case has no backstop: the probe passes, another session takes t_pkey, the create succeeds, and the desired file's contract is violated silently — narrowed to a window, not closed. The claim appears in the create.go header, the ErrCreateCollision godoc, and four docs pages. Detection is cheap and unambiguous precisely because absence was proved first: after the CREATE TABLE step, the actual constraint-index names either match the claimed first-choice names or the race happened.

3. serial / GENERATED AS IDENTITY sequence names are part of the desired file's contract, are not in the claim set, and do not fail closed. ImplicitIndexNames returns index names only — by name and by design — so CREATE TABLE s (id serial PRIMARY KEY) claims [s_pkey] and never s_id_seq. Proved for both spellings: with s_id_seq occupied, the create succeeds and the sequence lands as s_id_seq1; identical for id bigint GENERATED BY DEFAULT AS IDENTITY. This is the same defect class the PR closes for constraint indexes — the catalog holds an object whose name the desired file implies but does not have, and a later re-diff cannot tell it from a stray — except it needs no race at all: it happens on a quiet catalog, every time. The machinery is already in place, since sequences live in pg_class and the probe does not filter relkind; what's missing is the name in the claim set.

Action items

1. (Finding 1) Change CheckNamesAbsent to accept preflight.AbsentTarget rather than schema string, deriving the schema from the proof. If the raw-string signature has a caller you want to keep, reject schema == "" and add the case to TestCheckNamesAbsent.
2. (Finding 3) Add the column-owned sequence names (<table>_<column>_seq for serial and identity columns) to the claim set, and cover both spellings in create_integration_test.go. Sequences are already in scope for the probe's query.
3. (Finding 2) Qualify the backstop sentence wherever it appears: the duplicate-name SQLSTATEs cover explicit index names, and the implicit constraint index has no SQLSTATE to catch. Either state the residual window plainly, or close it by comparing the constraint indexes' real names against the claimed ones after the CREATE TABLE step.
4. (optional) docs/schemabot-integration.md names the unnamed CREATE INDEX as the coverage boundary; once finding 3 lands, the boundary is worth restating as "names the server invents", which is the property that actually decides it.

Verified (tried to break, couldn't)

ImplicitIndexNames covers table-level and inline PRIMARY KEY/UNIQUE with their different first-choice forms (t_pkey vs t_col_key), EXCLUDE (_excl), explicitly named constraints via conname, and the literal expr the server substitutes for expression elements; FOREIGN KEY/CHECK build no index and are correctly absent; the probe deliberately omits a relkind filter, which is right because relation names share one per-schema namespace — a table squatting an index name is caught, and the new test pins cross-schema isolation; parameters carry the names so nothing is interpolated; IsNameOccupied gates the collision mapping and any other probe error returns wrapped as operational, so an unreachable catalog cannot become a passing check or a false refusal; slices.DeleteFunc mutates a slice freshly built from the local map, so there is no aliasing, and the map's non-deterministic order is neutralized by ORDER BY relname LIMIT 1 exactly as the comment claims; the probe is one bounded query under dbconn's statement timeout with the name list bounded by the desired set; INV ST-7/ST-8 checks and the PARTITION OF/INHERITS/LIKE/OF/IF NOT EXISTS refusals are unchanged; the privilege-vs-index-collision ordering change is real and is documented honestly rather than papered over; go build ./... clean and ./pkg/preflight ./pkg/executor ./pkg/migrate ./pkg/statement all pass locally at head; 14/14 CI green; no tests or assertions removed.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Second pass on the same head (2fc6a04c), through the two lenses that matter beyond correctness: what an outside adopter experiences, and what SchemaBot does with the new refusal.

Lens 1 — outside adopter

Strong. The refusal text in migrate/desired.go is the best kind: it names the occupant, states that nothing was executed, and gives two concrete ways out (drop or rename the occupant, or name the constraint's index explicitly). That is a refusal an operator can act on without reading the source. docs/limitations.md and docs/capabilities.md both land the "proved free before, not discovered during" framing well, and cli-output-examples.md showing the actual rendered refusal is exactly the right way to document it.

Two rough edges an adopter will hit before we do:

1. The sequence gap in finding 3 of the correctness pass is the one place a first run leaves a silently renamed object behind. An adopter's first desired file is very often id serial PRIMARY KEY, and if <table>_id_seq happens to be taken, they get a successful create and a sequence they did not name — the exact surprise this PR exists to remove, with none of its subtlety.
2. docs/limitations.md reads as if the only uncovered case is an unnamed CREATE INDEX ON t (v). Once the boundary is stated as "names the server invents rather than names you wrote", both the index case and the sequence case fall out of one sentence, and the doc stops needing an enumeration that will drift.

Lens 2 — SchemaBot integration

The wire contract is unchanged (ErrCreateCollision and CodeCreateCollision both already existed), so nothing breaks on bump — but the conditions under which SchemaBot renders those two branches change materially, and its text is now wrong in two ways. pkg/engine/postgres/apply.go handles both:

• The preflight.IsNameOccupied branch says a relation already occupies the name %q on the target with the table name interpolated. After this PR the occupied name is usually an index or constraint name, not the table — the table's own absence was proved separately, one step earlier. The message will name an object that is demonstrably free.
• Both that branch and the CodeCreateCollision branch tell the operator to re-plan against the current schema. This PR's own docs are explicit that re-planning does not clear a name collision: the desired file still claims the name, so the next plan produces the same refusal. The remedy is the one pg-sprite already wrote — drop or rename the occupant, or name the constraint's index explicitly.

Both sites synthesize their own detail and discard pg-sprite's Detail, which is why the drift is invisible: refusalForOutcome's exhaustiveness test over executor.Codes() still passes, because the vocabulary didn't move — only the firing conditions and the correct advice did. That's a SchemaBot-side follow-up, not a blocker on this PR; I'll carry it. Worth a line in docs/schemabot-integration.md noting that a create-collision may now be about a name the table needs rather than the table itself, so the next consumer doesn't inherit the same wrong assumption.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving — the probe is correct and fail-closed, and my three findings are all follow-ups rather than blockers: an unenforced precondition on a core-package export, one uncovered claimed-name class (sequences), and a backstop claim in the docs that is narrower than stated. Details in the two review comments above.

This review was generated by Claude Code (claude-opus-5).