Add a watermarked query-text fetch seam, default off (#2150) - #2291
Conversation
The query_store payload selects query_sql_text (nvarchar(max)) inside a TOP ... WITH TIES ... ORDER BY last_execution_time. A Top-N Sort carries every output column through the sort and reads all of its input before emitting row one, so choosing the rows to ship materialized the text for the entire qualifying set. Measured with #2210's plan XML already gone and that one column as the only difference, on a purpose-built Azure SQL DB store: 1,505 rows / 12.8 MB text: ttfr 4.67s -> 0.45s drain 8.06s -> 0.50s 4,037 rows / 34 MB text: ttfr 5.02s -> 0.57s drain 16.95s -> 1.45s So #2210 did not finish this. It removed the larger column (195 KB average plan against 8.5 KB of text on that store) and left the one that still dominates. Neither knob bounds it, both measured: TOP (500) cost the same as TOP (50000), and wall time was flat from a 4 MB to a 256 MB client budget because the server finishes before the client sees a byte. This commit adds only the seam and changes no behaviour. The new FetchQueryTextSeparately flag defaults false and no host sets it yet, so the emitted SQL is byte-identical to before -- verified against the SQL captured from dev prior to the change, and pinned by a test that normalizes the one column out of both forms and requires the remainder to match exactly. It is a flag rather than a deletion because Lite stores that text inline in DuckDB and its grid reads it from there, so nulling the column unconditionally would blind Lite. Gated, the placeholder keeps the column's ORDINAL, which matters because the readers index the row by number. The fetch is watermarked rather than deduped per pass, a decision this collector already made once: #1556 shipped each plan once per PASS via ROW_NUMBER and #2164 replaced it because that form re-ships every pass forever, and drain is 94-97% of a pass. query_id is an identity, monotonic within a database, so a statement's text is fetched once ever -- and keying on query_id rather than query_text_id needs no new fact-table column and no migration, because query_id is already a stored payload column. Deliberately simpler than the plan fetch: no candidate-window estimator, because SUM(DATALENGTH(query_plan)) forces decompression of every plan in the window while query_sql_text has no such cost; and no content hash, because plan XML can be rewritten in place whereas a statement's text is fixed for the life of its id. The refresh horizon is kept for the hazard that does apply -- query_id is monotonic in first-seen order, not in "we have stored it", so a Query Store reset renumbers from the start. Verified by executing the new watermark facts in a plain net10.0 harness (Darling.Tests is net10.0-windows): 10 facts / 17 cases, plus direct assertions on the emitted SQL for flag-off byte-identity, ordinal stability, the fetch's shape, and all four stall-guard inputs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (!long.TryParse(raw.AsSpan(0, split), NumberStyles.Integer, CultureInfo.InvariantCulture, out textId) | ||
| || textId < 0) |
There was a problem hiding this comment.
Nit: this class's own doc comment claims it has "same encoding, same conservative-zero rules, same never-backward advance" as QueryStorePlanXmlState, but the validity boundary here differs from the sibling it's modeled on. Here a stored textId == 0 parses as valid (only < 0 is rejected):
if (!long.TryParse(raw.AsSpan(0, split), NumberStyles.Integer, CultureInfo.InvariantCulture, out textId)
|| textId < 0)QueryStorePlanXmlState.TryParse rejects planId <= 0 instead, treating a stored 0 as malformed (PerformanceMonitor.Collectors/QueryStorePlanXmlState.cs:348).
Resolve() happens to return 0 either way today, so there's no observable behavior difference yet — but ResolveStamp() diverges: on the text side a stored "0:<stamp>" carries its stamp forward, while the plan side would discard it and have the caller "stamp now" instead. Since AdvanceWatermark(0, []) legitimately returns Watermark = 0, a future write-back for a database with zero query-store rows would persist exactly this "0:<stamp>" shape, so the two "sibling" watermarks would start behaving differently the moment the follow-up PR wires the write-back path. Worth either matching the plan side's <= 0 check or documenting why text intentionally treats 0 as a legitimate watermark.
|
Reviewed. This is a tightly scoped, flag-gated seam PR — no behavior change with Went through it for correctness, Lite/Darling parity, security, and performance:
Left one inline nit on |
CI caught this, and the guard that caught it is the point: adding a per-database state key prefix without classifying it is a new orphan class, and nothing else about adding one would fail. textwm: is keyed prefix + databaseName exactly like planwm:, so it goes in QueryStorePerDatabaseState.PrunableKeys and both hosts drop it when the database is dropped. Paired with its OWN collector name rather than the plan fetch's -- the two watermarks are stored separately on purpose, and a prefix pruned under the wrong owner silently deletes nothing, which looks exactly like having nothing to prune. Also updated the literal count pin in AzureForeignStatePruneTests from 3 to 4 and added the matching Contains, and named QueryStoreTextState in the drift guard's discovered-class assertions. The reflection already found it without being told; naming it means a rename that drops it out of the pattern fails instead of silently shrinking the set under test. Verified by replicating the guard's reflection locally against the built assembly: three state classes discovered, four prefixes declared (done:, hole:, planwm:, textwm:), every one with a verdict, count 4, NotKeyedByDatabase still empty, and KeyFor confirmed to be exactly prefix + databaseName -- which is what justifies PrunableKeys over the other list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| } | ||
|
|
||
| if (!long.TryParse(raw.AsSpan(0, split), NumberStyles.Integer, CultureInfo.InvariantCulture, out textId) | ||
| || textId < 0) |
There was a problem hiding this comment.
Minor inconsistency with the sibling this class claims to mirror: QueryStorePlanXmlState.TryParse rejects planId <= 0, but this rejects only textId < 0 — so a stored watermark of "0:<timestamp>" parses successfully here (Resolve still returns 0 either way, but ResolveStamp now returns the real stamp instead of null, which changes how a caller doing "carry the stamp forward" would behave on a first pass that lands zero rows).
The class doc says "same encoding, same conservative-zero rules" as the plan-side state, so this divergence is either an intentional improvement (plan-side arguably has the same latent gap for an empty-catalog first pass) worth calling out explicitly, or an unintended drift worth matching to <= 0. Either way it's untested — none of the AMalformedWatermarkRefetchesEverything cases cover "0:<ts>", so this behavior isn't pinned.
Not a live bug today since nothing calls Format(0, ...) yet (the fetch isn't wired into any runner in this PR), but worth deciding before the follow-up wires it up.
|
Reviewed. This is a clean, well-scoped seam PR — Correctness
Lite/Darling parity
Security
Performance
Solid work overall — the test coverage (byte-identity pin, ordinal-stability pin, all four watermark-stall inputs) is thorough for a flag that changes nothing yet. |
#2292) The storage half of the seam merged in #2291. collect.query_store_text is keyed (server_id, database_name, query_id), and query_id was chosen because it is ALREADY a stored fact column -- so this rung adds a table and alters nothing, readers get the join key for free, and no migration touches query_store_stats. Text is stored inline rather than as a digest into a content-addressed dimension. QueryStorePlanMap earns that machinery because plan XML is enormous and duplicated; Query Store has already de-duplicated text one row per statement per database, so there is nothing to squeeze -- and inline removes the dimension GC liveness interlock whose failure mode is silently missing text. The upsert overwrites the TEXT, not just the stamp. query_id is unique within a database only until Query Store is RESET, which renumbers from the start, so id 5 afterwards is a different statement than id 5 before. The refresh horizon brings us back to re-read it and this is where the corrected text lands; touching only last_seen would leave the old statement's text on the new id forever, which reads as a plausible wrong answer rather than as missing data. Pruned on last_seen rather than by drop_chunks (a keyed store, not a time series), bounded to one chunk-width of the oldest rows per call, with the retention margin ADDED to the fact horizon so text outlives the rows that reference it. SHIPPED INERT. FetchQueryTextSeparately is still false, because flipping it nulls the payload column while six reader surfaces still project query_text straight off query_store_stats -- the flip and the reader conversion have to land together or those surfaces silently lose text for new rows. They get their own reviewable change. What ships live: the fetch pass, its per-database watermark under its OWN state owner, and the Viewer probe. The state owner matters -- the load merges both owners, so writing the text watermark under the plan fetch's owner would read back fine and then never be pruned, because the shared prune set pairs textwm: with query_store_text and a prefix pruned under the wrong owner deletes nothing. The Viewer probe is the three-place edit: a probe column, a reader argument, and a map parameter. Verified in lockstep at 50/50/50 with contiguous ordinals 0..49 -- note the probe's raw "EXISTS (" count is 51 because one column is a compound EXISTS(...) OR EXISTS(...), so counting occurrences is a false failure. A probe that cannot SEE the newest object maps every fully-migrated store below the required version and the Viewer refuses to open. Darling.Tests BUILDS on macOS (it cannot run -- needs the Windows desktop runtime), so the whole suite is compile-verified here; the new facts run in CI. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
First of two for #2150. This one changes no behaviour — it adds the seam with the flag off. The follow-up wires Darling's storage and turns it on.
Why, with the number
The query_store payload selects
query_sql_text(nvarchar(max)) inside aTOP ... WITH TIES ... ORDER BY last_execution_time. A Top-N Sort carries every output column through the sort and reads all of its input before emitting row one, so choosing the rows to ship materialized the text for the entire qualifying set.Measured with #2210's plan XML already gone and that one column as the only difference, on a purpose-built Azure SQL DB store (S2,
INTERVAL_LENGTH_MINUTES = 1,QUERY_CAPTURE_MODE = ALL), medians of 3:So #2210 did not finish this. It removed the larger column — 195 KB average plan against 8.5 KB of text on that store — and left the one that still dominates. That was a reasonable read at the time: round 3 measured "the two
nvarchar(max)columns" together, and it was fair to assume the 96 KB one was nearly all of it.Neither knob that looks like it should bound this can, both measured:
TOP (500)cost the same asTOP (50000)because the sort consumes its input either way, and wall time was flat across a 4 / 8 / 16 / 32 / 64 / 256 MB client budget sweep because the server finishes before the client sees a byte.Shipped off
FetchQueryTextSeparatelydefaults false and no host sets it, so the emitted SQL is byte-identical to before. I verified that against the SQL captured fromdevprior to the change (7044 == 7044 chars), and pinned it with a test that normalizes the one column out of both forms and requires the remainder to match exactly — which covers "nothing else moved" for every column at once rather than for the handful someone thought to list.It is a flag rather than a deletion because Lite stores that text inline in DuckDB and its grid reads it from there, so nulling the column unconditionally would blind Lite. Gated, the placeholder keeps the column's ordinal, which is load-bearing: the readers index this row by number, so a column that moved would silently shift every later field onto the wrong value.
The
query_store_query_textjoin deliberately stays when the column is nulled. It is one row per key, and the measurement above was taken with it in place — removing it would be an unmeasured change riding along on a measured one.Watermarked, not deduped — a decision this collector already made once
#1556 shipped each plan once per pass via
ROW_NUMBER; #2164 replaced it because that form "ships each plan once per pass but re-ships it every pass forever, and since drain is 94-97% of a pass and is per-row LOB cost, NOT fetching is worth far more than fetching less." Then #2210 took the column out entirely.query_idis an identity, monotonic within a database, so a statement's text is fetched once, ever. And keying onquery_idrather thanquery_text_idmeans no new fact-table column and no migration —query_idis already a stored payload column, so it is already a join key. The cost is storing a duplicate for the rarequery_ids that share one text (aquery_idis per text plus context settings, so they are close to 1:1), which is the cheaper side of that trade.Deliberately simpler than the plan fetch
SUM(DATALENGTH(query_plan)) OVER (ORDER BY plan_id)forces the server to decompress every plan in the window —sys.query_store_plan.query_planis decompressed by the view on access.query_sql_textis not, so a flat coarse bound plus the exact running-byte total is enough.query_idis monotonic in FIRST-SEEN order, not in "we have stored it", so a Query Store reset renumbers from the start and without a bounded horizon would suppress every text forever.Verification
Darling.Testsisnet10.0-windows, so I Compile-Include'd the new watermark tests into a plainnet10.0harness and executed them: 10 facts / 17 cases. Separately asserted directly against the emitted SQL: flag-off byte-identity, ordinal stability, the fetch's resume predicate / candidate bound / exact budget /ROWS UNBOUNDED PRECEDINGframe / ship order, and all four inputs that would make the fetch ship nothing and silently stall the watermark (inline-text host, zero budget, zero candidates, null context) — a stalled watermark looks exactly like a quiet database, which is why those are exceptions rather than no-ops.What the follow-up does
Adds
collect.query_store_textkeyed on(server_id, database_name, query_id)with a migration rung, a writer, the runner's fetch pass, readersCOALESCEing the new store against the existing inline column so already-collected rows keep their text, and sets the flag. Splitting it this way means text is never at risk from a bug in the storage path: until the flag flips, the inline column is still shipping.Not claimed here: that this accounts for the reporter's 37–99.8 minutes. Largest clean number on my rig is 16.95s of drain on 34 MB of text.
🤖 Generated with Claude Code