Stop re-shipping plan XML the store already holds (#2164) - #2183
Conversation
97% of the plan XML shipped in a three-hour fleet window (197,113 of 202,790 rows) was for plans the store had held for over an hour. The ROW_NUMBER gate from #1556 ships each plan once per PASS but re-ships it every pass forever, and since drain is 94-97% of a pass and costs per-row LOB bytes, not fetching is worth far more than fetching less. Lowering the byte budget 64MB to 12MB moved 5.3x less text and left the clock unchanged, which is what pointed here. A per-database watermark on plan_id — monotonic within a database — narrows the plan-text CASE to plans above the highest one whose XML actually stored. Absent, malformed or expired renders no predicate at all, so the conservative path is byte-identical to the pre-change query. Verified against four production servers and four real Query Store catalogs before merge, not just compiled. Rendered the actual SQL out of BuildPayloadBody and ran both shapes: row counts identical, XML rows down 884->509 / 213->136 / 474->318 / 244->174, elapsed down 28-51%, and zero rows at or below the watermark carried XML in any run. That last number is the correctness assertion, and it was measured at the MEDIAN plan_id; production watermarks sit near the max. Writing the tests and running them found three bugs in my own first cut: - The watermark leaked onto the BACKFILL path, which digs into intervals older than anything collected and so references plans numbered BELOW it. It would have suppressed essentially every plan the backfill exists to fetch, silently, since runtime stats still ship and a filled range would look complete. - The watermark advanced on budget-cut passes. Rows ship ordered by last_execution_time, not plan_id, so a cut drops an arbitrary set of ids off the tail — including ids below the highest one that stored. Advancing past them suppressed their XML on every later pass despite it never shipping once. - Reset detection was unsound in the common case. "Highest plan_id seen is below the standing watermark" is TRUE in any window where no new plan compiled, which on a steady workload is most windows, so it dropped the watermark constantly and defeated the whole optimization. Removed: exact detection needs the server's live MAX(plan_id), which this payload does not carry. Recovery from a reset is now the refresh horizon. That horizon is one day, not seven, and its stamp dates the last FULL fetch rather than the last advance — re-stamping on advance would push the horizon out forever on any database that keeps compiling plans, so it would never fire. One day removes ~99% of the per-pass redundancy (a 15-minute cadence re-ships a plan ~96 times daily); a week adds almost nothing and buys 7x the exposure on all three things the horizon guards: in-place XML rewrites, reset recovery, and the dormant-plan gap. Known gap, documented at the predicate: plan_id is monotonic in COMPILE order, which is not the same as "we have stored it", so a plan compiled before monitoring began and dormant through every collected window arrives below the watermark. Bounded by the horizon. The exact fix is a store-DERIVED watermark and needs host plumbing on both products. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /// <summary>Non-empty so the host loads and persists this collector's per-database watermarks; the real | ||
| /// keys are dynamic (one per database), and the host reads every row for this collector regardless.</summary> | ||
| public override IReadOnlyList<string> StateKeys { get; } = new[] { PlanWatermarkStateKeyPrefix }; |
There was a problem hiding this comment.
Minor parity nuance worth confirming is intentional: StateKeys is now unconditionally non-empty for this collector, regardless of CapturePlanXml. RemoteCollectorService.DefinitionRunner/DarlingCollectorRunner gate the collector_state SELECT on definition.StateKeys.Count == 0, so Lite (which never sets CapturePlanXml) will now issue one extra collector_state read per query_store collection cycle even though ResolvePlanWatermark's result is always discarded there (the CapturePlanXml ternary in BuildPayloadBody picks the plain nvarchar(1) placeholder, never the branch that uses watermarkPredicate). Impact is negligible (a local DuckDB point lookup), but it's a small behavior change for Lite that the PR description's "Lite ... is unaffected in behavior" doesn't quite cover — worth a one-line callout if intentional, or gating StateKeys/the ResolvePlanWatermark call on CapturePlanXml if not.
Review summaryWent through the watermark logic, the write-back bookkeeping, and the test coverage in
One minor nuance flagged inline: No missing-index or other DMV-folklore suggestions, per the review brief. Nothing else blocking. |
Two contract tests caught a real design mistake, not a test that needed updating. I had the query_store DEFINITION declare a StateKeys prefix, which broke CollectorStateContractTests (default_trace_events is pinned as the only state-declaring collector) and QueryStoreBackfillTests (which pins that the query_store definition declares none). Both pins exist for a reason I had missed, and the backfill had already solved this exact problem: collector_state rows for per-database Query Store work are owned by the HOST under its own name, not the definition's, because the keys are one per database and only known at runtime. QueryStoreBackfillState's own doc calls that 'the seam that lets both stay true'. Worse, my version was heading for the silent failure the contract test warns about. A declared prefix is not a real key: the host would have persisted the watermark under the DEFINITION's name and the load would still never have matched a dynamic per-database key, so the watermark would have resolved absent forever and collection would have quietly kept paying full price while every test passed. Declaring it would also have made this a two-host change rather than the local one it is. So the watermark now lives in QueryStorePlanXmlState, beside QueryStoreBackfillState and shared across SKUs for the same reason - a row written today must decode identically after an upgrade, and both hosts read the same definition. The definition declares nothing. The Darling runner loads the rows under query_store_plan_xml when plan capture is on, and routes query_store's pending state to that owner instead of the definition name. Lite needs no equivalent: it never captures plan XML, so nothing there reads or writes these. The rendered SQL is byte-identical to the previous commit - this changes only where the state lives. Re-verified: 25 unit tests pass, the predicate still renders inside the CASE and still never renders for backfill or with plan capture off, and Collectors, Darling.Service, Darling.Tests, Lite and Lite.Tests all build with zero errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch, and it's now moot in the best way — the redesign in dd3a100 removed You were looking at the same seam two other tests were, from the Lite side. Which also means my version was heading somewhere worse than one wasted DuckDB lookup. A declared prefix isn't a real key — the host would have persisted watermarks under the definition's name while the load still never matched a dynamic per-database key, so the watermark would have resolved absent forever and collection would have quietly kept paying full price with every test green. Your "the result is always discarded there" observation was one step from that: the value was going to be discarded on Darling too. So the watermark now lives in The rendered SQL is byte-identical across that commit — I re-verified the predicate still lands inside the CASE and still never renders for backfill or with plan capture off. The PR description's "Lite is unaffected in behavior" is now literally true rather than approximately true. |
| /// State key prefix; the remainder is the database name, because <c>plan_id</c> is only unique within one | ||
| /// database's Query Store and means nothing across databases. | ||
| /// </summary> | ||
| public const string WatermarkKeyPrefix = "planwm:"; |
There was a problem hiding this comment.
Minor/non-blocking: these planwm:{database} rows are never deleted, unlike QueryStoreBackfillState's hole keys, which get an explicit DeleteCollectorStateKeyAsync once resolved (see DarlingCollectorRunner.cs around the backfill worker). A dropped or renamed database leaves an orphaned row in collector_state forever — harmless individually, but on a server with churny database lifecycles (frequent create/drop, e.g. dev/test or multi-tenant provisioning) this can accumulate rows with no cleanup path. Not a correctness bug (an orphaned row is simply never read again), just a gap relative to the cleanup pattern this PR's sibling state (QueryStoreBackfillState) already established. Might be worth a follow-up that prunes planwm: keys for databases no longer in sys.databases during the eligibility/enumeration pass.
ReviewTraced the watermark logic end-to-end against both entry points (on-prem Correctness
Lite/Darling parity
Security
Minor — see inline comment on No missing-index DMV suggestions (none applicable here), no other correctness/perf/security issues found. |
|
Correct, and it's a fair catch against the sibling pattern — One thing I noted on the issue that makes the naive version of the fix worse than nothing, in case it saves someone time: the enumerated database list is not the same as "every database that exists." A database can be absent from a cycle's enumeration because it's offline, excluded by configuration, or failed its probe — none of which mean it was dropped. Pruning on a single absence would delete a live watermark and force a full plan-XML refetch for that database, which is the safe direction but self-defeating on precisely the servers that have excluded or intermittently-offline databases. So it wants either absence across consecutive enumerations or a check against Bounded in the meantime: one row per database name ever seen, ~100 bytes each, never read once orphaned. |
1 participant
What does this PR do?
Closes #2164.
Query Store collection was re-shipping execution-plan XML the store already had. In a three-hour fleet window it shipped 202,790 plan-XML rows, of which 197,113 (97.2%) were for plans the store had already held for over an hour.
The #1556 dedupe lands each plan once per pass — but it re-lands it on every pass, forever. Three measurements narrowed it to that:
Given (3), not fetching a plan is worth far more than fetching less of one — which is why a smaller budget bought nothing. So collection now carries a per-database watermark on
plan_id(monotonic within one database's Query Store, per Erik) and narrows the plan-text CASE to plans above the highest one whose XML actually stored.Absent, malformed or expired renders no predicate at all, so the conservative path is byte-identical to the pre-change query. That matters because absent is what a first run, a restarted host and a broken store all look like.
Which component(s) does this affect?
Lite shares the collector and is unaffected in behavior: it never sets
CapturePlanXml, so the predicate never renders and it writes no watermark. Both Lite projects build.How was this tested?
Against four real production Query Store catalogs, before merge. I rendered the actual SQL out of
BuildPayloadBodyby reflection — not a hand-written approximation — and ran both shapes with the watermark at each database's MEDIANplan_id, so both sides of the predicate were non-empty. 15-minute window, 45s command timeout, read-only apart from the payload's ownDROP TABLE IF EXISTS #pm_qs_slice.at_or_below_wm_with_xml= 0 in every watermarked run. The predicate suppresses exactly the plans at or below the watermark and nothing above it. That is the correctness assertion.XML megabytes fall less than XML row count (88.78 → 69.31 MB, 22%, versus 42% fewer rows) because the older plans below the median carry smaller XML than the newest ones — consistent with the per-row LOB finding.
SQL Server versions: the four instances span the production use2 fleet (2019 and 2022 box/RDS). Unit coverage is 25 tests in
QueryStorePlanWatermarkTests, all driven through the collector's public surface (BuildPerItemQuery,BuildBackfillPerItemQuery,ReadItemAsync) over a realDbDataReadergenerated from the collector's ownPayloadColumns— so no production visibility was widened for tests, and a column added to the collector cannot silently shift the ordinals the read loop depends on.Three bugs the testing found in my own first cut
Worth listing, because each was silent and two were shipped-code-shaped:
BuildBackfillPerItemQuerypasses no database name, so it fell through toCurrentDatabaseNameand applied. Backfill digs into intervals older than anything collected, whose rows reference plans compiled long ago and therefore numbered below the live watermark — so it would have suppressed essentially every plan the backfill exists to fetch. Silently: runtime stats still ship, so a filled range would look complete while carrying no plan XML at all.last_execution_time, notplan_id, so a cut drops an arbitrary set of plan_ids off the tail of the window — including ids below the highest one that stored. Advancing past them suppressed their XML on every later pass despite it never having shipped once. It now does not advance at all on a cut pass; the cut is already resumable on the time watermark, so that costs one repeated fetch and nothing else.MAX(plan_id), which this payload does not carry.The refresh horizon, and why it is one day
The watermark expires, and its stamp dates the last full fetch rather than the last advance. Re-stamping on advance would push the horizon out every time a new plan compiled — so on any database that keeps compiling, which is the busy ones where a stale plan matters most, the horizon would never fire and the watermark would effectively be permanent.
One day rather than seven, because the redundancy being removed is per-pass: a 15-minute cadence re-ships the same plan ~96 times a day, so a daily full fetch already eliminates ~99% of it and a weekly one adds almost nothing. What a longer horizon does buy is 7x the exposure on all three things the horizon guards:
Known gap, documented at the predicate
plan_idis monotonic in compile order, which is not the same as "we have stored it". A plan compiled before monitoring began, dormant through every collected window, then executed again, arrives below the watermark and has its XML suppressed until the horizon expires. Bounded to one day by the above.The exact fix is a store-derived watermark — the host asking its own plan dimension for the lowest plan_id missing XML, which cannot be wrong by construction — but that needs host plumbing on both products, so it is deliberately not in this PR.
Checklist
The watermark is inlined into the SQL as a parsed
long, never operator input, because the body nests insidesp_executesqlon three paths and threading another parameter through all of them buys nothing.(Database names are relabelled
db-*— this repo is public and the originals are client-identifying. Server names and every measurement are unchanged.)