Repository navigation
Detach plan_correction from the sequential body, generalizing #2701 - #2720
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| using var detachedGate = IsPlanCorrectionCollector(collectorName) | ||
| ? _detachedCollectorGates.GetOrAdd((runtime.ServerId, collectorName), static _ => new DetachedCollectorGate()).TryAcquire() | ||
| : null; | ||
|
|
||
| if (IsPlanCorrectionCollector(collectorName) && detachedGate is null) | ||
| { | ||
| _logger.LogInformation( | ||
| " [{Server}] {Collector} skipped this tick — a previous detached run has not finished (#2717). " + | ||
| "Re-reads the live set next tick; no rows are lost.", | ||
| server.Config.DisplayName, collectorName); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Minor maintainability nit: IsPlanCorrectionCollector(collectorName) is evaluated twice here — once to decide whether to acquire the gate, once more to decide whether a null result means "busy" vs. "not applicable to this collector." That's the exact ambiguity QueryStoreServerGate.NotGated exists to eliminate a few lines up (its doc comment: "Without it, 'not gated' and 'gate busy' would both be null and every such site would need to re-test the predicate to tell a skip from a pass-through; getting that wrong silently skips a collector nobody meant to gate").
Today's code re-tests the predicate correctly, so there's no live bug. But DetachedCollectorGate explicitly models itself on QueryStoreServerGate ("Same CompareExchange-flag shape... and for the same reason") without adopting its NotGated sentinel, and the doc comment above invites a "future third collector" to extend this condition. If that extension only updates the ternary at line 5037 (to acquire the gate) but not the if at line 5041 (or vice versa), the two checks silently drift apart — reintroducing precisely the failure mode NotGated was built to prevent.
Worth either:
- hoisting
IsPlanCorrectionCollector(collectorName)into a localboolused in both places, or - adding a
DetachedCollectorGate.NotGatedsentinel mirroringQueryStoreServerGate.NotGated, so the check collapses to a singleif (detachedGate is null).
|
Reviewed. C#-only change (no T-SQL touched), scoped to Darling's One maintainability nit posted inline on Lite/Darling parity note (not a blocker, flagging per review scope): That said, #2701 (the original query_store detach) was itself titled "Darling: split query_store..." and never touched Lite, so this PR isn't introducing new drift so much as continuing an existing, apparently deliberate scoping decision. Given Lite's smaller typical fleet size and 1-minute collection interval, the cost/benefit may genuinely differ from Darling's fleet-wide service. Worth a maintainer call on whether that's intentional (e.g., filed as a Lite follow-up) or worth doing now while the generic gate type already exists and is shared. No correctness bugs, security issues, or T-SQL conventions concerns found (no SQL was touched). |
…2717) plan_correction's own SQL has no further tuning headroom: it's already correctly seek-based post-#2687, averaging ~1 second. But on a server whose Query Store carries an abnormally large distinct-plan population (the same leaflogix-class decimal-parameter-instability signature already root-caused for query_store elsewhere in the fleet), it can spike to 20+ seconds - confirmed on one affected server: avg 1057ms, max 21351ms, 97% of that worst cycle attributable to one database (independently confirmed via query_store_plan_map: 36,232 distinct plan_ids, 28,260 distinct digests). That is the identical bimodal shape #2701 detached query_store for, and plan_correction was never included in that fix - it was still awaited inline in RunDueCollectorsAsync's sequential foreach, still able to block every other due collector on an affected server. Generalizes the fix instead of duplicating it: DetachedCollectorGate is a collector-agnostic sibling of QueryStoreServerGate, keyed by (server, collector name) so a future third collector needs only its own IsXCollector check. QueryStoreServerGate itself is untouched - it has an orthogonal second job (excluding the separate backfill loop) this type doesn't need to solve. RunDetachedQueryStoreAsync is renamed RunDetachedAsync and shared by both detached collectors rather than duplicated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5f7af14 to
ccbc400
Compare
| using var detachedGate = IsPlanCorrectionCollector(collectorName) | ||
| ? _detachedCollectorGates.GetOrAdd((runtime.ServerId, collectorName), static _ => new DetachedCollectorGate()).TryAcquire() | ||
| : null; | ||
|
|
||
| if (IsPlanCorrectionCollector(collectorName) && detachedGate is null) |
There was a problem hiding this comment.
IsPlanCorrectionCollector(collectorName) is evaluated twice — once to decide whether to acquire, once more here to tell "not gated" apart from "gate busy," since both cases produce detachedGate == null.
QueryStoreServerGate solved this exact ambiguity with a NotGated sentinel (see its doc comment: "Without it, 'not gated' and 'gate busy' would both be null and every such site would need to re-test the predicate to tell a skip from a pass-through; getting that wrong silently skips a collector nobody meant to gate."). DetachedCollectorGate doesn't have an equivalent, so this call site re-implements the exact pattern that sentinel exists to avoid.
It's harmless today because both sites test the same single predicate, but the comment above (and in DetachedCollectorGate's own doc) explicitly anticipates a third detached collector being added here via an ||. Whoever adds it has to remember to update both the ternary and this if with the identical OR-expression — miss one and it either never skips (double-run risk) or skips a collector that was never meant to be gated. Worth either adding a DetachedCollectorGate.NotGated-style sentinel, or at least computing the predicate once into a local and reusing it in both places.
|
Reviewed the C# changes (no T-SQL in this diff). One correctness/maintainability nit posted inline; the item below is a scope question rather than a bug in the code as written. Lite/Darling parity:
|
…ia NotGated sentinel DetachedCollectorGate modeled itself on QueryStoreServerGate's CompareExchange shape without adopting its NotGated sentinel, so the gate-acquisition site tested IsPlanCorrectionCollector twice (once in the ternary, once in the following if) to tell "not gated" apart from "gate busy" — both produce null. Add DetachedCollectorGate.NotGated mirroring QueryStoreServerGate.NotGated so the check collapses to a single `if (detachedGate is null)`, matching the adjacent queryStoreGate idiom in the same method and removing the two-call-sites-must-stay- in-sync risk the surrounding doc comment already flags for a future third collector. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| /// awaiting it in the sequential foreach, single-flighted per server so a still-running previous tick | ||
| /// skips rather than overlaps.</para> | ||
| /// | ||
| /// <para><b>#2717 is the second.</b> <c>plan_correction</c> (1-minute cadence, a <c>PerItemWallClockBudget</c> |
There was a problem hiding this comment.
Nit: this says plan_correction is "1-minute cadence", but its schedule default is 5 minutes (CollectorScheduleDefaults.cs: ["plan_correction"] = new(5, 30)), same tier as query_store — which this same doc comment correctly calls "5-minute cadence" two paragraphs up. Given how much weight this codebase's comments carry as documentation (issue numbers, exact timings), worth fixing so it doesn't mislead a future reader who trusts it over the schedule table.
|
Reviewed the diff ( Correctness — the generalization looks sound. Left one inline nit: a doc comment in the new Lite/Darling parity — no drift introduced by this PR specifically. Lite's Security / other — no external input, network, or process boundaries touched; this is pure in-process scheduling/concurrency control. The new test file's |
… not 1) Same tier as query_store, per CollectorScheduleDefaults.cs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Reviewed the diff ( One parity question worth a look, not necessarily a blocker: Lite never got the "detach from the sequential body" treatment at all — for either collector. I don't think this PR needs to fix Lite's copy — it's consistent with how #2701 shipped (Darling-only), and Lite's single/few-server desktop usage is lower-stakes than Darling's unattended fleet loop. But since this PR doubles the number of collectors carrying the fix in Darling while Lite still has zero, it might be worth a tracking issue so the gap doesn't keep widening silently. Minor: no |
1 participant
Fixes #2717.
Problem
Erik flagged plan_correction's Collector Cost Regression on one fleet server (217,072 ms/day, 3.4x its 14-day baseline) and asked for further perf tuning beyond #2687/#2688.
Investigation found plan_correction's own SQL has no further tuning headroom. It's already correctly seek-based post-#2687 (avg 1057ms). But
get_collection_healthshows a max of 21,351ms, with 97% of that (20,755ms) attributable to one database. Checkedcollect.query_store_plan_mapdirectly for that database: 36,232 distinct plan_ids, 28,260 distinct digests — the identical application-class decimal-parameter-instability signature already root-caused for query_store on other servers earlier tonight. This is a data-volume problem downstream of an already-tracked app-side bug, not a query-shape defect in plan_correction.The actual actionable finding: plan_correction's cost shape (avg ~1s, occasional 20+s spike depending on the affected server) is exactly the bimodal shape #2701 fixed for
query_store(avg 5-35s, occasional 100-230s spike) — which caused query_store to block every other due collector sharing its per-server sequential body and risk BODY_OVERRUN. #2701's fix applies only to query_store. plan_correction was still awaited inline inRunDueCollectorsAsync, still able to block other collectors on an affected server for the duration of its own spike.Fix
Generalizes #2701's detach pattern instead of duplicating it for a second collector:
DetachedCollectorGate(new,PerformanceMonitor.Common): a collector-agnostic sibling ofQueryStoreServerGate, same CompareExchange-flag single-flight shape, never blocks.QueryStoreServerGateitself is untouched — it has an orthogonal second job (mutual exclusion against the separate first-contact backfill loop) this type doesn't need to solve._detachedCollectorGatesinDarlingWorker: keyed by(int ServerId, string CollectorName), so a future third collector detached this way needs only its ownIsXCollectorcheck — the dictionary already generalizes.IsPlanCorrectionCollectormirrorsIsQueryStoreCollector's renaming-safety pattern (compared against the collector's own declaredName).RunDetachedQueryStoreAsyncrenamedRunDetachedAsyncand shared by both detached collectors rather than duplicated — its body (contain a shutdown-timeOperationCanceledException) was already fully generic.RunOneAsyncgates plan_correction through the new generic gate the same way it already gated query_store through its own.plan_correction is safe to detach for the same reason query_store is: its window isn't wall-clock derived. It re-reads
sys.dm_db_tuning_recommendationswhole on every successful pass (no persisted watermark to invalidate), so a skipped tick just re-reads the current — possibly since-refreshed — live set next time. No rows are lost, only deferred.Test plan
dotnet build(project + full solution) clean, 0 errors, no new warningsDetachedCollectorGateTests.csadded, mirroringQueryStoreServerGateTests.cs's five properties (exclusion, release-lets-next-in, never-blocks, double-dispose-safety, single-holder-under-contention)Lite.Tests/Darling.Testslocally (net10.0-windows, this Mac lacks the WindowsDesktop runtime) — verified all 14 assertions pass against the real, shippedDetachedCollectorGatetype via a throwawaynet10.0console harness referencing the actual project (not a retyped copy), per this repo's documented macOS verification practice. Relying on CI for the full xUnit run.FleetIdentifierScrubTests, since this repo is public — replaced with generic language) and a stacked doc-comment block left over from renamingRunDetachedQueryStoreAsynctoRunDetachedAsync(DocCommentHygieneTests). Commit amended and force-pushed to a branch nothing else was based on.🤖 Generated with Claude Code