get_pg_top_queries: make it parse, and stop rounding the PostgreSQL int8 identities (#2554, #2548)

[erikdarlingdata]

Fixes #2548. Fixes #2554.

Both are queryid defects in get_pg_top_queries, and #2554's touches the same statement, so they are taken together.

---

#2554 — the read had never returned a row, on any engine

Defect 1: the SQL did not parse

[#2219] added LEFT JOIN collect.pg_statement_text AS t so the read could carry statement text. That put t.queryid in scope beside differenced.queryid and made the unqualified references ambiguous. 42702 is a parse-time error, so the read threw on every call from #2219 onward — on Aurora, the one engine it exists for, as much as anywhere — and a store with zero rows failed exactly the way a full one did.

Reproduced against a real PostgreSQL by PREPAREing the shipped constant (dumped from the built assembly, not retyped):

FAILS    DarlingPgStatementReader.PgTopQueriesSql
           ERROR:  column reference "queryid" is ambiguous
           LINE 27:     queryid,

The one-word fix in the issue does not work, and that was measured rather than assumed. Qualifying only the GROUP BY — the repair the report proposes — still fails, on the select-list queryid, which is the reference PostgreSQL names first:

variant	result
shipped	FAILS 42702 ... LINE 27: queryid,
GROUP BY differenced.queryid only	FAILS 42702 ... LINE 27: queryid,
both references qualified	PARSES

Defect 2: the capability gate was downstream of the throw

The gate sat inside if (rows.Count == 0), so it could only speak when the query succeeded and returned nothing. On stock PostgreSQL — where pg_statement_stats can never run, its AppliesTo being Aurora-only — the parse error surfaced as a raw SQL error where the sibling get_pg_wait_stats gives the honest "does not run on that engine … and never will".

Fixed by consulting the gate on the throw path, deliberately NOT by moving it ahead of the read. DarlingEngineCapability's own contract is explicit on this:

Called only on the miss path. Every call site checks capability after its read came back empty, never before it. … a server whose registry row says one engine while its collected rows say another (a re-registration, a restored database) still gets its DATA rather than a confident explanation of why it cannot have any.

Moving it first would trade this defect for that one, across every read. On the throw path there is no data to prefer, so the same reasoning points the other way.

Nothing caught either, because nothing executed the query

About a dozen tests assert things about this query's text and every one passed throughout. A substring assertion cannot resolve a name — only a server can. So the deliverable is not the qualifier:

DarlingPgReadSqlParsesLiveTests PREPAREs every shipped PostgreSQL read against a real PostgreSQL. PREPARE runs the whole front end — name resolution, ambiguity detection, type inference — and stops before execution, so it needs no fixture and no seeded rows, which is exactly why it catches a defect that zero rows was never a defence against. The reads are discovered by reflection, not listed, so a new one is covered the day it lands; and the discovered count is asserted, so a filter that quietly stopped matching cannot turn the guard into a test that passes by finding no work to do. All 12 shipped read constants across the 9 readers now parse.

DarlingPgTopQueriesLiveTests executes the tool end to end. The awkward part is guarding defect 2 without being vacuous: once defect 1 is fixed the read no longer throws, so an assertion on the ordinary empty path would pass with or without the gate fix. The throw is therefore induced — delta_calls at bigint extremes makes the read's CAST(SUM(...) AS bigint) overflow at runtime, deterministic and needing no DDL against the shared store — and the discriminating half is the Aurora case: the same fault on an engine that can collect must still read as an error, or the "fix" would just be an exception swallower.

Sibling sweep (issue's ask #2)

• Ambiguity shape: 1 of 12 shipped reads failed; the other 11 parse clean. Now 0 of 12.
• Gate-after-the-query shape: repo-wide — all nine PostgreSQL tools have exactly one catch block, and only this one consults the gate there. But with defect 1 fixed no other read can currently throw, so the other eight are recorded here rather than changed.

---

#2548 — queryid was a JSON number, and JSON numbers round

PostgreSQL's queryid is a signed int8 derived from a hash, so its values are spread over the whole 64-bit range and most sit past 2^53 — where every parser that decodes JSON numbers as IEEE-754 doubles rounds. Reproduced against real JSON.parse in node, before any fix:

wire=-4185925123159566327   JSON.parse -> -4185925123159566300    LOST
wire=8993582471124583231    JSON.parse -> 8993582471124583000     LOST
wire=4185925                JSON.parse -> 4185925                 exact   <- control

The issue's illustrative value was exactly right. The value was never wrong on the wire; it was unrecoverable after parsing, which for an identity is the same thing. queryid is the ONLY identity a PostgreSQL statement has, and every use of it is an equality join. A rounded metric is still approximately true; a rounded key matches nothing. Option 1 from the issue, taken deliberately — and it matches how SQL Server's query_hash already reaches this surface, as 0x… text.

root_backend_id is the same defect in a worse form, and is fixed here

The issue flagged it as "worth a look, though neither is used as a join key the way queryid is". That aside is wrong, in the dangerous direction. PgBlockingCollector builds the id by CONCATENATING the backend's start epoch with its zero-padded pid, giving a 17-digit integer around 1.79e16 — about 2x past 2^53, where adjacent doubles are 2 apart. Half of all backend ids are odd and have no representation at all, and an unrepresentable one rounds onto its even neighbour, which is a different backend:

COLLIDE: 17870000000001000 and 17870000000001001 both parse to 17870000000001000
of 200 distinct backend_ids: 100 lost precision, 99 collided into an id belonging to another backend

queryid loses the join; root_backend_id makes the wrong one — in the field the tool's own description tells a reader to prefer over root_pid for exactly that cross-capture comparison.

What was deliberately LEFT

Swept every long/long? reaching JSON across both MCP surfaces. Nothing else has a defect to fix: database_id / user_id are PostgreSQL oids (unsigned 32-bit, structurally unable to reach the range); root_pid and the pid arrays are int; Query Store query_id / plan_id (both apps, plus structured_remediation.force_plan_targets[]) are sequential bigint IDENTITY columns nowhere near 2^53, so changing them is a breaking change across Lite + Darling + web + the remediation contract with no defect to point at; query_hash / plan_hash / sql_handle / plan_handle already reach this surface as 0x… text; AG LSNs are already strings; PVS transaction ids, health-parser broker/node ids and view_id are small.

Guard

PgInt64IdentityWireShapeTests pins both fields as strings and database_id / root_pid as numbers (so "stringify everything" cannot pass), asserts its own fixture is genuinely out of double range, and asserts four distinct backends stay four distinct ids after a double-decoding parse.

Writing that guard caught two of my own false claims, both of which would have made it look rigorous while proving nothing: long.MinValue is exactly -2^63, a power of two, so a double holds it exactly and it belongs in the control set; and "every backend_id is corrupted" is false — only the odd ones are, which is why the test states it as Assert.Equal(id % 2 == 0, SurvivesDouble(id)).

---

Verification, and its limits

Every fix was proven red first, by reverting only that fix, then green:

revert	harness result
the two .ToString(CultureInfo.InvariantCulture) calls	15 checks fail, incl. chains[1] decoding onto chains[0]'s id
the SQL qualification only	the read EXECUTES instead of throwing 42702 FAILS — 42702: column reference "queryid" is ambiguous
the catch-path gate only	on stock PostgreSQL the same throw answers not_collected FAILS — got error: 22003 bigint out of range

Run on macOS against a real migrated PostgreSQL store in Docker, driving the shipped GetPgTopQueries through a real NpgsqlDataSource, plus real JSON.parse in node for the wire shape. Harnesses are gitignored under Darling/tools/pg-harnesses/.

What I could not verify: Darling.Tests targets net10.0-windows — it compiles on macOS but cannot run there (No frameworks were found / Microsoft.WindowsDesktop.App). The two new live test classes are mirrored assertion-for-assertion by harnesses I did run against real PostgreSQL, but CI is the arbiter for the xUnit run itself. Not run against a live Aurora target; the Aurora-specific columns are exercised only as stored data.

Web

No change needed, and that is checked rather than assumed. The Query ID column is { key: "queryid", label: "Query ID", mono: true } with no format, so panels.js renders it via String(raw) and simply starts showing the true digits. inferFormat already returns "text" for any string, so custom-view auto-detect picks the right formatter. root_backend_id is on no grid.

[claude]

Review

Went through the full diff (CHANGELOG, DarlingPgStatementReader.cs, DarlingMcpPgStatementTools.cs, DarlingMcpPgBlockingTools.cs, and the four test files).

Correctness

• The ambiguity fix (differenced.queryid AS queryid in the select list and GROUP BY differenced.queryid, database_id) is complete — both places PostgreSQL would resolve the bare queryid against are qualified, and the t alias in the LEFT JOIN no longer collides. Verified there's no other unqualified reference to a column that exists in both differenced and t.
• queryid/root_backend_id → string via .ToString(CultureInfo.InvariantCulture) is the right fix for the 2^53 double-rounding defect, and database_id/root_pid are correctly left as numbers (PostgreSQL oid is unsigned 32-bit, root_pid is int32 — neither can reach the rounding range).
• The catch-block capability gate in GetPgTopQueries calls DarlingEngineCapability.NotCollectedStatusAsync, which internally catches its own exceptions and returns null on failure (DarlingEngineCapability.cs:71-78) — so a second DB call from inside a catch block can't itself go unhandled. No regression there.
• BuildCycleEntries returning List<object> still serializes each element by its runtime type under System.Text.Json's default (non-source-generated) reflection serializer, which is what CycleEntriesKeepTheirMembersThroughTheObjectBoxing pins — confirmed this is correct default behavior, not an assumption that could silently break.

Web parity claim (checked, not just taken on faith)

• wwwroot/js/pages/server-tabs.js:2608 — the queryid column has no format, so panels.js:139 renders it via plain String(raw). Confirmed.
• derive.js:55 — inferFormat returns "text" for any string value before it ever reaches the numeric/_id$ heuristics, so custom-view auto-detect on an upgraded store still gets "text". Confirmed.
• root_backend_id does not appear anywhere in server-tabs.js. Confirmed not on any grid.

Lite/Darling parity

• No get_pg_* tools, queryid, or root_backend_id exist anywhere under Lite/ — PostgreSQL support is Darling-only, so there's no counterpart to drift out of sync here.

Security

• Both queries remain fully parameterized ($1/$2/$3), no string concatenation introduced.

Nothing to flag — the fix is scoped correctly, the qualification fix is measured rather than assumed (matches the PR's own note that the GROUP BY-only fix doesn't work), and the wire-shape guard tests exercise the shipped serializers rather than a re-implementation.