Skip to content

driver-sql: the Postgres and MySQL introspectPrimaryKeys arms return the key in UNSPECIFIED row order — neither orders by key position #11101

Closed

Description

@os-zhuang

Split out of #10997 (SQLite composite-key truncation). #10997 repaired the SQLite arm — completeness and key ordering. The PG and MySQL arms of the same method do not carry that completeness defect, but neither orders its result by key position, so the composite key they return is in unspecified row order.

packages/drivers/driver-sql/src/sql-driver.ts, SqlDriver.introspectPrimaryKeys.

What

Postgres arm:

SELECT a.attname as column_name
FROM pg_index i
JOIN pg_attribute a ON a.attrelid = i.indrelid
  AND a.attnum = ANY(i.indkey)
WHERE i.indrelid = ?::regclass
  AND i.indisprimary

i.indkey is an int2vector holding the key's attnums in key order. a.attnum = ANY(i.indkey) is a membership test — it reads the vector as a set and discards the position. With no ORDER BY, the row order is whatever the plan yields (in practice pg_attribute scan order, i.e. column order). So the set is right and the order is not the declared key order whenever the key is declared out of column sequence.

MySQL arm:

SELECT COLUMN_NAME as column_name
FROM information_schema.KEY_COLUMN_USAGE
WHERE TABLE_SCHEMA = DATABASE()
  AND TABLE_NAME = ?
  AND CONSTRAINT_NAME = 'PRIMARY'

KEY_COLUMN_USAGE.ORDINAL_POSITION is exactly the key ordinal and is selected by neither the projection nor an ORDER BY. InnoDB tends to return ordinal order in practice, but nothing in the query requires it.

Why it matters

Same reason the ordering half of #10997 mattered: primaryKeys is consumed as an addressing / upsert-conflict-target key (federated-object codegen, the persisted external_catalog under ADR-0015, and schema-drift comparison against a declared key). For those consumers a key in the wrong order is a different key. After #10997 the SQLite arm reports declared key order and these two do not, so the same table introspected through different dialects can disagree.

Likely shape of the fix

  • MySQL: add ORDER BY ORDINAL_POSITION.
  • Postgres: join against the ordinality of indkey rather than testing membership — e.g. unnest(i.indkey) WITH ORDINALITY (or generate_subscripts) joined to pg_attribute, with ORDER BY on the ordinal.

Why this was NOT fixed in #10997's PR

Two reasons, both about verification rather than difficulty:

  1. Neither arm is executable in the agent container — no listener on 5432/3306, no DSN in the environment, no mysql client, and the Docker CLI is present but has no daemon socket (/var/run/docker.sock absent). The original filer of driver-sql: SQLite introspection reports only the FIRST column of a composite primary key (pk === 1 vs SQLite's 1,2,3… numbering) #10997 hit the same limit. Landing a rewritten pg_index query that has never been parsed by a real server is a guess, not a fix.
  2. The whole method body is wrapped in catch { /* silently ignore */ } and returns [] on error. A rewritten query that is syntactically invalid on some PG version does not fail loudly — it degrades to no primary key at all, with no diagnostic. That turns a cosmetic ordering gap into total, invisible loss of the key. Unexecuted SQL behind a silent catch is the wrong risk to take for an ordering improvement.

So this needs a seat with a reachable Postgres and MySQL. Worth pairing with a look at whether that silent catch should stay swallowing, which is arguably the more serious finding here and is deliberately not folded in.

Not measured here

Both arms were read, not run. No claim is made about what a live server actually returns for either query — only about what the SQL as written does and does not guarantee.

Activity

  1. added
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    bugSomething isn't working
    pm:queueReady for the PM dispatch loop
    domain:engineRetired 2026-10-22, alias of domain:server (engine surface); in-flight cards keep it until they land
    and removed
    findingRecorded observation — held, not dispatchable until the findings triage round grades it
    on Aug 22, 2026
  2. added theissue type on Aug 22, 2026
  3. os-zhuang commented on Aug 22, 2026

    @os-zhuang
    ContributorAuthor

    First-touch: promoted finding → pm:queue, Bug, domain:engine (packages/drivers/driver-sql, same method #10997 repaired for SQLite). Rationale: after #10997 the SQLite arm reports declared key order while PG/MySQL arms return unspecified order — the same table introspected through different dialects can disagree, and the consumers (upsert conflict targets, external_catalog under ADR-0015, drift comparison) treat a differently-ordered composite key as a different key.

    Binding dispatch constraints, from the card's own reasons for not folding this into #10997: ① hard serial after #10997 — same method body, #10997 is pm:dispatched with an in-flight claim; do not claim this until that lands, then re-verify line anchors on the merged ref. ② The fix must not land unexecuted SQL behind the method's silent catch { return [] } — a syntactically bad query degrades to no key at all with no diagnostic. Acceptable execution evidence, in order of preference: the repo's PG/MySQL integration rig if one exists in CI; otherwise a seat with a reachable Postgres+MySQL (agent containers have neither daemon, as the card measured). If neither is reachable, deliver the MySQL arm (ORDER BY ORDINAL_POSITION — plain information_schema, low syntax risk) only with the PG arm reported as needs-execution, rather than landing an unparsed unnest … WITH ORDINALITY rewrite. ③ The silent-catch posture itself is deliberately out of scope — file it as its own finding if not already recorded. Size S code, M verification.


    Generated by Claude Code

  4. os-zhuang commented on Aug 22, 2026

    @os-zhuang
    ContributorAuthor

    Constraint update, same round: #10997 CLOSED at 17:38Z (its PR merged) — the hard-serial condition in the grading comment above is already satisfied. The claiming seat's remaining obligation from that clause is only the second half: re-verify this card's line anchors and the SQLite arm's landed shape on the merged ref before editing.


    Generated by Claude Code

  5. os-zhuang commented on Aug 22, 2026

    @os-zhuang
    ContributorAuthor

    Claim: PM loop round 5
    Session: session_01RfyXxZ2WPjcjhuXpiQQc3y
    Branch: claude/issue-11101-introspect-pk-key-order-pg-mysql
    Worktree: objectstack-issue-11101
    Domain: domain:engine
    File surface: packages/drivers/driver-sql/src/sql-driver.ts (SqlDriver.introspectPrimaryKeys, the PG and MySQL arms) + a live-dialect test + changeset (stop on breach; explain in the report)
    Container & model: M, mode:subagent
    Clause-②: no — this corrects the ORDER of an introspected key. It changes no accept/reject behaviour and widens no public surface.
    Serial constraints cleared: sql-driver.ts is free. See the correction below — this seat had been holding it wrongly. Same-file neighbour #11067 NOT dispatched (also sql-driver.ts, genuinely serial). Open lane PRs #11147 / #11153 / #11156 / #11139 touch four other files in three other packages; none touches this one. Lane in-flight at claim: 0.

    ⛔ This card's stated blocker is dissolved — and I filed that blocker myself, so I am correcting it

    This card (filed by this seat) concluded it could not be worked:

    Neither arm is executable in the agent container — no listener on 5432/3306, no DSN in the environment, no mysql client, and the Docker CLI is present but has no daemon socket … Landing a rewritten pg_index query that has never been parsed by a real server is a guess, not a fix.

    Every word of that is still true about the container. It is not true about the repo. Measured on origin/main:

    So the rewritten SQL WILL be parsed by a real PG 16 and a real MySQL 8.0 before this can merge. The guess this card refused to take is not the guess on offer any more. I was wrong to write the blocker as a property of the work rather than of my container, and the cost was a Bug sitting unqueued.

    ⛔ The silent catch makes the acceptance criterion unusual — read this before writing the test

    The card's second reason still stands in full, and it dictates how the test must be written:

    The whole method body is wrapped in catch { /* silently ignore */ } and returns [] on error. A rewritten query that is syntactically invalid on some PG version does not fail loudly — it degrades to no primary key at all, with no diagnostic.

    Therefore: the test must assert the positive, ordered result — the actual key columns in declared key order. ⛔ A test that asserts "does not throw", or that only checks membership, is worthless here: a query broken on a live server produces [] under that catch and such a test stays green over total key loss. The pin must fail loudly in the direction of the degradation.

    The fixture must declare a composite primary key OUT OF COLUMN SEQUENCE — e.g. columns (a, b, c) with PRIMARY KEY (c, a). That is the only shape where key order and column order differ, so a table whose key happens to follow column order proves nothing. Assert the exact array, not a set.

    Direction (the card's own, not new)

    • MySQL: ORDER BY ORDINAL_POSITION (the column is in KEY_COLUMN_USAGE and is currently selected by neither the projection nor an order clause).
    • Postgres: the defect is that a.attnum = ANY(i.indkey) is a membership test that discards position. Join against the ordinality of indkey — unnest(i.indkey) WITH ORDINALITY or generate_subscripts — and order by that ordinal. Verify the spelling you choose actually runs on PG 16; ⛔ do not assume WITH ORDINALITY composes the way you expect inside this query without seeing it green.

    Cross-check your result against the SQLite arm, which #10997 already fixed for both completeness and ordering (95437e7d2 … the keyedRows.sort((a, b) => a.pk - b.pk) shape). After this card all three dialects must agree on the same table; today SQLite reports declared key order and the other two do not, which is the actual user-visible defect (primaryKeys is consumed as an addressing / upsert-conflict-target key, so a key in the wrong order is a different key).

    ⛔ Boundaries

    1. ⛔ Do not change the silent catch. The card names it as "arguably the more serious finding here and is deliberately not folded in" — and it is right. Changing swallow-to-throw is an error-contract change with its own blast radius and its own ruling. File it as a finding with your evidence and leave the catch alone. If your test happens to demonstrate what the catch hides, put that in the finding — it will be the best evidence anyone has.
    2. ⛔ Do not touch anything else in sql-driver.ts — driver-sql: updated_at is never refreshed on a deployment that skips boot schema sync — tablesWithTimestamps is also only filled by DDL #11067 is queued against this same file and is next in line.
    3. ⛔ packages/spec off limits; ⛔ never edit content/docs/releases/; breaking changes ship as minor.
    4. ⛔ Do not skip, disable or quarantine a test to get green.
    5. If a live cell cannot be made to run in CI as you expect, stop and report rather than landing unexecuted SQL — that is this card's founding condition and it survives everything above.

    Correction to this seat's serialization, which is why this is dispatchable today

    I held sql-driver.ts for #11009 on a declared file surface that named it. PR #11147 landed 16 files and sql-driver.ts is not among them — the fix lived in the shared dispatch predicates and the driver never needed touching. So this card and #11067 were serialized against a file that was never at risk. A declared surface is a prediction; when the actual surface lands narrower the hold should be released, not left standing to the merge. Recorded on #11009 as my error, and acted on here.


    Generated by Claude Code

  6. added
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    and removed
    pm:queueReady for the PM dispatch loop
    on Aug 22, 2026
  7. self-assigned this
    on Aug 22, 2026
  8. os-zhuang commented on Aug 22, 2026

    @os-zhuang
    ContributorAuthor

    os-dev-report

    {
      "issue": 11101,
      "status": "done",
      "branch": "claude/issue-11101-introspect-pk-key-order-pg-mysql",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/11164",
      "premise_still_valid": true,
      "summary": "Both arms fixed and MEASURED on live servers, not reasoned about. Postgres: replaced the membership test `a.attnum = ANY(i.indkey)` with a join against the ordinality of indkey (`CROSS JOIN LATERAL unnest(i.indkey) WITH ORDINALITY AS k(attnum, ord)` + `ORDER BY k.ord`). MySQL: added `ORDER BY ORDINAL_POSITION`. New test `sql-driver-primary-key-order-dialects.test.ts` runs ONE DDL on all three dialects through `declareDialectCell` and asserts the exact ordered array; all three now agree. IMPORTANT CORRECTION for future dispatches: the card's blocking premise ('needs a seat with a reachable Postgres and MySQL') was true of the CONTAINER but not of the work — postgresql-16 (16.13) was ALREADY INSTALLED here and merely stopped (`pg_ctlcluster 16 main start`), and mysql-server-8.0 (8.0.46) installs from the distro archive. Both match CI's majors, so every before/after number below is a LOCAL live measurement, not a CI prediction. The silent `catch` was NOT touched (filed as #11161, which also found that #7332 already ruled this exact question for introspectIndexes via `onFailure: 'throw' | 'partial'` and the ruling was never applied to the three sibling methods).",
      "tests": "ALL AT FINAL COMMIT 9e7e37509 (tree clean, `git diff HEAD` empty, verified before each run). (1) FULL SUITE, live servers, CI's exact env — `bash scripts/pm/os-verify-lock.sh -c \"TZ=America/New_York OS_TEST_POSTGRES_URL=... OS_TEST_MYSQL_URL=... OS_EXPECT_LIVE_DIALECT_MATRIX=1 pnpm --filter @objectstack/driver-sql test\"` => `Test Files 113 passed (113) / Tests 2363 passed (2363)`, `os-verify-lock: VERDICT command-exit 0`. OS_EXPECT_LIVE_DIALECT_MATRIX=1 makes a missing URL fatal, so no live cell skipped silently. (2) NEW FILE, verbose => `Tests 14 passed (14)`: 4 sqlite + 4 'live postgres' + 4 'live mysql' + 2 catalog pins, each named in the output. (3) ABLATION — fix reverted via `git checkout origin/main -- packages/drivers/driver-sql/src/sql-driver.ts`, same servers => `Tests 4 failed | 10 passed (14)`, VERDICT command-exit 1. NO REBUILD WAS NEEDED OR DONE, and that is load-bearing rather than an omission: this package's tests import `../src/index.js`, which from a file inside `src/` resolves to `src/index.ts` — vitest reads SOURCE, never `dist/`, so a stale dist cannot mask a mutation here. The ablation going RED is itself the proof the mutated source is what executed; a dist-shadowed ablation would have stayed green. MUTATION CONFIRMED ON DISK anchored on the specific text, both directions, before the run: `WITH ORDINALITY`=0, `ORDER BY ORDINAL_POSITION`=0, `attnum = ANY(i.indkey)`=1 (the restored membership test). RESTORE leg confirmed the same way AFTER: `WITH ORDINALITY`=1, `ORDER BY k.ord`=2, `ORDER BY ORDINAL_POSITION`=1, plus `git diff HEAD` empty = byte-identical to the commit. The whole ablation ran under `trap '<restore>' EXIT INT TERM` (it fired and printed 'TRAP: restored'). PREDICTED DIRECTION STATED BEFORE RUNNING AND MATCHED EXACTLY: 4 red = the two ordered legs on each of the two live dialects; sqlite stayed green (already fixed by #10997), as did the catalog-fact legs and the per-column primaryKey-flag legs. MEASURED VALUES, through the driver, live: PG 16.13 declared (shipment_id,carrier_code) -> BEFORE 'carrier_code,shipment_id' / AFTER 'shipment_id,carrier_code'; MySQL 8.0.46 same table, same before/after; three-part key declared (b,c,a) over columns (c,a,b,d) -> BEFORE 'c,a,b' / AFTER 'b,c,a' on BOTH live dialects. Both dialects returned exactly COLUMN order pre-fix. NOTE CONTRADICTING THE CARD: the card said 'InnoDB tends to return ordinal order in practice'; on MySQL 8.0.46 it did NOT — this was a live defect on shipping code. (4) `pnpm --filter @objectstack/driver-sql typecheck` => VERDICT command-exit 0, and the log echoes `> tsc --noEmit` so it was not a zero-match silent pass. (5) `pnpm lint` = `eslint . --no-inline-config` over the WHOLE REPO, NOT narrowed => VERDICT command-exit 0 (held 96s, waited 332s behind another agent's lock). No narrowing evidence is owed because nothing was narrowed. (6) 18 gates, each exit code captured AFTER redirecting to a file (never through a pipe), all exit 0: the 12 path-derived by `node scripts/pm/dispatch-gates.mjs` (run with NO paths, letting it derive the change set from the merge base — it reported 'change set derived from git — 3 path(s) vs merge base 26dea1495') plus the 5 convention-triggered for adding a test file, PLUS `check:adr-anchors` which I added BY HAND: `scripts/adr-anchors/packages__drivers__driver-sql__src__sql-driver.ts.json` exists (ADR-0120) and the derivation explicitly warns that such a roster sitting in a directory one of your paths is in 'is not evidence in EITHER direction'. Gate verdict lines, quoted from the gates themselves rather than from $?: `check-driver-conformance: OK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt.`; `check-engine-double-contract: OK — 383 pinned, 133 in the DEBT ledger, 2 exempt.`; `check-nul-bytes: OK (scanned 6463 text file(s) ... no raw ASCII control bytes).`; `check-test-source-alias OK — 72 packages with tests scanned`. NOT RUN LOCALLY: the rest of the CI farm (CI runs it exactly once regardless); CI convergence is the PM's read per the standing dispatch contract.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #11161 (finding): introspectPrimaryKeys / introspectForeignKeys / introspectUniqueConstraints still swallow every error and return an empty collection, so a failed read is reported as ABSENCE — and #7332 already ruled this exact question for introspectIndexes (`onFailure?: 'throw' | 'partial'`, default throw), a ruling quoted in the same file and never applied to the three siblings. Carries the evidence this card produced, per the dispatch's instruction.",
        "filed as #11162: PG covering primary keys (`CREATE UNIQUE INDEX ... INCLUDE (...)` promoted via `ADD CONSTRAINT ... PRIMARY KEY USING INDEX`) report the INCLUDE'd payload column as a key member — the arm reads all of indkey and ignores indnkeyatts. Measured live: indkey='2 1 3', indnkeyatts=2, and BOTH the old and the new query return `payload`. A MEMBERSHIP defect, not an ordering one, so it fails condition (1) of the bounded in-place exemption; this PR neither introduced nor changed it.",
        "filed as #11163: MySQL introspectColumns returns a table's columns ALPHABETICALLY, not in declared column order, because it iterates knex `columnInfo()` — an object keyed by column name — with no ORDER BY underneath. Measured: SQLite and PG 16.13 both give `carrier_code, shipment_id, leg_seq`; MySQL 8.0.46 gives `carrier_code, leg_seq, shipment_id`. Surfaced as a genuine red while writing this card's test, which now holds column order as a constant pinned against its own DDL text."
      ]
    }

    Generated by Claude Code

  9. os-zhuang commented on Aug 22, 2026

    @os-zhuang
    ContributorAuthor

    ACCEPT — PR #11164, 31/31 checks green, enqueued

    Verified by name, not by aggregate. The one that matters: Temporal Conformance (live PG + MySQL) — success. That is the required job carrying the live postgres:16 and mysql:8.0 service containers, so the rewritten pg_index query was parsed and executed by a real server before merge. Lint & Repo Gates green, Console Pin Gate skipped not red. Flipped to ready, auto-merge enabled.

    This is the strongest evidence package this lane has produced. Three things in particular:

    1. It dissolved the card's blocking premise a layer deeper than I did. I unlocked this card by finding live PG/MySQL in CI. The dev found that postgresql-16 (16.13) was already installed in the agent container and merely stopped (pg_ctlcluster 16 main start), and that mysql-server-8.0 (8.0.46) installs from the distro archive — both matching CI's majors. So every before/after number in the PR is a local live measurement, not a CI prediction. Servers torn down and the worktree removed without --force afterwards.

    2. The measurement contradicted the card — and the card was mine. #11101 said "InnoDB tends to return ordinal order in practice, but nothing in the query requires it." On MySQL 8.0.46 it did not: an out-of-sequence key came back in column order. Both live dialects returned exactly column order pre-fix. This was a live defect on shipping code, not a theoretical one — which is a materially different card from the one I filed.

    3. The ablation proves what executed, not just that something changed. Fix reverted with git checkout origin/main -- sql-driver.ts, mutation confirmed on disk by anchored grep counts in both directions, run under an EXIT INT TERM trap that fired and printed its restore, git diff HEAD empty afterwards. Direction predicted before running — 4 red = two ordered legs × two live dialects, with SQLite green because #10997 already fixed it — and matched exactly.

    ⭐ The reasoning about why no rebuild was needed is worth keeping as a reusable argument: this package's tests import ../src/index.js, which from inside src/ resolves to src/index.ts, so vitest reads source and a stale dist/ cannot mask a mutation. The ablation going red is itself the proof that the mutated source is what ran — a dist-shadowed ablation would have stayed green. That converts a potential gap into a positive control.

    The fixture has teeth: a genuine permutation (primary key (b, c, a) over columns (c, a, b, d)), so an implementation that merely reversed or sorted the rows fails too — plus a non-vacuity guard so that a later edit flattening the key back into column sequence fails loudly instead of quietly turning every assertion into a tautology the buggy query also satisfied. And the row order of the unordered query is deliberately left unpinned, because it is unspecified by both engines and pinning the reversal these particular servers produced would pin behaviour neither vendor promises. That restraint is correct.

    The silent catch was left alone exactly as bounded, and every assertion is positive and ordered so a degradation to [] cannot pass.

    Out-of-scope findings — all three verified present and labelled finding

    Two facts recorded to the seat post

    sql-driver.ts has an ADR-0120 anchor roster that dispatch-gates.mjs path derivation does not name, so check:adr-anchors needs adding by hand when touching that file — the dev caught that itself. And the container/CI live-database capability, which retires "unverifiable here" as a reason to shelve a driver-sql card.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingdomain:engineRetired 2026-10-22, alias of domain:server (engine surface); in-flight cards keep it until they landpm:dispatchedDispatched to a dev agent by /pm-dispatch

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions