Repository navigation
query_store plan/text fetch goes activity-driven: the store is the watermark (#2312) - #2333
Merged
Merged
Conversation
…termark The invariant 40-110s-per-run bill (#2312) had a named mechanism at last: the #2210 watermark walk's daily expiry was supposed to be replaced by a re-verify cursor that was built, tested, documented, and never wired (Finding 4) - so catalogs whose full walk needs more than a day expired MID-walk, restarted from plan_id 0, and looped the full catalog fetch forever. TouchSql, the liveness refresh the dimension GC depends on, had the same story: designed as 'the whole of the protection', zero callers (Finding 3), latent only because the perpetual walk's re-upserts were accidentally standing in for it. The reshape retires all of it in one shape change: the cycle's collected rows name their plans/texts; one touch-and-probe round trip per database refreshes map/dim liveness AND returns the missing set plus per-cycle in-place-rewrite / Query-Store-reset detection via the live hashes the payload already carries; the fetch selects exactly those ids under the same byte-budget arithmetic. A caught-up database issues NO target query. A reset recovers as the normal path. A dormant plan resuming execution is fetched the cycle it resumes. V77: query_store_plan_map.digest goes nullable (the NULL-XML content-less marker, so unpersistable plans read as known instead of riding every fetch list), query_store_text gains query_hash (the reset detector), and the orphaned planwm:/textwm: state rows are deleted wholesale. Budget- deferred ids carry over in memory so a plan referenced once cannot starve; target-side-gone ids drop only on a provably-uncut pass. Upserts COALESCE digest and hash toward knowledge, never absence. Retired: QueryStorePlanXmlState's watermark half (sizing stays - it caps server-side decompression), QueryStoreTextState wholesale, the cursor members, and the planwm:/textwm: entries in the shared prune set. Tests: QueryStorePlanWatermarkTests is reborn as QueryStorePlanFetchTests (new builder/probe/upsert shape pins + the surviving sizing tables, harness- executed against the built assembly), the V77 rung ceremony lands in ActivityDrivenPlanFetchStoreTests with the V74/V75/V76 files demoted, and QueryStoreFetchProbeLivePostgresTests round-trips the probe verdicts, liveness stamping, hash adoption, and the NULL-digest marker against a real store. Closes #2312 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…metic, and two source pins Both splice edits landed between a member's doc block and its body - the fetch method's doc stranded above the new extractor helpers, and V76's rung doc stranded above the V77 insertion. Third instance of this exact trap; both docs now sit with their members and a local scan mimicking the hygiene test confirms zero stacks. The seed candidate cap pin claimed the issue text's rounded ~118 where ceil(12MB/160KB*1.5) is 116. The text call-site literal QueryStoreTextStoreTests pins is restored to one line, and the runner's comments stop naming the retired prefixes literally so the retirement source pin can hold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 19, 2026
erikdarlingdata
added a commit
that referenced
this pull request
Aug 19, 2026
With #2333's catalog walk gone, the per-database split named the phase that does not subside after catch-up: wm, 1-3s per database per cycle. It is a read against OUR store, not the monitored server -- an unbounded MAX over a non-partitioning timestamp, so it touches every chunk that database has. Measured on the live use1 store (106 GB, 5 chunks), same server/database pair: unbounded, cold: 25,766 buffer reads + 195 written (temp spill) unbounded, warm: 228 ms bounded to 3h: 29 ms, 5 chunks excluded The unbounded cost is a function of store size and cache residency rather than of the monitored workload, so it degrades precisely where an operator is weakest: a long-lived store, a busier Query Store, slower disks. The bound changes no answer, and that is the whole justification: every consumer ends at max(stored, now - MaxCatchup), because ClampCatchup floors anything older and a null result falls back to query_store's 60-minute first-run window -- the same instant as the floor. So a row below the horizon cannot move the result whether it is found or not, and the unbounded MAX was paying to confirm a value the clamp would have produced anyway. The predicate goes on collection_time (the partitioning column) because a predicate on the watermark column alone prunes nothing, and a row's watermark can never exceed its own collection_time, so nothing qualifying hides behind it. Bounded for query_store ONLY, on both hosts, name-guarded like the other query_store-specific behavior in these runners: a ring-buffer collector whose legitimate catch-up spans days must keep reading its whole history, and the floor would silently truncate it. WatermarkPolicy.ReadFloor carries the reasoning and the measurements; its tests pin that the floor sits strictly older than the clamp horizon and that a buried watermark and a not-found one reach the same instant. Closes #2344 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Aug 19, 2026
With #2333's catalog walk gone, the per-database split named the phase that does not subside after catch-up: wm, 1-3s per database per cycle. It is a read against OUR store, not the monitored server -- an unbounded MAX over a non-partitioning timestamp, so it touches every chunk that database has. Measured on the live use1 store (106 GB, 5 chunks), same server/database pair: unbounded, cold: 25,766 buffer reads + 195 written (temp spill) unbounded, warm: 228 ms bounded to 3h: 29 ms, 5 chunks excluded The unbounded cost is a function of store size and cache residency rather than of the monitored workload, so it degrades precisely where an operator is weakest: a long-lived store, a busier Query Store, slower disks. The bound changes no answer, and that is the whole justification: every consumer ends at max(stored, now - MaxCatchup), because ClampCatchup floors anything older and a null result falls back to query_store's 60-minute first-run window -- the same instant as the floor. So a row below the horizon cannot move the result whether it is found or not, and the unbounded MAX was paying to confirm a value the clamp would have produced anyway. The predicate goes on collection_time (the partitioning column) because a predicate on the watermark column alone prunes nothing, and a row's watermark can never exceed its own collection_time, so nothing qualifying hides behind it. Bounded for query_store ONLY, on both hosts, name-guarded like the other query_store-specific behavior in these runners: a ring-buffer collector whose legitimate catch-up spans days must keep reading its whole history, and the floor would silently truncate it. WatermarkPolicy.ReadFloor carries the reasoning and the measurements; its tests pin that the floor sits strictly older than the clamp horizon and that a buried watermark and a not-found one reach the same instant. Closes #2344 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Aug 19, 2026
With #2333's catalog walk gone, the per-database split named the phase that does not subside after catch-up: wm, 1-3s per database per cycle. It is a read against OUR store, not the monitored server -- an unbounded MAX over a non-partitioning timestamp, so it touches every chunk that database has. Measured on the live use1 store (106 GB, 5 chunks), same server/database pair: unbounded, cold: 25,766 buffer reads + 195 written (temp spill) unbounded, warm: 228 ms bounded to 3h: 29 ms, 5 chunks excluded The unbounded cost is a function of store size and cache residency rather than of the monitored workload, so it degrades precisely where an operator is weakest: a long-lived store, a busier Query Store, slower disks. The bound changes no answer, and that is the whole justification: every consumer ends at max(stored, now - MaxCatchup), because ClampCatchup floors anything older and a null result falls back to query_store's 60-minute first-run window -- the same instant as the floor. So a row below the horizon cannot move the result whether it is found or not, and the unbounded MAX was paying to confirm a value the clamp would have produced anyway. The predicate goes on collection_time (the partitioning column) because a predicate on the watermark column alone prunes nothing, and a row's watermark can never exceed its own collection_time, so nothing qualifying hides behind it. Bounded for query_store ONLY, on both hosts, name-guarded like the other query_store-specific behavior in these runners: a ring-buffer collector whose legitimate catch-up spans days must keep reading its whole history, and the floor would silently truncate it. WatermarkPolicy.ReadFloor carries the reasoning and the measurements; its tests pin that the floor sits strictly older than the clamp horizon and that a buried watermark and a not-found one reach the same instant. Closes #2344 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Aug 19, 2026
…ally use (#2344) (#2346) With #2333's catalog walk gone, the per-database split named the phase that does not subside after catch-up: wm, 1-3s per database per cycle. It is a read against OUR store, not the monitored server -- an unbounded MAX over a non-partitioning timestamp, so it touches every chunk that database has. Measured on the live use1 store (106 GB, 5 chunks), same server/database pair: unbounded, cold: 25,766 buffer reads + 195 written (temp spill) unbounded, warm: 228 ms bounded to 3h: 29 ms, 5 chunks excluded The unbounded cost is a function of store size and cache residency rather than of the monitored workload, so it degrades precisely where an operator is weakest: a long-lived store, a busier Query Store, slower disks. The bound changes no answer, and that is the whole justification: every consumer ends at max(stored, now - MaxCatchup), because ClampCatchup floors anything older and a null result falls back to query_store's 60-minute first-run window -- the same instant as the floor. So a row below the horizon cannot move the result whether it is found or not, and the unbounded MAX was paying to confirm a value the clamp would have produced anyway. The predicate goes on collection_time (the partitioning column) because a predicate on the watermark column alone prunes nothing, and a row's watermark can never exceed its own collection_time, so nothing qualifying hides behind it. Bounded for query_store ONLY, on both hosts, name-guarded like the other query_store-specific behavior in these runners: a ring-buffer collector whose legitimate catch-up spans days must keep reading its whole history, and the floor would silently truncate it. WatermarkPolicy.ReadFloor carries the reasoning and the measurements; its tests pin that the floor sits strictly older than the clamp horizon and that a buried watermark and a not-found one reach the same instant. Closes #2344 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Sep 4, 2026
Merged
pull Bot
pushed a commit
to ehtick/PerformanceMonitor
that referenced
this pull request
Sep 10, 2026
erikdarlingdata#2902) The two carryover dictionaries erikdarlingdata#2312's activity-driven fetch defers budget-capped ids into were keyed by (ServerId, Database) with no collector in the key, while the fetch that spends them is gated on a SKU flag -- CapturePlanXml for plans, FetchQueryTextSeparately for text -- rather than on a collector identity. Five collectors take the enumerated dispatch path off Azure (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config), and only query_store can ever CREATE debt there, because ExtractPlanReferences and its text twin return empty for any other collector's batch. So the other four arrived having referenced nothing, found query_store's deferred ids under the key they happened to share, drained them, and had the fetch's wall clock recorded against their own collection_log row: ~346s of Query Store fetch time over 38h on one monitoring host, billed to plan_correction, query_store_health and index_object_stats. The draining was never intentional. FetchAndStorePlansAsync is documented as "the activity-driven plan-XML fetch for one database"; its call site says "this fetch is Query-Store-only"; and ExtractPlanReferences' own doc asserted that a non-query_store batch reaching it "never actually happens" because of the CapturePlanXml gate -- a belief that was false on every cycle, and that comment is corrected here. erikdarlingdata#2333's honesty notes name every deliberate trade the carryover makes (process-memory, restart forgets, the plan-dimension growth bend) and cross-collector draining is not among them. Both fields landed in one commit (8c79f0c) with the key shape copied from _consecutiveQueryStoreItemFailures, which is per-database for the same reason and is safe that way only because every one of its call sites sits behind an explicit definition.Name == query_store check. The shape was inherited; the guard was not. carryKey is one variable feeding four per-(server, database) fetch-state dictionaries, so all four widen together rather than two of them growing a near-identical sibling key: _observedPlanSize's learned plan size and backoff count and _textFetchFailures were also being reset by those foreign visits on their nothing-owed return path. Behaviour is unchanged for them beyond that, since after this only query_store reaches a fetch that reads or writes any of the four. FetchStateKey exists as a method rather than an inline literal so the pin drives the shipped construction -- an inline literal at each site is how the collector went missing. What this does not change: no work is saved and none is moved. The same ids are fetched and the same bytes ship; query_store's own measured cost rises by roughly what its siblings stop being charged. It does change when the debt is paid -- query_store's next visit rather than the first of five, at most one 5-minute cycle later, since query_store and plan_correction are both on the 5-minute tier while query_store_health is hourly, index_object_stats daily and database_scoped_config on-load. There is no cap and no eviction on the debt, so halving the 5-minute-tier drain rate makes an existing capacity question visible rather than creating one. PlanFetchCarryoverKeyTests pins the behaviour, not the shape: debt written under one collector's key is invisible under any other's, still visible under its own, and still separated by server and database. The family is derived by invoking the definitions' own RunsPerDatabase/BuildEnumerationQuery over CollectorCatalog.All, with a non-vacuity floor; the artifacts under test are the dictionaries' own declared key type and the runner's own key builder, both reached by reflection so a key-shape change goes red instead of failing to compile; and erikdarlingdata#2898's shared IL decoder proves both fetch state machines call the builder, with the probe calls as positive controls checked first. Each of the five assertions was proven red by its own mutation of the shipped source. No schema change: the per-phase persistence rung is erikdarlingdata#2860's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2312. ⚠ HOLD FROM MERGE until 3.5.0 cuts — the release candidate is soaking on the box from dev @ 9f01385, and this must not enter the release build. Review rounds proceed as usual; the merge waits.
The mechanism, finally named (Findings 3 & 4, on the issue)
The #2210 second-generation design had four moving parts. Three were built, tested, documented, and never wired: the adaptive sizing (Finding 1, wired in #2322),
TouchSqlliveness (Finding 3 — 'the whole of the protection' for Query Store digests, zero callers), and the re-verify cursor (Finding 4 — only tests invoke it). What ran in production was the budgeted walk alone, withResolve's daily expiry still armed: catalogs whose full walk needs more than a day (measured 15.9–107.5 hours) expired MID-walk, restarted from plan_id 0, and looped the full catalog fetch forever. That is the 40–110s-per-cycle bill — and the loop's re-upserts were also accidentally substituting for the unwired liveness refresh, which is why killing the walk and wiring the touch must land together.The reshape
TouchSql) refreshes map+dimlast_seenAND returns per-idresolved/hash_stale— the missing set is the fetch list, and the live hashes the payload already carries give per-cycle in-place-rewrite detection (plans) and Query-Store-reset detection (texts), retiring both the cursor and the reset arm.BuildPlanFetchByIdsQuery/BuildTextFetchByIdsQuery), chunked ascending, same running-bytes budget with the oversized-admission arithmetic intact, hash columns riding along (no decompression). A caught-up database issues no target query at all — the 23s-to-discover-nothing becomes nothing.query_store_plan_map.digestnullable (NULL-XML plans land a content-less marker so they read as known instead of riding every fetch list — the posted design's one gap),query_store_text.query_hash(reset detector; legacy rows adopt the live hash on first touch, zero refetches), and wholesale deletion of the orphanedplanwm:/textwm:rows. Full rung ceremony including the viewer probe arm and the V74/V75/V76 pin-file demotions.Honesty notes
CapturePlanXml); the shared prune set shrinks 5 → 3 and Lite's no-op deletes shrink with it.Tests
QueryStorePlanWatermarkTestsreborn asQueryStorePlanFetchTests: by-ids builder shape + guards (both fetches), touch-and-probe SQL pins (hourly guard, hash adoption, NULL-digest-resolves, stale expression, dim-touch NULL filter), upsert COALESCE pins, the surviving sizing decision tables, and the read-loop stages-no-state pin. The builders were additionally executed against the built assembly in a throwaway harness (escaping, id lists, budgets, all four guard throws — pass).ActivityDrivenPlanFetchStoreTests: the V77 rung ceremony (ladder top, three strokes, probe arm, three-places-agree).QueryStoreFetchProbeLivePostgresTests(gated): live round-trip of probe verdicts (missing/marker/current), liveness stamping on map AND dim, hash adoption then stale detection, and the writer's NULL-digest marker.QueryStoreTextWatermarkTestsretired with its subject.🤖 Generated with Claude Code