preflight: LookupOwnedRelationNames reads a table's owned index and sequence names

[Kiran01bm]

preflight.LookupOwnedRelationNames reads the constraint-index and column-owned-sequence names a table actually owns, so a caller can compare them against the names a create claimed.

Why

The create path proves a table's implicit constraint-index and sequence names absent before the CREATE TABLE step, but PostgreSQL never raises 42P07/42710 for an implicit name taken inside the time-of-check window — it silently suffixes (t_pkey1, t_id_seq1) and the create succeeds with names the plan never claimed. Closing that window needs a read of what the table ended up owning, after the step, from the catalog. No such read exists today; CheckNamesAbsent only asks whether a name is free.

What

OwnedRelationNames{ConstraintIndexes, Sequences} and LookupOwnedRelationNames(ctx, pool, schema, table) in pkg/preflight/owned.go: one query over pg_constraint (contype p/u/x — the constraints that build an index on the table itself; a foreign key's conindid is the referenced table's index and is excluded) for constraint indexes and pg_depend (deptype a/i, relkind S) for column-owned sequences, both sorted and duplicate-free. A name is present whether the server invented it or the desired file stated it — a named constraint's index and an ALTER SEQUENCE … OWNED BY sequence are owned just the same. Schema is required — the caller must inspect the exact schema its absence proof covered, not a search_path resolution; a missing relation returns ErrTableNotFound and a non-table at the name returns ErrNotTable, as the sibling lookups do. Standalone indexes are excluded because their own CREATE INDEX steps report duplicate-name SQLSTATEs, and the table itself because its name is already covered by the absence proof. Integration tests on PostgreSQL 16 cover primary-key, unique, exclusion and named-constraint indexes with serial and identity sequences, foreign keys to another table and to itself, an adopted versus a merely-defaulted-from sequence, the server-suffixed case (t_pkey1, t_id_seq1), a table with no owned relations, a missing table, a view at the name, a same-named table in another schema, a partitioned parent, and the empty-schema error. docs/schemabot-integration.md states what the function is for. This PR adds the read only; the create path's comparison after step 1 stacks on it.

Before / after

Before                                        After

┌──────────────────┐                          ┌──────────────────┐
│ CheckNamesAbsent │ probe: is the name free? │ CheckNamesAbsent │ probe: is the name free?
└────────┬─────────┘                          └────────┬─────────┘
         ▼                                             ▼
┌──────────────────┐                          ┌──────────────────┐
│ CREATE TABLE t   │ server may suffix        │ CREATE TABLE t   │ server may suffix
└────────┬─────────┘ t_pkey → t_pkey1         └────────┬─────────┘ t_pkey → t_pkey1
         ▼                                             ▼
   (nothing reads back                        ┌──────────────────────────┐
    what the table owns)                      │ LookupOwnedRelationNames │ ─► {t_pkey1}, {t_id_seq}
                                              └──────────────────────────┘
                                                (caller compares against the claimed set — next PR)

[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 (1/2 — correctness) — 6f23d254 (3 files, +258/−0)

The set this reads has to match the set statement.ImplicitRelationNames predicts, or the comparison it's built for reports the wrong thing in both directions — so that symmetry is where I started, and it holds. The claim side covers primary-key, unique and exclusion index names plus serial/identity sequence names; the readback covers contype IN ('p','u','x') and deptype IN ('a','i') filtered to relkind = 'S'. Same four sources, same two lists.

t.Run("server suffixes occupied names") (owned_integration_test.go:84) is the right heart for this. It squats both names the server would choose, forces a suffix on each, and then asserts against statement.ImplicitRelationNames(create) directly — so it pins the actual property (t_pkey1/t_id_seq1 come back, and neither claimed name does) rather than the two lists separately. That's the test that would catch this function drifting from the thing it exists to be compared against.

Mutations — 13 run, 10 killed, 3 survivors.

constraint  contype: drop 'x'                         killed
            contype: add 'f'                          killed
            JOIN pg_class i ON i.oid = c.conrelid     killed
            drop ORDER BY i.relname                   killed   ← sortedness is pinned
sequence    deptype: drop 'i'                         killed
            relkind = 'S' → IS NOT NULL               killed
            deptype: add 'n'                          SURVIVED
            refobjsubid > 0 → >= 0                    SURVIVED
            drop the d.classid predicate              SURVIVED
guards      relkind != "r" (drop the 'p' allowance)   killed
            relkind guard removed entirely            killed
            ErrNoRows → return empty, no error        killed
            empty-schema guard removed                killed

The three survivors are all on the sequence arm and they are all defense in depth rather than dead weight — details in Verified below, including why the d.classid one is a soundness requirement even though no test can distinguish it. But one of them points at a test that doesn't exercise what its comment says it does, which is finding 2.

#	Sev	Where	What
1	low	owned.go:59,62	The two 'pg_class'::regclass casts resolve through search_path. Under an explicit pg_catalog demotion the Sequences arm comes back silently empty — a false clean on exactly the window this function closes
2	low	owned_integration_test.go:60-63	The loose sequence is excluded by dependency direction, not by the deptype pair the comment credits. Adding 'n' to that pair keeps the suite green

---

1 — the two ::regclass casts resolve through search_path (low)

owned.go:59 and :62 compare pg_depend's class columns against 'pg_class'::regclass. That cast is name resolution, so it goes through search_path, and pg_depend.refclassid/classid store the real pg_class OID (1259) — so if the cast resolves anywhere else, the predicate matches nothing.

Against postgres:16, with a passthrough view at the shadowing name so every column still resolves and nothing errors:

CREATE VIEW zzevil.pg_class AS SELECT * FROM pg_catalog.pg_class;
SET search_path = zzevil, pg_catalog, public;

Running the query from owned.go verbatim, against a table with a serial PK and a UNIQUE column:

                  relkind   constraint_indexes   sequences
verbatim          r         {t_e_key,t_pkey}     {}           ← silently empty
pg_catalog-qualified  r     {t_e_key,t_pkey}     {t_id_seq}   ← correct

No error, and relkind still reads r, so the ErrNotTable guard passes and the caller gets a clean OwnedRelationNames with one list truthful and the other empty. A server-suffixed t_id_seq1 goes unreported — which is a false clean on the one thing the function exists to catch.

On reachability, which is why this is low and not higher. pg_catalog is searched implicitly before the path unless it is named in it, so the common cases are safe — I checked rather than assuming:

search_path                       'pg_class'::regclass resolves to
zzevil, public                    1259    ← real; implicit pg_catalog wins
"$user", public  (the default)    1259    ← real
zzevil, pg_catalog, public        31880   ← the shadow

dbconn.buildPoolConfig (dbconn.go:162) sets lock_timeout, statement_timeout and application_name and never search_path, so a pool gets the role/database default and is safe today. Reaching this needs a search_path that explicitly names pg_catalog after a user schema — an ALTER ROLE/ALTER DATABASE ... SET search_path someone wrote to make resolution deterministic, on a database pg-sprite doesn't own the settings of.

Worth fixing anyway, because the repo has already made this call. progress.go:228-230 qualifies every catalog name and the operators, with a comment naming this exact hazard:

Every catalog name is pg_catalog-qualified, operators included: a user schema ahead of pg_catalog on search_path could otherwise substitute a pg_cancel_backend that returns true and signals nothing.

and pkg/executor (native.go:804, recover.go:290 and throughout) is uniformly pg_catalog.-qualified with OPERATOR(pg_catalog.=). So this is one line each on :59 and :62 ('pg_catalog.pg_class'::pg_catalog.regclass), plus the four unqualified table references if you want to match the executor's standard.

Two things to keep separate from this. pkg/preflight's existing to_regclass($1) calls (preflight.go:102,155, privileges.go:209) are not the same issue — those resolve a user table name and are deliberately search_path-wide, documented as such at preflight.go:125. And 'pg_class'::regclass is new to this package with this PR (git grep on main finds none in pkg/preflight), so there's no existing convention here to be consistent with — the neighbours to match are pkg/progress and pkg/executor. pkg/schemadiff/introspect.go:163-180 has the same unqualified pattern already; that one is pre-existing and not this PR's to fix, but it's the other place worth a sweep.

And the fix is qualification, not deletion. The d.classid mutation survived, so removing that predicate is green — but see Verified: it's what makes the objid → pg_class join well-defined, and dropping it trades a rare silent miss for a rare silent false positive.

2 — the loose sequence proves the direction, not the deptype pair (low)

The sequence arm's comment says the deptype pair "is the definition of column ownership — 'a' for serial and OWNED BY, 'i' for identity", and the test at owned_integration_test.go:60-63 reads as the case that proves the exclusion half:

Ownership is the OWNED BY relationship, not use: a sequence a column merely defaults from is not owned

Adding 'n' to the pair leaves the whole suite green, so that assertion can't fail for the stated reason. The catalog says why — for the test's own fixture, here is every pg_depend row whose objid is one of these sequences:

   seq   | classid  |  refclassid  | refobj | subid | deptype
---------+----------+--------------+--------+-------+---------
 adopted | pg_class | pg_class     | t      |     2 | a
 adopted | pg_class | pg_namespace |  <ns>  |     0 | n
 loose   | pg_class | pg_namespace |  <ns>  |     0 | n      ← its only row
 t_n_seq | pg_class | pg_class     | t      |     4 | i
 t_s_seq | pg_class | pg_class     | t      |     3 | a

loose has no row pointing at the table at all — its only dependency is on its schema. The DEFAULT nextval('loose') dependency exists, but it runs from the default expression to the sequence (classid = pg_attrdef, refobjid = the sequence), so it never satisfies d.refobjid = t.oid. loose is excluded by the shape of the dependency, and the deptype filter never gets a say.

The inclusion half is genuinely covered — adopted pins 'a' for OWNED BY, t_n_seq pins 'i' for identity, and dropping either kills. It's only the exclusion half that's vacuous, and I don't think it's constructible: a relkind = 'S' relation with a pg_class → pg_class dependency on a table column at some other deptype isn't something you can make from SQL. So this is a comment fix rather than a test to add — the loose case is worth keeping and worth describing as what it actually pins (a sequence a column merely uses is never owned, because using it creates no dependency on the table), and the deptype pair's precision is better explained where the other two survivors are, as guarding the join rather than as the thing separating loose from adopted.

---

Verified — the three survivors, the FK subtlety, the partitioned parent, and the error contract

The three survivors are the predicates that make the objid → pg_class join well-defined, not redundancy. pg_depend.objid is only meaningful together with classid — the same OID value can name a row in any catalog — so JOIN pg_class s ON s.oid = d.objid is unsound without d.classid = 'pg_class'::regclass. The test fixture contains the exact rows that prove it. Every pg_depend row pointing at the table with refclassid = pg_class:

  classid   |  objid  | subid | deptype
------------+---------+-------+---------
 pg_attrdef |  31839  |     1 | a       ← DEFAULT nextval('loose')
 pg_attrdef |  31840  |     3 | a       ← the serial's default
 pg_class   | adopted |     2 | a
 pg_class   | t_s_seq |     3 | a
 pg_type    |  31838  |     0 | i       ← the table's rowtype
 pg_class   | t_n_seq |     4 | i

The two pg_attrdef rows satisfy refclassid, refobjid, refobjsubid > 0 and deptype IN ('a','i') — every predicate the sequence arm applies except classid. They're excluded today because an attrdef OID happens not to collide with a pg_class OID that is also relkind = 'S'; classid is what makes that a guarantee instead of luck. Same story for the pg_type row and refobjsubid > 0. So all three survive because JOIN pg_class + relkind = 'S' absorbs them in practice, and all three should stay.

contype IN ('p','u','x') is complete, and excluding foreign keys is the subtle part the body gets right. Those three are the only constraint types that build an index on the table itself; 'c', 'f', 't' and 'n' have no conindid of their own. A foreign key's conindid is the referenced table's index, which is why selecting by contype rather than by "has a conindid" is the correct discrimination — and the comment says exactly that. The test covers it properly: two FKs into another schema plus a self-referencing one, asserting ["t_pkey"] and explicitly that parent_pkey and parent_code_key don't appear. The self-reference is the good case there, since it's the one where the borrowed index really is a name the table owns and must still not be double-reported.

relkind IN ('r','p') correctly admits partitioned parents. A partitioned parent's PK conindid is a relkind = 'I' partitioned index, and the arm returns its name, which is the name that would collide — so t_pkey comes back for a PARTITION BY RANGE parent, and dropping the 'p' allowance kills. The guard reads relation kind from the same CTE row rather than a second query, so it can't disagree with the OID the arms used.

The error contract matches the siblings, and ErrNoRows is separated from a real query failure — ErrTableNotFound for a missing relation, ErrNotTable (with the relkind in the message) for a view at the name, both wrapped with qualifiedName, and the view test asserts NotErrorIs(ErrTableNotFound) so the two can't collapse. Folding ErrNoRows into a clean empty result kills, which is the fail-closed behaviour that matters here: a caller must not read "table isn't there" as "owns nothing".

One nit: the empty-schema guard at owned.go:33 is the only error in pkg/preflight with no sentinel, and it renders bare table rather than qualifiedName. The test pins what it isn't (NotErrorIs(ErrTableNotFound)) but there's no positive handle, and the doc comment's error contract names the other two shapes and not this one. The compare privileges.go:256 uses fmt.Errorf("%w: schema %s does not exist", ErrTableNotFound, schema) for its analogous case. Since the realistic caller passes the schema from its own absence proof, an empty schema is a programming error and a sentinel buys little — mentioning it only because the doc comment reads as exhaustive.

ORDER BY is load-bearing and pinned (dropping it kills), so the doc's "sorted" claim is enforced. "Duplicate-free" isn't enforced by the SQL — there's no DISTINCT — but it holds by construction: an index backs at most one constraint, and OWNED BY is one column per sequence. I couldn't construct a duplicate from either arm. There is a real asymmetry with the claim side here, though, which I've put in my second comment since it lands on the stacked comparison rather than on this PR.

No build tag on owned_integration_test.go, consistent with the package — all five *_integration_test.go files here are plain preflight_test gated by testutil.StartPostgres(t), so nothing new is being introduced.

Local checks. go build ./..., go vet ./pkg/preflight/, gofmt -l clean. TestLookupOwnedRelationNames green at 6f23d254 (2.7s) and re-green after every mutation was reverted; 14/14 CI green. Probe test removed, owned.go restored from backup, probe schemas dropped, git status --porcelain empty. Leak-checked: the diff is generic PostgreSQL catalog work with invented identifiers (t, parent, loose, adopted, named_uq, sq_pkey).

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

[aparajon]

🤖 Adversarial correctness review (2/2 — invariants, docs, and the stack) — 6f23d254

Invariants

Extends ST-7's discipline to the names an absence proof structurally cannot cover. ST-7 is about identity: the executor "refuses, before anything executes, any statement whose target table does not match the preflight proof it was handed," and ExecuteCreate re-proves every desired statement's target against the absence proof. That covers the name the file states. It cannot cover the names the server invents, because those don't exist at proof time — and 42P07/42710 won't report them either, since the server suffixes rather than failing. This lookup is the readback that makes those names checkable at all, so it's the substrate ST-7's guarantee needs to reach the implicit names rather than a change to what ST-7 says. No *Enforced:* line moves.

OC-1 is the entry finding 1 in my first comment lands on. It forbids converting "engine-state uncertainty" into "a passing/ready/succeeded status", which is precisely what a silently-empty Sequences arm does: the caller can't distinguish "this table owns no sequences" from "I couldn't see them," and the stacked comparison reads both as nothing-to-report. The consequence is narrow — it needs a search_path that explicitly demotes pg_catalog, which pg-sprite never sets — but the direction of the failure is the one OC-1 exists to forbid, which is why a one-line qualification is worth taking even at low severity.

Nothing to add to RF, correctly. Every RF entry is a refusal, and this PR refuses nothing — it's a reader whose only errors are "the table isn't there" and "that isn't a table." The refusal that will eventually cite the RF preamble is the one the create path raises when the two sets disagree, and it isn't here yet.

No new invariant — but this registry has a convention that would accommodate one, so the choice is worth stating. The rule worth pinning is "a create step's server-chosen names are proven against the names the plan claimed", and nothing enforces it in shipped code yet, by this PR's own admission. Deferring the entry to the PR that wires the comparison is defensible. But it isn't the only option here: docs/invariants.md already records rules ahead of their enforcement with a *Planned enforcement:* line — LK-1 carries one for the advisory-lock pool, and ST-6 carries one for "all execution paths in preflight" — so an entry landing now in that form would be consistent with its neighbours rather than aspirational clutter. Either way, the summary is the place to say which was chosen, so the next reader doesn't have to guess whether the registry was considered and skipped or simply not opened.

Docs

The schemabot-integration.md hunk is placed well and, more importantly, doesn't overclaim. It lands directly after the paragraph that admits the open race:

For server-chosen names, the probe narrows the race to the time-of-check window, but nothing catches a name taken inside it.

and closes with "The create path does not yet run that comparison itself." That's the sentence that keeps the section honest — a reader who stops at "so a caller can compare them" would otherwise walk away thinking the window is closed. Keep it in any later edit; it's the load-bearing clause.

The FK carve-out is also called out at the right level of detail for that document ("foreign keys do not contribute the indexes they borrow from the referenced table") — it's the one behaviour of this function an integrator could plausibly guess wrong.

The stack

The claim side can carry duplicates and the readback side can't, so the comparison has to be set-based. This is the one thing I'd want settled before the next PR, because it's a difference the obvious implementation gets wrong. LookupOwnedRelationNames documents its lists as "sorted and duplicate-free" and delivers that. statement.ImplicitRelationNames doesn't dedupe, and PostgreSQL merges identical constraints, so the two lists differ in length for a legal input:

CREATE TABLE t (a int UNIQUE, UNIQUE (a))

claim     ImplicitRelationNames  ->  [t_a_key, t_a_key]     2 entries
readback  LookupOwnedRelationNames -> [t_a_key]             1 entry

Verified both sides: the claim really does return the name twice, and the server really does collapse the two identical UNIQUE constraints into one constraint with one index (conname = t_a_key, one row in pg_indexes). So a length check, a positional walk, or a "every claimed name has a readback slot" comparison reports a phantom missing name on a table that is in fact exactly as claimed. A set comparison is clean: {t_a_key} == {t_a_key}.

The reason this is worth flagging rather than obvious is that ST-8 deliberately establishes a positional contract for the create path — statements ordered at construction so "the position mapping between a greenfield plan's statements and the create path's step verdicts holds by construction." The surrounding code is positional by design, so reaching for the same shape here is the natural move, and it's the wrong one for these lists. Deduping the claim side before comparing (or comparing sets and reporting the difference both ways) is the fix, and it's cheap to build in from the start.

One thing the comparison will need that this PR can't give it. The lookup is a point-in-time read, so a comparison built on it proves the names are right at readback, not that they stayed right. That's fine for the collision case — the names it's checking were fixed the moment the create committed — but it means the readback has to happen inside whatever ownership the create step already holds rather than as an independent follow-up query, or the next PR reintroduces a window of its own. LK-1 is the intended holder, though its own enforcement is still marked planned, so "reuse the create step's ownership" is the constraint to write down now rather than a lock to lean on yet.

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

[aparajon]

🤖 Approving at 6f23d254. The symmetry with statement.ImplicitRelationNames holds, the suffix test pins the property this exists for by cross-checking the two sides directly, and 10 of 13 mutations died — the three survivors are defense-in-depth on the sequence arm, not gaps.

Two low findings in my first comment (the 'pg_class'::regclass casts resolving through search_path; the loose case not exercising the deptype pair its comment credits) and the claim/readback duplicate asymmetry in my second — none of them block.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/83, follow-up commit

Seven of eight findings fixed in the follow-up commit; one (the sequence-arm predicates' negative cases) is answered rather than changed because no DDL can reach the mutant. Both surviving mutants that are reachable — FK leak and the empty-schema guard — were reintroduced and now fail.

#	Concern	Status	Explanation
1	JOIN pg_constraint … conindid <> 0 also matches foreign keys, injecting the referenced table's index names (and a self-FK's duplicate PK) into ConstraintIndexes	fixed	Predicate is now c.contype IN ('p', 'u', 'x') — the only constraints that build an index on the table itself — with a comment stating why conindid alone is wrong. New subtest "foreign keys borrow the referenced table's indexes": FKs to another schema's PK and UNIQUE plus a self-reference; expects exactly {t_pkey} and asserts parent_pkey/parent_code_key absent. Reintroduced mutant (back to conindid <> 0) fails that subtest
2	A view/matview/foreign table at the name is reported as ErrTableNotFound, conflating two causes; siblings use ErrNotTable	fixed	relkind is selected rather than filtered in the CTE; the Go side returns ErrNotTable with the relkind for anything but r/p. New subtest "view at the name" asserts ErrorIs ErrNotTable and NotErrorIs ErrTableNotFound
3	The empty schema subtest cannot fail for the right reason — deleting the guard leaves it green	fixed	Subtest now also asserts NotErrorIs(err, ErrTableNotFound), pinning the typed outcome rather than prose (ErrorContains would be a text assertion, which the repo's test rules avoid). Reintroduced mutant (guard deleted) fails
5	"server-chosen … owned by" overstates the result: user-named constraints and OWNED BY sequences are returned too; duplicates not mentioned	fixed	Type and function comments rewritten around ownership (present whether the server invented the name or the desired file stated it; sorted and duplicate-free). New subtest "sequences owned by columns": a DEFAULT nextval() on a non-owned sequence is excluded, an ALTER SEQUENCE … OWNED BY sequence is included. docs/schemabot-integration.md and the PR title/body updated to "owned", with the FK exclusion stated
4	Sequence-arm predicates have no negative case: dropping refobjsubid > 0 or the deptype filter survives	rejected	No DDL produces a pg_depend row from a sequence to a table with deptype outside a/i, or with refobjsubid = 0 — a DEFAULT nextval() dependency runs from the attrdef to the sequence, not the reverse (the new "sequences owned by columns" subtest shows that case is excluded regardless). Killing the mutant would need a hand-inserted catalog row under allow_system_table_mods, which is not a test the suite should carry. The predicate pair is documented in the query comment as the definition of column ownership rather than a filter over reachable inputs
6	c.conindid <> 0 is inert given the pg_class join	fixed	Removed; subsumed by the contype filter
7	The assert.NotEqual in "server suffixes occupied names" is a tautology	fixed	Replaced with a per-claimed-name NotContains over both lists, sensitive to a missing-suffix regression
8	One DDL, two literals: const create is not the statement executed	fixed	Single create string feeds both Exec and ImplicitRelationNames. table == "" stays unguarded, as the review allowed — schema == "" has search_path meaning in the siblings, an empty table name has none and simply finds no row

Verification on the follow-up: gofmt -l clean; go build ./... and go vet clean; make lint 0 issues; go test -race ./pkg/preflight/ ok against PG 16 (all TestLookupOwnedRelationNames subtests including the three new ones); SKIP_INTEGRATION=1 go test ./... ok. The branch carries the main merge (post-#79) that was already on the remote; the review fixes rebased onto it without conflict.

Agent review at head df32e60c