fix(pg-meta): pair composite FK columns positionally to avoid cartesi…#317
Conversation
|
Hi @gregnr : no rush on this, but I wanted to check whether you see this as a real issue before it goes further. The short version: for composite foreign keys, list_tables (verbose) returns the cartesian product of columns (N² pairings instead of N), so it reports column relationships that don't exist in the schema , which an AI agent could silently trust. There's a minimal repro in the PR. I might be missing context on why it works this way, so I'd genuinely value your read: is this a bug worth fixing, or intended behavior? Happy to adjust or close if I've misunderstood something. |
There was a problem hiding this comment.
@anp0429 thanks for the PR and the ping! I just tried this and repro'd the bug on my end - nice find. The fix looks good, though one thing I noticed - for a composite FK, foreign_key_constraints is outputting one row per column pair vs a single row that represents captures both columns. If we keep it as is, nothing really signals that 2 rows belong to one atomic constraint vs 2 independent FKs.
I think we'll likely want to group within a single constraint instead - something like:
{
"name": "child_parent_fk",
"source_columns": ["a", "b"],
"target_table": "public.parent",
"target_columns": ["a", "b"]
}so that the composite is explicit. What do you think?
|
@gregnr Yeah, agreed : grouping is the right call. The per-pair rows kind of have the same problem as the original bug, just one level up: nothing tells you those two rows are one atomic constraint. |
…an product The foreign-key subquery in tables.sql joined source and target columns independently (attnum = any(conkey) with attnum = any(confkey)), producing the cartesian product of a composite key's columns. An N-column foreign key yielded N^2 relationship rows, reporting column pairings that do not exist in the schema. This surfaces in list_tables (verbose) foreign_key_constraints. Pair the columns positionally with unnest(conkey, confkey) with ordinality so column i maps only to column i. Adds a regression test.
c47537d to
d17cdfb
Compare
|
@gregnr Went ahead and pushed the grouped shape so there's something concrete to review - went with unnest(conkey, confkey) with ordinality so the positional pairing is structural, plus the non-alphabetical column-order test mentioned above. For the single-column question I defaulted to one-element arrays for a uniform shape, but it's a two-line change either way if you'd rather keep those as-is. |
|
To sanity-check the grouping change beyond the committed tests, I ran an automated review over this branch : an LLM proposes edge-case tests, then a deterministic gate actually executes each one against the code (no model in the pass/fail path). 9 additional cases, all passing:
0 gaps found. Happy to commit any of these as tests if you want the coverage in the suite. |
|
@anp0429 this is great work, nice job. All of those additional test scenarios look valuable - please commit them if you can! I'll merge this once those are in. We'll keep it as you have it where the FK columns are always arrays, even for non-composite FKs. |
…-PK unique ref, multi-FK, 3-column, single-column arrays)
|
Done, added all of those: self-referential, cross-schema, non-PK unique reference, multiple FKs between the same tables, three-column ordering, and single-column arrays. Full suite green (110/110). Thanks for the review, glad this is landing. |
|
Fixed the Biome formatting. The remaining failures look unrelated to this PR, the e2e tests are failing on x-api-key header is required (missing API key in CI), and the Management API types check is flagging pre-existing upstream type drift. My unit tests (including the new FK ones) pass. Let me know if you'd like me to do anything else. |
|
Merged. Thanks again @anp0429! |
|
Thanks @gregnr, glad it was helpful. Enjoyed working through this one. |
What kind of change does this PR introduce?
Bug fix: data-integrity issue in
list_tables(verbose) foreign-key output.What is the current behavior?
For any composite (multi-column) foreign key,
list_tables(verbose) returns the cartesian product of the source and target columns. An N-column FK produces N²foreign_key_constraintsentries instead of N : reporting column pairings that do not exist in the schema.Repro:
list_tableswithverbose: truereturns 4 constraints forchild_parent_fk:Only
a=>aandb=>bare real. Because this tool exists so an AI agent can reason about database structure, the fabricated relationships are silently trusted - an agent could infer key relationships that don't exist.Root cause is in
pg-meta/tables.sql. The FK subquery joins source and target columns independently:any(conkey)againstany(confkey)cross-joins every source column with every target column, instead of pairing them positionally.What is the new behavior?
Each foreign key is now returned as one constraint object with the grouped shape discussed in the review below:
{ "name": "child_parent_fk", "source_table": "public.child", "source_columns": ["a", "b"], "target_table": "public.parent", "target_columns": ["a", "b"] }unnest(conkey, confkey) with ordinality, so wrong pairings are impossible by construction, and both arrays aggregate ordered by constraint ordinality -source_columns[i]pairs withtarget_columns[i], in constraint-definition order (not attnum, not alphabetical).source_tableis kept in the output becauserelationshipsincludes constraints where the listed table is the referenced target.foreign key (b, a) references parent (y, x)) so any future regression to attnum or name ordering fails immediately.Additional context
The same
pg-metaquery backs the schema-docs work in #278, so this fix helps that path as well.