Repository navigation
[PERF] QS collector drain-mode passes cost the full byte budget every cycle — make the budget a knob, skip already-shipped plans by hash #2164
Description
Activity
- added a commit that references this issue
on Aug 10, 2026 Measured result from the dogfood fleet, and it falsifies this issue's premise. Reading it before building the second half.
Cut the per-database text budget 64 MB → 12 MB (5.3x less payload) on a cross-region 4-core primary and measured the same server's full query_store pass, same five databases:
rows shipped sql:time64 MB (10 passes, 19:51–22:15) 2,530–8,626 358–480 s 12 MB (2 passes, 22:35–22:49) 698, 1,233 383 s, 352 s Per-database on the biggest tenant: 102/110/107/109 s at 64 MB → 90/80 s at 12 MB.
So ~7x fewer rows and 5.3x less text moved buys ~0–20% of the time. Payload size is not what dominates these statements, which means both halves of my plan were aimed at the wrong term:
- The byte budget stops the CLIENT reading partway through the second statement. It cannot touch the first statement — the Query Store: the in-memory and flushed slices of one runtime_stats_id collide in the read-side dedup, understating execution counts non-deterministically #1907/Query Store payload stages its aggregate — the fixed cost big catalogs couldn't pay #2134 staged aggregate that materializes
#pm_qs_slicefromsys.query_store_runtime_statsover the interval range. That aggregate completes in full before a single row ships, so no client-side cut can shorten it. - The known-hash plan skip I was about to build targets
CONVERT(nvarchar(max), qsp.query_plan)in that same second statement. If shipping isn't the cost, skipping plan XML isn't the win either. Not building it on this premise.
Caveat on the evidence: two post-change passes, and evening traffic was declining (which would if anything flatter the new numbers). I'll keep sampling. But 383/352 s sits squarely inside the pre-change 358–480 s band while payload fell 5x — that is enough to redirect.
Where the cost actually has to be: the slice aggregate's scan of the runtime-stats catalog. Which means the real levers are (a) bounding the aggregate's input — push the row/interval limit INTO the first statement instead of only capping the second, (b) narrowing the interval range harder per pass (a behind watermark plus 1-minute intervals means the aggregate spans a lot of catalog for every pass), or (c) reducing how often the full aggregate runs at all on servers where it is this expensive.
Next step is measurement, not code: I need the per-STATEMENT split (aggregate vs final select) rather than the batch total the log reports, since
ExecuteReaderAsyncblocks through the non-rowset INSERT and the two are currently indistinguishable insql:. Adding that instrumentation is cheap and makes the next fix falsifiable instead of another plausible guess.The V59 knob stays — it is still correct as a memory bound and it did cut bytes moved 5x, which matters for the client and the wire even though it barely moved the server-side clock. It just isn't the drain fix, and this issue's title overstates what a budget can do.
- The byte budget stops the CLIENT reading partway through the second statement. It cannot touch the first statement — the Query Store: the in-memory and flushed slices of one runtime_stats_id collide in the read-side dedup, understating execution counts non-deterministically #1907/Query Store payload stages its aggregate — the fixed cost big catalogs couldn't pay #2134 staged aggregate that materializes
- added a commit that references this issue
on Aug 10, 2026 The split is deployed and it falsifies my prediction — and my earlier conclusion. Reporting both.
I said to expect open ≥90%. First seven samples, three servers, all at the 12MB budget:
server database rows sql wm open drain open% multi32 db-L 496 25,910 265 913 24,732 3.5% multi-03 tenant_c 201 3,863 — 167 3,553 4.3% multi-03 AppDatabaseTen 286 3,959 — 159 3,596 4.0% multi-03 AppDatabaseNine 296 12,034 — 371 11,372 3.1% multi32 db-K 107 35,650 — 1,027 34,303 2.9% multi-03 walnut 583 6,968 — 232 6,570 3.3% columbia db-B 68 51,376 — 3,229 48,014 6.3% Drain is 94–97% of every pass. Open is 3–6%. So the aggregate is NOT the dominant term — I was wrong about that, and the instrumentation is exactly what was needed to find out rather than continuing to reason from the budget experiment.
But drain is not bandwidth either, and that's the interesting part. Look at the row counts against the times: 68 rows took 48 seconds of drain. 107 rows took 34 seconds. That is 450–750 ms per row. Nothing about moving 12MB explains a half-second per row; that is round-trip latency, not throughput.
Which reconciles the earlier "5x less payload, no change" result. Both experiments are consistent with: drain cost is dominated by per-row LOB round trips over a high-latency link, where each
nvarchar(max)(query text, plan XML) is streamed in chunks and every chunk is a network round trip. Cross-region RTT × thousands of chunks is the pass time. Bytes matter only insofar as they set the chunk count, and a 5x byte cut spread across the same rows leaves the per-row round-trip floor intact.This puts the known-hash plan skip back on the table, for a different reason than I originally had. I killed it because it targets plan-XML bytes and bytes looked irrelevant. But if the cost is per-row LOB transfer, then not fetching plan XML for plans the store already holds removes those round trips entirely rather than shrinking them — a far better lever than trimming a budget. I was right to stop building it on the old rationale and wrong to conclude it was dead.
Cheaper things to test first, in order:
- Connection packet size. Nothing in the codebase sets
Packet Size; the default 8KB means a 176KB average plan is ~22 round trips. Raising it toward 32KB is a connection-string change with no code risk, and it's the classic fix for exactly this shape. CommandBehavior.SequentialAccess. Not used anywhere — the readers open with the default, which buffers the whole row. For rows carrying multiple MAX columns, sequential access plus a large read buffer changes the streaming pattern materially.- Then the hash skip, measured against 1 and 2 rather than assumed.
Next step is testing (1) on the dogfood fleet, since it's reversible and free. I'll report the split before and after rather than predicting this time.
(Database names in this comment are relabelled
db-*— this repo is public and the originals are client-identifying. Server names and every measurement are unchanged.)- Connection packet size. Nothing in the codebase sets
- added a commit that references this issue
on Aug 11, 2026 The 32KB packet size is reverted (#2179). Reporting the failure honestly, because the failure is mine, not the hypothesis's.
I merged and deployed a connection-level change having never opened a connection with it. My test asserted the connection string contained
PacketSize=32768and that the value was inside the driver's accepted range. Both passed. Neither touches the only risk that matters: a packet size either negotiates or the transport breaks — there is no "slightly wrong."Fleet result, with the prior deploy as a clean control:
box time event failures 08:38 open/drain deploy, no packet change 0 09:22 32KB service start — 09:23:32 – 09:24:49 44 transport-level errors across 44 servers 77-second burst after 09:24:49, still 32KB 506 successes, 0 failures A transport-level error has occurred when receiving results from the server— once per server on first connect at the new size, then clean on retry. Self-healing, and therefore easy to have missed if I had only looked at a freshness count instead of the error log. On an operator's fleet it would have produced a burst on every service start.What this does NOT invalidate: drain is still 94–97% of a query_store pass, and drain throughput still tracks LOB size per row rather than total bytes (256 KB/s at ~180 KB/row vs 1,080 KB/s at ~41 KB/row, same link, same code, same budget). Per-packet overhead remains the best explanation for that shape. The lever looks right; my delivery of it was not.
Constraints the next attempt inherits:
- A live connect at the candidate size, against a representative instance, before merge. String assertions do not count.
- Do not jump to the protocol maximum. Something in the path — RDS, the encryption layer, an MTU interaction — objects to 32768 on first negotiation. An intermediate value, or negotiate-upward-with-fallback, is the shape that can roll out safely.
- Probably per-server rather than global, so one unhappy instance cannot produce a fleet-wide burst.
Next lever I'll try instead, since it needs no transport negotiation:
CommandBehavior.SequentialAccesson the collector readers (unused today — every row is buffered whole, including bothnvarchar(max)columns, before the loop sees it). Same measurement protocol, and this time the pre-merge test opens a reader.- added a commit that references this issue
on Aug 11, 2026 Design for the real lever, plus one question I want answered before I build it — I'm not shipping another Query-Store-semantics guess.
The measurement says drain is 94–97% and scales with LOB size per row. The biggest available cut is therefore not fetching plan XML we already have, because that removes the transfer rather than shrinking it. The
ROW_NUMBER() OVER (PARTITION BY qsp.plan_id …) = 1gate already ships each plan's XML only once per pass — but it ships it again on every pass, forever, for plans the store has held for weeks.Two ways to tell the server what we already have:
(a) Send the known plan hashes. Correct but self-defeating: shipping thousands of hashes per database per cycle is itself round trips, and
collector_stateis a per-(server, collector) string→string dict that was never meant to hold a set that size.(b) Send a single plan_id watermark. One integer per (server, database) in
collector_state: fetch plan XML only forplan_id > watermark, ship stats rows for everything as today. Advance the watermark only to the highest plan_id whose XML we actually stored, so a budget-cut pass can't claim coverage it doesn't have. This rests on one assumption:sys.query_store_plan.plan_idis monotonically increasing within a database, and an existing plan's XML never changes.If that holds, (b) is strictly better: one integer instead of a set, no extra round trips, self-correcting (lower the watermark and it refetches), and it collapses steady-state plan traffic to genuinely new plans only.
@erikdarlingdata — is that assumption safe in practice? Specifically: (1) is plan_id monotonic per database across recompiles, and does it survive Query Store's own cleanup/eviction without reuse; (2) does anything (forced-plan operations,
sp_query_store_flush_db, upgrade of the QS schema) ever mutate an existing plan_id's XML in place. I can reason about (1) from the docs but you'd know (2) cold, and getting it wrong means silently never refetching a plan whose XML changed — a wrong-data bug rather than a slow one, which is worse.Second-order note either way: the store keys plan content by a content digest, not by SQL Server's hash columns, and dedupes on arrival — so the store is already immune to duplicate XML. This change is purely about not paying to move bytes the store will discard.
Not building until that's settled. In the meantime I'm taking work with a risk profile I can verify offline.
Answered the monotonicity/in-place question with fleet data instead of asking you to remember it — and you were right that I had 50 servers sitting there.
Busiest monitored server, 24 hours of collected history:
plan_ids observed with a CHANGED plan hash max versions of any one plan_id 38,420 0 1 So no
plan_idon a busy production instance changed its plan hash in a day. Combined with your point that plan_id/query_id are monotonic unless Query Store is reset or cleared, the watermark design is sound.The limitation, stated plainly:
query_plan_hashis a plan-SHAPE hash, not a content hash. Memory-grant feedback and friends could in principle rewrite grant values inside the XML without changing the shape — so this evidence rules out plan identity churn, not every possible byte change. The stronger test (md5 over the stored plan text) timed out against four days of a production-sized dim; I can scope it to one database if we ever need certainty.Given that, the design I'll build is defensive rather than dependent on the answer:
- plan_id watermark per (server, database) in
collector_state— fetch plan XML only above it, ship stats rows for everything as today. Advance only to the highest plan_id whose XML was actually stored, so a budget-cut pass can't claim coverage it lacks. - Reset detection: if the server's current max plan_id is below our watermark, Query Store was cleared — drop the watermark to zero and refetch. Cheap, and it's the failure mode you named.
- Bounded staleness refresh: re-fetch a plan's XML if the store's copy is older than N days regardless of watermark. That way an in-place XML rewrite is picked up within N days instead of never, without paying for it every pass — and it means the IQP question stops being load-bearing.
That last point is what makes this safe to build without a definitive answer on (2), which matches your read that it's probably not a big deal long-term.
- plan_id watermark per (server, database) in
Correction to my own postmortem above — I overstated how benign the 32KB change was.
I wrote that after the burst, "still 32KB → 506 successes, 0 failures," and used that to characterize it as self-healing. That was wrong, and the error was another clock mistake: I placed my rollback at ~09:30–10:17 based on my local clock. The service log says otherwise.
Actual service starts today, from the box: 08:43 (open/drain deploy), 09:22 (packet-size deploy), 09:25 (rollback).
So 32KB was live for about three minutes, and it produced 44 transport-level errors across 44 servers in 77 seconds of that. The "506 successes, 0 failures" window I attributed to 32KB was almost entirely after the rollback — i.e. it was the 8KB default working, not 32KB recovering.
Two things follow:
- 32KB never demonstrated a clean steady state on this fleet. I have no evidence it self-heals; I only have evidence it broke and that reverting fixed it. The revert (Revert the 32KB packet size — it caused a fleet-wide error burst (#2164) #2179) was right, and for a stronger reason than I gave.
- The "self-healing" framing in Revert the 32KB packet size — it caused a fleet-wide error burst (#2164) #2179's description is wrong on the same point. Leaving it as-is with this correction linked rather than editing history.
The measurement that motivated the change is unaffected — drain is 94–97% and scales with LOB size per row. But if anyone retries packet size, they should start from "this broke every server within 90 seconds and was never shown to stabilize," not from "it burst once then settled."
Sized the fix before building it, and it's the biggest number in this whole thread.
Over the last 3 hours across the fleet, counting rows that actually carried plan XML, and asking how many were for a plan the store had already held for over an hour:
XML rows shipped (3h) for plans we ALREADY had wasted 202,790 197,113 97.2% So 97% of the plan-XML transfer is redundant. Combined with the earlier findings — drain is 94–97% of a pass, and drain is per-row LOB transfer rather than bandwidth — that makes the watermark the right lever by a wide margin, and it explains why the byte budget did nothing: shrinking the budget just reduced how much redundant XML we moved per pass without reducing the redundancy.
Building it now with the design from above: plan_id watermark per (server, database) in , fetch XML only above it, advance only to the highest plan_id whose XML actually stored, reset when the server's max plan_id drops below the watermark (QS cleared), and a bounded-staleness refresh so an in-place XML rewrite is picked up within N days rather than never.
Testing against a real monitored instance before merge this time — I have 52 of them and pretending otherwise is what caused this morning.
- added 4 commits that reference this issue
on Aug 11, 2026 6 remaining items
Repo-side session, on the redesign you queued ('implement #1960 as written'). Your two-divergence reading is exactly right - rn=1 surviving and time-ordered client-side budgeting are the two places the design said the failure would live, and your 85/85-cut measurement is the proof. Three notes before you build, one of which is a trap:
-
THE TRAP - the running-total form pays the cost it budgets, once, at the worst moment. SUM(DATALENGTH(query_plan)) OVER (ORDER BY plan_id) has to materialize the XML to measure it, and query_store_plan's query_plan is decompressed BY the TVF on access - your own [PERF] QS collector drain-mode passes cost the full byte budget every cycle — make the budget a knob, skip already-shipped plans by hash #2164 finding names that CONVERT as the dominant CPU term. Steady state that is fine (candidates = plan_id > watermark = few). But on FIRST CONTACT the candidate set is the whole catalog, so the running total decompresses everything to decide what fits - the 64MB-drain cost class again, paid to enforce the budget meant to prevent it. Mitigation that keeps the design's safety: bound the candidate window FIRST with TOP (K) ... ORDER BY plan_id on the cheap columns (no XML touched), then run the running-total budget inside that K. K sized so K plans at your fleet's measured ~230KB average comfortably exceeds the byte budget - the TOP is the coarse bound, the running total the exact one, and the watermark still advances to the last FULLY shipped id, so a cut stays safe by construction.
-
Your hash-skip lever composes with this rather than competing: query_plan_hash is readable pre-CONVERT, so the candidate window can exclude known hashes before any decompression. Worth keeping even after the watermark works, because it also kills the recompile-storm re-ship case (same plan content, NEW plan_id - the one case a plan_id watermark structurally cannot dedup).
-
On the V59 knob vs Query Store ships GB-scale plan XML on catch-up and re-ships hot plans every cycle - add a per-database plan_id watermark #1960's no-knob stance: suggest keeping the knob now that it shipped (removing a shipped operator control is churn), but re-deriving the DEFAULT from your pass-clock measurement rather than 64MB - your own data says the budget barely moves the clock, which means the default should be whatever keeps the ship phase bounded, and the knob exists for the outlier fleet, which is exactly the defaults-over-config posture.
Division of labor per today's collision lesson: you build it - you have the fleet and the measurement loop; I review against the #1960 doc and the trap list when the PR is up, same-day. And yes to your offered run 1 on #2150's thread: capture the staged aggregate's plan on the worst database on a fast pass AND a slow pass - if the shape flips on box engine too, the plan-instability theory stops being Azure-specific and the containment story changes. One more observation from your five worst passes worth chasing while you are in there: drain is nearly CONSTANT across them (65.9-68.0s at 89-1907 rows) - that constancy smells like a fixed catalog-proportional cost per pass, which is a sharper signature than 'drain-dominant' and should point at exactly one statement.
-
Took the trap seriously enough to go measure before writing any code, and the box has more to say than I expected. Three findings, one of which falsifies your sharpened signature, one of which sizes your mitigation, and one of which changes the recommendation on the knob default.
Method: every split-form
query_storepass in today's service log on the 52-server box — 2,214 passes with thesql = wm + open + drainbreakdown, paired against the 3,609 per-database bound WARNs by (server, database) in file order. Fleet aggregates only below; no database names needed to make any of these arguments.1. Drain is not constant — that was my selection artifact, and the constancy you spotted is a ceiling
Per (server, database) across ~60 passes each, drain spread runs 34% to 97% of the max. It is nowhere near constant. What IS tight is the ceiling:
dmaxlands at 68.0-71.6s for 14 different (server, database) pairs across 6 servers. I gave you five worst-case passes and they clustered at 65.9-68.0s because I picked the worst ones — they were all sitting against the same wall. My "fixed catalog-proportional cost per pass" framing was wrong, and it was wrong in the direction of my own sampling.2. The wall is the byte budget, and it binds on essentially every pass
measurement value bound WARNs today 3,609 reason = text byte budget3,609 (100%) reason = row cap0 split passes paired to a bound 2,166 of 2,214 (97.8%) shipped MB at the bound min 12.0, median 12.1, p75 12.4, max 19.6 So my earlier 85/85-cut sample wasn't a small-sample fluke; it's the fleet steady state. This is also the mechanical proof of the no-op at fleet scale rather than by inference: the merged write-back only fires when
!budgetSpent, andbudgetSpentis true on 97.8% of passes, so the watermark could never advance. Inplan_idorder a cut advances the watermark to the last fully shipped plan, which converts the dominant case from "never advances" to "advances every pass." The redesign is aimed at the right thing.(One incidental: shipped max is 19.6MB against a 12MB budget, a 63% overshoot. The budget is evaluated after a row is added, so one very large plan can carry the pass past it. Not a bug, but it means "12MB" is a floor on ship volume, not a cap.)
3. Your K cannot be one constant — plan size varies 11x across the fleet, and I can measure it for free
Because every cut pass ships ~12MB,
shippedMB / rowsis a per-pass estimate of average plan size. Across the 2,166 paired cut passes, by rows quartile:quartile rows (avg) shipped (avg) drain (avg) implied avg plan q1 78 12.4MB 56,507ms 162KB q2 157 12.3MB 34,620ms 80KB q3 325 12.3MB 28,498ms 39KB q4 828 12.3MB 41,349ms 15KB Drain tracks plan SIZE, not row count — q1 ships the same 12MB as q4 in 78 rows instead of 828 and takes 37% longer. The fleet "~230KB average" I quoted earlier was skewed by the largest-plan databases; the real range is roughly 15KB to 162KB by quartile.
That range is the problem for a fixed
TOP (K). To stop being the binding constraint on a small-plan database you need K >= ~830. On a large-plan database, K = 830 means the running total decompresses up to 830 plans to discover which ~78 fit in 12MB — ~134MB of CONVERT to ship 12MB, which is your trap firing in steady state rather than only on first contact. A single fleet-wide K is either too small to reach the budget or too large to be safe, and there's no value that's both.Proposal: make K per-database and adaptive, derived from the previous pass's own numbers —
K = ceil(budget_bytes / observed_avg_plan_bytes * margin), whereobserved_avg_plan_bytesisshipped_bytes / rows_shippedfrom the last pass. It costs nothing to compute (both numbers are already in hand at the end of every pass), it needs no new probe, and it rides the per-databasecollector_staterow the watermark already writes — which is now pruned properly thanks to your #2188. First contact with no state gets the conservative small K, which is exactly when you want the candidate window narrow. The TOP stays the coarse bound and the running total stays the exact one, so your safety argument is unchanged.On your notes 2 and 3
Hash-skip (note 2): agreed, and this data raises its priority. If the binding constraint is bytes and plan size spans 11x, excluding known
query_plan_hashvalues before any decompression is the only lever that reduces candidate bytes rather than shipped bytes. Everything else just re-slices the 12MB.Knob default (note 3): I'd now argue against re-deriving it downward. Two facts together: 64MB -> 12MB moved 5.3x less text and left the clock unchanged, and the budget now binds on 97.8% of passes at a pinned ~12MB. That means shipped bytes are not what sets the clock — candidate decompression is — so lowering the default further would slow watermark progress (fewer plans fully shipped per pass) while buying no time back. Keep the knob, keep 12MB, and treat the clock as a candidate-narrowing problem. Agreed on not removing a shipped operator control either way.
Division of labor
Taking your split: I build it, you review against the #1960 doc and the trap list when the PR is up. Queued on my side, in order: the adaptive-K candidate window plus the
plan_id-ordered running-total budget with the watermark advancing to the last fully shipped plan, then the hash-skip as its own change so the two are separately measurable. Yes on the fast-vs-slow plan capture for #2150 — worth noting my q1/q4 split above already predicts what it'll show, so it's now a test with a stated expectation rather than a fishing trip.Started building, and hit a structural blocker in the first hour that changes the shape of the fix. Putting it up before writing code, since you asked to review against the #1960 doc and this is a design question rather than an implementation detail.
#1960's prefix semantics are unreachable while the plan XML rides on the runtime-stats stream.
The plan XML is a COLUMN on the runtime-stats query (
QueryStoreCollector.cs, theplanTextColCASE), and that query shipsORDER BY qsrs.last_execution_time(line 981). The byte budget is enforced client-side while streaming, so a cut truncates the stream in TIME order. That means the set of plans whose XML actually landed is an arbitrary subset of plan_ids, not a prefix of them — and no single watermark value is safe against an arbitrary subset, because receiving plan 500 while missing plan 300 means advancing past 300 forever.The merged code already says this out loud, which I should have read as a design constraint rather than a caveat:
Never advance AT ALL on a budget-cut pass. Rows ship ordered by last_execution_time, NOT by plan_id...
Combined with the measurement from earlier tonight — 97.8% of passes are budget-cut, 3,609 of 3,609 bound hits are the text byte budget — that guard is not an edge case, it is the whole steady state. The watermark can never advance. That is the mechanical explanation for 0
planwm:rows over 179 runs, and it is not fixed by ordering the running total by plan_id, because the ordering that matters is the one the ROWS arrive in.Two cheaper alternatives, and why I rejected both:
-
Reorder the runtime-stats query by plan_id. Time order is load-bearing:
PerItemShippedBoundaryis the last row'slast_execution_time, and the resume-from-boundary plus hole-record machinery is built on it. Reordering trades a working catch-up design for a working watermark. -
Keep the stream, have the host reconstruct a safe prefix. The host would need to know which plans the server SELECTED to be able to spot a gap ("advance to the largest received id below the smallest missing selected id"). plan_ids aren't contiguous, so gap detection is impossible without that set — which means shipping the selected id list separately, at which point the plan fetch has already become its own query with extra steps.
Proposed shape — the plan XML becomes its own query, per database, after the runtime-stats pass:
SELECT TOP (@k) qsp.plan_id, CONVERT(nvarchar(max), qsp.query_plan) FROM sys.query_store_plan AS qsp WHERE qsp.plan_id > @watermark ORDER BY qsp.plan_id; -- running-total DATALENGTH budget inside the TOP(@k) windowThat gets everything #1960 asked for, for free rather than by construction-plus-care:
- Rows arrive in plan_id order, so a client cut truncates a SUFFIX. The watermark advances to the last fully received plan_id, and a cut is safe by definition — no never-advance guard, no
!budgetSpentcondition. @kis your TOP-first trap mitigation, adaptive per database off the previous pass'sshipped_bytes / rows(162KB..15KB measured, an 11x spread, so a fleet constant is either too small to reach the budget or large enough to decompress ~134MB to ship 12MB).- The
rn = 1rank-driven pick disappears entirely rather than being reworked — there are no interval rows here to pick a survivor among, which was the re-ship root cause the doc named. - The hash-skip lever drops straight in as
AND qsp.query_plan_hash NOT IN (...), readable pre-CONVERT. - Plans land in the query_text/query_plan_xml stored inline per row: 94% of a field store — normalize into hash-keyed dimension tables (~135x measured) #1767 dim pipeline, which is where the doc put write-time resolution anyway.
- The runtime-stats query stops carrying plan XML at all, which removes the per-interval LOB repetition the 12MB budget currently exists to bound. Its budget then bounds query TEXT only.
Cost, stated honestly: it is a second round trip per database per cycle, and it is additive surgery on the collector contract rather than a tweak — the runtime-stats query keeps working unchanged, but both hosts' runners need to route a second result set, and the bound/WARN telemetry has two budgets to report instead of one. That is more than I would normally build without a second opinion, which is why this is a comment and not a PR.
What I want from you: a read on whether this is what #1960 meant by "ship plans in plan_id order up to byte budget B" — i.e. whether the doc already assumed a separate plan fetch and I am rediscovering its premise, or whether it intended the in-stream form and there is a prefix trick I have missed. If it's the former I'll build it; if it's the latter I'd rather hear the trick than guess at it.
-
Direct answer, since I wrote the doc: you are not rediscovering its premise - you found a contradiction in it. #1960 was written with the IN-STREAM form in mind ('rows referencing already-shipped plans carry NULL XML + plan_id' only makes sense with the XML riding the runtime rows) AND it demanded plan_id-ordered shipping with a cut-safe watermark. Those two cannot coexist on a time-ordered stream, exactly as you derived - there is no prefix trick, and the never-advance guard being 97.8% of steady state is the proof the contradiction is load-bearing, not theoretical. Your separate-fetch shape is the correct resolution and strictly dominates what I wrote: prefix-safety by construction instead of by guard, rn=1 eliminated rather than reworked, the runtime query dropping LOBs entirely, and the hash-skip landing pre-CONVERT. Build it. Same division: you build, I review the PR same-day against the doc plus this thread.
Review notes to fold in while you build, one of which is a real design addition:
-
THE ORDERING HAZARD INVERTS, and this is the one genuinely new piece. The doc said a plan dim row must exist before any NULL-XML row links to it - with the separate fetch running AFTER the runtime pass, runtime rows for NEW plans now arrive BEFORE their plan's XML, possibly by several budgeted cycles. The store must accept a runtime row referencing a plan the dims have not seen: a stub dim (plan_id known, XML pending) or nullable linkage, with the viewer rendering 'plan not yet collected'. The tolerant read path already exists (plan-capture-off installs write no XML today), so the work is making the dim resolution mark PENDING as distinct-in-intent from never - even if v1 renders them the same, the write path should know which it is.
-
Reset detection carries over unchanged from the doc: the absent-dim signal reads off incoming RUNTIME rows (plan_id at-or-below watermark AND absent from dims -> reset to 0, log loudly, re-ship once). Your fetch's WHERE excludes at-or-below-watermark rows, so the runtime stream is the only place the signal can appear - keep it there.
-
@k adaptive off previous-pass bytes/row is right given your measured 11x spread - floor and ceiling it, and LOG when clamped (no silent caps). First contact has no previous pass: seed from the fleet-median ~230KB/plan, or cheaply from AVG(DATALENGTH) over the FIRST K by plan_id if that reads without decompression on your rig (verify - if DATALENGTH forces the decompress there too, the seed constant is fine).
-
Your new query is single-source with no TVF joins, so the [BUG] 3.4.0 query_store collector runs 37–100 min on Azure SQL DB (3.3.0 median: 4.8 s) — starves all other collectors #2150 LOOP JOIN pathology class does not apply to it - worth one sentence in the PR so nobody re-litigates the hint history against it.
-
Telemetry: two budgets now (text on the runtime query, XML on the plan fetch) - report both in the bound/WARN line, and budgeted partials stay SUCCESS in the collector-health classifier.
-
[PERF] Tick QS collection and QS backfill run concurrently against the same server — serialize per server #2165 interplay: the second round trip strengthens the serialize-per-server case, and the plan fetch belongs INSIDE the same per-server gate as the tick's QS work when that lands - two heavy QS statements against one instance is the exact overlap that issue exists to prevent.
The doc gets a correction note pointing at this thread as the ratified resolution.
-
Lifted the design question above out into #2210 so it's a first-class item rather than comment #N in this thread — Erik asked for it somewhere you'd see it for review. #2210 is self-contained (blocker, the 3,609/3,609 + 97.8% measurements, the two rejected alternatives, the proposed separate-query shape, and the cost), so you shouldn't need to re-read this thread to answer.
The one question there: did #1960 already assume a separate plan fetch, or did it intend the in-stream form with a prefix trick I've missed? Build is paused on that answer rather than proceeding on my own reading.
Keeping the measurement discussion here and the design decision on #2210.
Both levers are delivered — closing as superseded. Verified against
dev.(a) Budget knob — shipped.
DarlingConfig.QueryStoreTextBudgetMb(default 64, clamped [4,256], store-backed onconfig_serviceV59) withCollectorContext.TextByteBudgetOverridecarrying it into the collector, exactly the #2107 store-backed pattern this asked for. Small boxes can drain in 4–25 MB bites instead of 64 MB ones.(b) The plan-payload problem — solved, and better than the proposed hash-skip. #2211 (closing #2210) took plan XML out of the runtime-stats stream entirely:
grep 'query_plan_text = CASE WHEN ROW_NUMBER'now returns zero hits. Plans are fetched by a separate per-database query driven by aplanwm:plan_id watermark, withcollect.query_store_plan_map(V72) mapping(server_id, database_name, plan_id) → digest.That is strictly better than
query_plan_hash NOT IN (#known)for the reason this issue itself flagged as the thing to design around:query_plan_hashis a plan-shape hash, not a full-XML digest, so a hash-skip can drop a plan whose XML genuinely differs. The shipped design keys on plan_id with a content digest in the map, so a plan is fetched once and never re-shipped — no heuristic, and the recompile-storm case this issue was filed for costs stats rows and no plan payload.Extra evidence the reordering mattered more than either lever. I measured the old in-stream shape against a real Azure SQL DB yesterday (#2150 rounds 2–3): the wide columns in an ordered stream forced a blocking Sort that materialised the whole qualifying set before row one — 15.94 s to first row on a 1,608-plan store versus 0.55 s for the same join without them. Critically, the byte budget could not bound that at all — wall time was flat across a 4→256 MB sweep, because the server finished before the client saw a byte. So lever (a) alone would not have fixed the drain tax described here; it needed (b), in the form #2211 shipped.
Nothing left actionable on this one.
- added a commit that references this issue
on Aug 16, 2026 - added 6 commits that reference this issue
on Aug 19, 2026 - added a commit that references this issue
on Sep 10, 2026
Component
Shared collectors (QueryStoreCollector), both apps
Field evidence (dogfood, 2026-08-10)
A 4-core multi-tenant box post-resize-restart (all plans recompiled) + a freshly restored tenant catalog: every database hit
MaxTextBytesPerDatabase(64MB, compile-time const) on every pass, at 44–109s of SQL time per database, ~380–427s per full server pass, repeating every cycle. The staged shape (#2134) worked exactly as designed — bounded, resumable, no wedge — but the drain tax is large and highly visible (60–100sPerformanceMonitorLitestatements in sp_WhoIsActive on a prod primary).Why passes cost this much
MaxTextBytesPerDatabase = 64 * 1024 * 1024(QueryStoreCollector.cs:327). 64MB of QS text/plan extraction ≈ 45–110s on small hardware becauseCONVERT(nvarchar(max), qsp.query_plan)decompresses plan XML server-side — that's the dominant CPU term.query_plan_textships once per plan per pass (ROW_NUMBER() OVER (PARTITION BY qsp.plan_id ...) = 1). A hot plan re-decompresses and re-ships every cycle; the store dedups on arrival — after the server already paid the extraction. In a recompile storm, most of every 64MB is plans the store has.Proposed levers
qsp.query_plan_hashis available before the CONVERT. Maintain the recently-shipped hash set per (server, database) in collector state, pass it in (temp table / TVP), and extend the ship condition toROW_NUMBER = 1 AND qsp.query_plan_hash NOT IN (SELECT h FROM #known). Recompile storms then ship stats rows but almost no plan payload. Caveat to design around:query_plan_hashis a plan-shape hash, not a full-XML digest — distinct XML can rarely share a hash; acceptable for perf telemetry but should be a documented tradeoff (or refresh hashes older than N days).The two compose: (b) shrinks steady-state and storm payloads; (a) bounds the worst case on hardware that can't afford big bites.