fix(pg-meta): order primary key columns by key position and exclude INCLUDE columns - #382
Open
Rjabov wants to merge 1 commit into
Open
fix(pg-meta): order primary key columns by key position and exclude INCLUDE columns#382Rjabov wants to merge 1 commit into
Rjabov wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #381
What kind of change does this PR introduce?
Bug fix: data-integrity issue in
list_tables(verbose) primary key output.What is the current behavior?
primary_keysis sorted by each column's physical position in the table rather than its position in the key, and it includes the non-keyINCLUDEpayload columns of the backing index.membership(user_id, org_id)["org_id","user_id"]inc(a) INCLUDE (b)["a","b"]Root cause in
pg-meta/tables.sql:a.attnum = any (i.indkey)is a set-membership test, so each column's position within the key is discarded and the enclosingjsonb_agghas noorder by.indkeyalso spansindnatts, which includes theINCLUDEpayload, rather thanindnkeyatts.This is the same defect #317 fixed in the foreign key subquery immediately below, which walks columns with
unnest(...) with ordinalityand aggregatesorder by cols.ord. The primary key subquery was not updated at the time.What is the new behavior?
indkeyis unnestedwith ordinalityand the aggregate is ordered by that ordinal, so columns come back in constraint-definition order - not attnum, not alphabetical.indnkeyatts, soINCLUDEpayload columns are excluded.to_jsonb(_pk) - 'ord'emits exactly the same keys as before.= any (indkey)is wrong, so it doesn't get "simplified" back.Two regression tests, both using a key order that is deliberately the reverse of the physical column order so any regression to attnum ordering fails immediately - the same shape as the FK ordering test added in #317.
Verification
Windows 11, Node 22 LTS, pnpm 10:
Both new tests pass:
The 5 failures are pre-existing and unrelated to this change. They are all
normalizeFilename:edge-function.tsimportsresolvefromnode:path, which is platform-dispatched, but strips a hardcoded POSIX prefix (/tmp/user_fn_.../), so on Windows the strip never matches and the fullC:\tmp\...path is returned. 3 direct failures inedge-function.test.tsplus 2 downstream inserver.test.ts(list edge functions,get edge function). I confirmed the identical 5 failures on cleanupstream/mainwith nothing applied, so they are the baseline, not a regression. Happy to file that separately -node:path/posixlooks like the one-line fix, and every caller already forces POSIX semantics withfileURLToPath(..., { windows: false }).End to end against a real MCP client and server (
createSupabaseMcpServeroverStreamTransport, PostgreSQL 16.4 via PGlite), callinglist_tableswithverbose: true:Single-column primary keys and the composite foreign key output from #317 are unchanged on both builds.
On whether this actually misleads a model
I tested it rather than asserting it, and the result is mostly negative, so it's worth stating plainly: across 18 runs (3 models × 3 questions × before/after, one replicate per cell) 17 answered correctly on both builds. Models generally did not trust
primary_keys- 17 of 18 runs calledexecute_sqland readpg_index/pg_constraintdirectly. Two runs spontaneously flagged the output as wrong, e.g. "list_tables reports the primary key as ["a","b"], which is misleading: it lumps the INCLUDE column in with the real key column."The one failure was a weaker model on "what's the primary key on membership?" against the unfixed build, which answered
org_id, user_id. Notably its own verification query usedinformation_schema.key_column_usagewithoutorder by ordinal_position, so it confirmed the wrong answer.So the case for this fix is not "agents give wrong answers". It's that
primary_keyscontradicts the database, that models are paying for a catalog round-triplist_tablesexists to save, and that a non-agent consumer of the published package has no fallback at all.What I did not test
indnkeyattsrequires PG 11+.list_tablesmore than these did.