Plan-fetch candidate window and cut-safe watermark advance (#2210) - #2211
Plan-fetch candidate window and cut-safe watermark advance (#2210)#2211erikdarlingdata wants to merge 2 commits into
Conversation
The destination-agnostic half of the #2164 redo: the two pure policies the plan_id-ordered plan fetch needs, which are identical whether the XML lands in the shared plan dim or a standalone table (that call is still open on #2210). Nothing consumes them yet; the fetch and its wiring follow in the same PR once the destination is settled. CandidateWindow is the trap mitigation. A running DATALENGTH total has to materialize the XML to measure it, so an unbounded candidate set pays a whole catalog's decompression to enforce a budget meant to prevent that. The window is adaptive per database because measured plan size spans 11x across the fleet (per-quartile averages 162/80/39/15 KB over 2,166 budget-cut passes): a constant sized for the small-plan end would decompress ~134MB to ship 12MB on the large-plan end, and one sized for the large end never reaches the budget on the small end. It reports when a bound clamped it so the caller can log rather than cap silently. First contact assumes LARGE plans, because the estimate is a divisor and small windows are the safe direction. AdvanceWatermark is why the redesign exists: under plan_id-ordered shipping a budget cut truncates a SUFFIX, so the highest landed id is safe even from a cut pass. The shipped design ships in last_execution_time order, where a cut leaves an arbitrary subset, no value is safe, and the resulting guard meant the watermark could not advance on 97.8% of passes -- so it never advanced at all. The ordering guard was wrong on its first pass and the scratch harness caught it: honouring the leading ascending run advances to 105 given {105, 101}, and with ordering broken there is no basis for inferring every selected plan below 105 landed. A descent now abandons the advance, and ArrivedInPlanIdOrder lets the caller log the violation instead of watching a watermark quietly stop. Verified by compiling the real source file into a throwaway net10.0 console and running the cases, since Darling.Tests is net10.0-windows and cannot run here -- the pinned tests in this commit are the same cases and CI executes them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public static int CandidateWindow(long? observedAvgPlanBytes, long budgetBytes, out bool clamped) | ||
| { | ||
| var avg = observedAvgPlanBytes is long observed && observed > 0 ? observed : FirstContactAvgPlanBytes; | ||
|
|
||
| if (budgetBytes <= 0) | ||
| { | ||
| clamped = true; | ||
| return MinCandidateWindow; | ||
| } | ||
|
|
||
| /* double for the margin, then one bounds check — the product cannot overflow int at any budget the knob | ||
| accepts, but the cast is guarded anyway because the budget is operator input. */ | ||
| var wanted = (double)budgetBytes / avg * CandidateWindowMargin; | ||
| var rounded = wanted >= MaxCandidateWindow ? MaxCandidateWindow : (int)Math.Ceiling(wanted); | ||
|
|
||
| clamped = rounded >= MaxCandidateWindow || rounded <= MinCandidateWindow; | ||
| return Math.Clamp(rounded, MinCandidateWindow, MaxCandidateWindow); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The watermark a pass earned, given the plan_ids whose XML actually landed. Under plan_id-ordered | ||
| /// shipping a budget cut truncates a SUFFIX, so the highest landed id is safe to keep even from a cut pass | ||
| /// — which is the whole point of the reordering (#2210): the previous design shipped in | ||
| /// <c>last_execution_time</c> order, where a cut left an arbitrary SUBSET and no value was safe, so the | ||
| /// watermark could not advance on 97.8% of passes and therefore never advanced at all. | ||
| /// | ||
| /// <para>Defensive on the precondition rather than trusting it: a DESCENT anywhere in | ||
| /// <paramref name="landedPlanIdsInOrder"/> abandons the advance entirely and returns | ||
| /// <paramref name="standing"/>, which the caller should log. Honouring the leading ascending run instead | ||
| /// looks safer and is not — given <c>{105, 101}</c> it would advance to 105, and once ordering is broken | ||
| /// there is no longer any basis for inferring that every SELECTED plan below 105 landed, so a plan whose | ||
| /// XML never arrived gets suppressed until the refresh horizon. Ordering is what makes a cut a suffix; with | ||
| /// it gone the pass has earned nothing, and one lost pass of progress is the cheap side of that trade.</para> | ||
| /// | ||
| /// <para>Never moves backward: a pass that lands nothing, or only ids at or below the standing watermark, | ||
| /// returns the standing value. Lowering it would refetch the catalog, and "no new plans this window" is an | ||
| /// ordinary quiet pass, not a reset — the reset signal lives on the runtime stream, where a plan at or below | ||
| /// the watermark that the store has never resolved can actually be observed.</para> | ||
| /// </summary> | ||
| public static long AdvanceWatermark(long standing, IReadOnlyList<long> landedPlanIdsInOrder) |
There was a problem hiding this comment.
AdvanceWatermark and ArrivedInPlanIdOrder are two separate pure functions with no enforcement that a caller invokes both together. Since AdvanceWatermark silently swallows the descent case (just returns standing, no signal beyond the return value), it's easy for a future caller to invoke only AdvanceWatermark and never notice a plan_id ordering violation — which is exactly the "watermark quietly stops moving" failure mode this whole redesign (#2164) exists to prevent, just moved one level up (from "the watermark can't advance" to "the violation isn't logged").
Since the wiring lands in a follow-up PR, worth considering merging these into one call (e.g. return a small struct/tuple (long Watermark, bool OrderViolated)) so the two facts can't be pulled apart at a call site by accident. Not a bug in this PR — there's no caller yet — but worth deciding before the fetch is wired up rather than after.
| /// <para><paramref name="clamped"/> reports that a bound was applied, so the caller can LOG it. A window | ||
| /// silently pinned at its ceiling looks identical to one that fit, and that is how a cap becomes invisible.</para> | ||
| /// </summary> | ||
| public static int CandidateWindow(long? observedAvgPlanBytes, long budgetBytes, out bool clamped) |
There was a problem hiding this comment.
Naming collision: Darling/PerformanceMonitor.Darling.Service/QueryStoreBackfill.cs:82 already has a private static readonly TimeSpan CandidateWindow = TimeSpan.FromDays(7); with a completely unrelated meaning (a time span for the backfill scan, vs. a plan count here). Different classes so it compiles fine, but the identical name for two unrelated "candidate window" concepts in the same subsystem (query_store fetch/backfill policy) will make grepping and code review confusing. Consider a more specific name here, e.g. CandidateWindowPlanCount or PlanCandidateWindow.
| /// <summary> | ||
| /// The candidate window sits just past what the budget can actually ship, at every plan size the fleet | ||
| /// ACTUALLY exhibits — per-quartile averages of 162 / 80 / 39 / 15 KB measured across 2,166 budget-cut | ||
| /// passes. The point of the pin is that none of these clamp: if a real fleet plan size hit a bound, the | ||
| /// bound would be doing the sizing instead of the measurement. |
There was a problem hiding this comment.
Continuation lines of this (and several other) new <summary> blocks in this file are indented 7 spaces before /// instead of matching the 4-space indent of the opening /// <summary> line (e.g. lines 395-397, 419-420, 457, 481-483, 505-507). It doesn't break the doc comment, but it's inconsistent with the rest of this file and with the clean alignment used in the actual QueryStorePlanXmlState.cs doc comments added in this same PR. Worth a pass to re-align.
|
Reviewed. This PR only touches Correctness: I hand-traced every Security / Performance: N/a for this PR — no I/O, no external input handling, no SQL. Pure in-memory policy functions; nothing here can regress performance since nothing calls them yet (by design, per the PR description). Left three inline notes, none blocking since this is explicitly pre-wiring:
|
Review notes on #2211, all three. The real one: AdvanceWatermark and ArrivedInPlanIdOrder were separate, so a caller could take the watermark and never ask whether ordering held -- a watermark that quietly stops moving with nothing logged, which is the exact failure this redesign exists to correct. They now come back together in one PlanWatermarkAdvance, so the signal cannot be forgotten rather than merely being documented as important. CandidateWindow collided with an unrelated TimeSpan of the same name in QueryStoreBackfill.cs (a backfill scan span, not a plan count). Renamed to CandidatePlanCount / MinCandidatePlans / MaxCandidatePlans / CandidatePlanMargin, which names the unit and kills the ambiguity rather than just the collision. Doc-comment continuation lines in the test file were indented 7 spaces before ///, an artifact of how I inserted them. Normalised to 4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three addressed in The decoupling risk — fixed structurally, not documented. You're right that it's this redesign's own failure mode moved up a level: a watermark that quietly stops with nothing logged is exactly what #2164 spent two attempts not noticing, and "the caller should also call the other function" is the kind of instruction that holds until the day it doesn't. Naming collision — renamed to name the unit. Doc indentation — fixed. Artifact of how I inserted those blocks; normalised to 4 spaces. Re-verified the coupled API through the same throwaway One note on the previous run's red Still draft, still waiting on the destination question over on #2210 before the fetch and its wiring land here. |
|
Independently re-reviewed the current state (post
The three items from the earlier pass (watermark/ordering-verdict decoupling risk, the No new issues found. Still fine as a draft pre-wiring PR. |
1 participant
Draft on purpose — this is half a change. Part of #2164 / #2210. It adds the two pure policies the plan_id-ordered plan fetch needs and nothing that calls them, because the destination question on #2210 (shared plan dim vs a standalone table) is still open and the fetch's wiring depends on the answer. The policies do not: they are identical either way, which is the only reason it was worth building ahead of the decision.
Putting it up now so the math can be reviewed while that question resolves rather than arriving as one large PR later.
CandidateWindow— the trap mitigation, adaptive per databaseSUM(DATALENGTH(query_plan)) OVER (ORDER BY plan_id)has to materialize the XML to measure it, so an unbounded candidate set pays a whole catalog's decompression to enforce a budget that exists to prevent exactly that.TOP (@k)on the cheap columns bounds it first.@kcan't be one fleet constant, because measured plan size spans 11x — per-quartile averages of 162 / 80 / 39 / 15 KB across 2,166 budget-cut passes on the 52-server box. A constant sized for the small-plan end (~820) would decompress ~134MB to ship 12MB on the large-plan end; one sized for the large end never reaches the budget on the small end. So it's derived per database from the previous pass's ownshipped_bytes / plans_shipped— free, since both numbers are in hand when a pass ends, and no probe can measure plan size without decompressing.Sanity check against the measured fleet, 12MB budget:
None of them clamp, which is the point of pinning it — if a plan size the fleet actually exhibits hit a bound, the bound would be doing the sizing instead of the measurement.
Margin is a deliberate 1.5x and not more: a windowed running total is evaluated over every row in the window, so the server decompresses all K plans whether the budget is reached at plan 5 or plan 500. Margin buys reachability and costs decompression. For the same reason first contact assumes LARGE plans (160KB) — the estimate is a divisor, so over-stating plan size yields a small window, and small is the safe direction.
Clamps report themselves via an
out boolso the caller can log. A window silently pinned at its ceiling reads exactly like one that fit.AdvanceWatermark— why the redesign exists at allUnder plan_id-ordered shipping a budget cut truncates a suffix, so the highest landed id is safe to keep even from a cut pass. The shipped design ships in
last_execution_timeorder, where a cut leaves an arbitrary subset, no single value is safe, and the guard that follows meant the watermark could not advance on 97.8% of passes — so it never advanced at all. That is the whole bug, and this function is the fix.The bug the scratch harness caught
My first ordering guard honoured the leading ascending run. Given
{105, 101}that advances to 105 — and once ordering is broken there is no basis for inferring that every selected plan below 105 landed, so a plan whose XML never arrived would be suppressed until the refresh horizon. Silent data loss, in the guard meant to prevent it.A descent now abandons the advance entirely and returns the standing watermark.
ArrivedInPlanIdOrderis separate so the caller can log the violation instead of watching a watermark quietly stop moving, which is the failure mode #2164 spent two attempts not noticing.Verification, and its limits
Darling.Testsisnet10.0-windows, so I can't run it here. I compiledQueryStorePlanXmlState.cs— the real file, not a copy — into a throwawaynet10.0console and executed every case: the four measured plan sizes, both clamps, a zero budget, a negative observed average, the estimator round-tripping the measured quartiles (12.1MB/78 plans → 158KB against a measured 162KB), and all eight watermark cases. The pinned tests here are those same cases; CI executes them.What is not verified: nothing has run the actual fetch against a real Query Store, because the fetch isn't written yet. The SQL and the wiring land in this PR once #2210's destination call is made, and I'll measure against production catalogs before it comes out of draft — same as #2183, which is also the change that taught me a green CI on this optimization proves nothing about whether it does anything.