Pin the sweep-body detach policy and the 1-minute-tier invariant (refs #2840) - #2843
Conversation
… (refs #2840) #2700/#2717 detach query_store and plan_correction from the sequential per-server collection body. That set had no test, and the criterion for admitting a third collector was recorded nowhere. Measured against production collection_log on use1 over 24h, the existing set is exactly right: query_store p90 65,053ms and plan_correction max 133,934ms are the only per-cycle cost outliers. index_object_stats reaches p90 15,640ms but runs on a 1440-minute cadence, so its body impact amortises to nil. The intuitive derivation from enumerated-vs-scalar fanout is disproved in both directions: database_scoped_config fans out to 10 databases at p90 127ms, while procedure_stats has zero fanout at p90 11,964ms. A fanout-derived split would detach the cheap collector and leave the expensive one starving the tier. The pin that generalises asserts no detached collector sits on the 1-minute tier. A detached run SKIPS when its previous run is still in flight, so detaching a 1-minute collector converts starvation into guaranteed misses - which is what would happen if the residual ~1.5-minute floor (#2841) were addressed by detaching procedure_stats. Proven red both ways against the real built assembly, controls passing on every build: detaching procedure_stats fails pin1+pin2; removing the plan_correction detach fails pin2+pin3.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// has zero fanout at p90 11,964ms. A fanout-derived split would detach the cheap collector and leave | ||
| /// the expensive one starving the tier. See #2840.</para> | ||
| /// | ||
| /// <para><b>The 4.5x evidence.</b> use2 runs the same Balanced preset with Query Store dead since |
There was a problem hiding this comment.
Minor: FrequencyMinutes < 2 also matches FrequencyMinutes == 0 (the on-load-only config-snapshot tier), not just the 1-minute tier. A 0-cadence collector runs once per connect, so detaching one can't cause the starvation-by-skipped-relaunch this invariant is guarding against — but if one were ever added to the detach set, this test would fail and point at "sits on the 1-minute tier," which would be a misleading diagnosis for that case. Not a blocking issue given today's fixed detach set (query_store, plan_correction, both cadence 5), just worth a > 0 && < 2 or a comment noting 0 is deliberately included as "no relaunch cadence to skip against" if that's intentional.
There was a problem hiding this comment.
Good catch, fixed in the follow-up commit — narrowed to FrequencyMinutes is > 0 and < 2 with a comment naming why 0 is excluded: an on-load-only collector runs once per connect, so there is no relaunch to skip and detaching one is harmless. You were right that it would have failed with the wrong diagnosis.
Not theoretical either — grep -c "= new(0, " on CollectorScheduleDefaults returns 5, so there are five collectors that would have tripped it.
Re-proved red after the change: detaching procedure_stats (cadence 1) is still caught, so narrowing did not weaken the pin.
Review summaryThis is a test-only PR (plus a CHANGELOG entry) — no production code changes, so no T-SQL or Lite/Darling parity drift to flag. Verified:
Left one non-blocking inline nit on the |
…d-only tier claude[bot] caught that FrequencyMinutes < 2 also matches 0, the on-load-only config-snapshot tier. A 0-cadence collector runs once per connect and has no relaunch cadence to skip against, so detaching one cannot cause the starvation-by-skipped-relaunch this invariant guards - it would fail the test with a misleading diagnosis. Five collectors in the catalog carry cadence 0, so the distinction is real rather than theoretical. Re-proved red: procedure_stats (cadence 1) is still caught by the narrowed predicate.
|
Reviewed. This is a test-only PR (new Verified against the current tree:
No correctness, security, or performance concerns. Good addition of missing test coverage for existing (already-shipped) behavior, with a criterion that will actually fail if someone tries to detach a 1-minute-tier collector later. |
My CHANGELOG entry cites [#2849] for the cadence model, but that issue had no link-reference line, so it rendered as literal text rather than a link. [#2862] and [#2843] were added with the entry; this is the third. Note for whoever touches that block next: [#2860], [#828] and [#887] each have DUPLICATE reference lines, and those predate this branch (confirmed against origin/dev). Left alone - #2889 is rewriting the block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a#2862) procedure_stats is the most expensive collector on the production us-east-1 store - 129.9M ms of target-side duration over 32,093 runs in 24 h, and 66% of the entire 1-minute collection body on a sampled sweep (4,735 ms of 7,221 ms) - and almost all of it is the read loop draining plan XML. A controlled decomposition split that read loop into RENDER 73.8% / TRANSFER 26.0%, against 0.2% for the same query with no plan apply at all. The render happens inside sys.dm_exec_text_query_plan, a SERVER-side TVF, so three quarters of the cost is CPU burned on the monitored production server - which is why the lever is cadence rather than a dedup key: hashing at the source removes the transfer and leaves the render on customer hardware. A plan_handle-keyed probe is separately disproven (8.5 distinct plan XMLs per handle per day). The schedule is untouched. erikdarlingdata#2843 pinned that no detached collector sits on the 1-minute tier, because a detached run skips while its predecessor is still in flight, so moving this collector off that tier would convert starvation into guaranteed misses. Gating the render instead keeps runtime statistics at full 1-minute resolution and amortises only the expensive half. New file-only darling.json knob procedureStatsPlanCycleInterval (default 4, clamped [1,60], read through a live provider). NO schema rung: it follows collectSchemaChangeEvents, the established file-only-knob precedent, and StoreConfigProvider.ApplyToConfig mutates the held config field by field, so a value absent from the store view survives a reload untouched. 1 is byte-identical to the old collector. The phase derives from server_id so the fleet staggers instead of spiking together - [10, 10, 11, 11] shipped against [42, 0, 0, 0] with a bare modulo, reflected out of the built assembly. That is also what makes the in-memory counter safe across a fleet-wide restart, so no state is persisted and collect.collector_state is not needed. No reader changes: the erikdarlingdata#1767 guard already skips NULL-plan rows and returns the latest row that has a plan. Modelled against erikdarlingdata#2849's cadence equation the change alone takes us-east-1 from ~113 s to ~82 s and cannot reach 60 s at max_concurrent_sweeps = 4, where the floor is ~72 s even at an infinite interval. The tail finding in erikdarlingdata#2862 is untouched and stays open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
The split already exists — this pins it instead
#2840 was filed to build a deep-sweep split. It shipped on 2026-08-30/08-31: #2700 detached
query_storefrom the sequential per-server collection body and #2717 did the same forplan_correction, both fire-and-forget behind per-(server, collector) gates. Both were in thenightly the boxes have run since 09-02.
So there was nothing to build. What was missing is that the detach set is two hardcoded name
checks with no test at all —
grep -rn Detached Darling/Darling.Tests/returns nothing — and thecriterion for admitting a third collector is recorded nowhere.
The set is correct, measured
collect.collection_logon the use1 monitoring host, 24 h to 2026-09-03:data_retentionquery_storeindex_object_statsprocedure_statsquery_statsplan_correctionquery_store_healthdatabase_scoped_configNothing qualifies for admission.
index_object_statsreaches p90 15,640 ms but runs once per serverper day, so its body impact amortises to nil.
data_retentionruns from the purge path, not thisloop.
procedure_statsis the second-worst collector on the box and is deliberately left in thebody — see the invariant below.
The intuitive derivation is disproved in both directions. Splitting on enumerated-vs-scalar
fanout would detach
database_scoped_config(fanout 10, p90 127 ms) and leaveprocedure_stats(fanout 0, p90 11,964 ms) starving the tier. The criterion is measured p90 against the fast
tier's cadence, not collector shape.
The pin that generalises
NoDetachedCollectorSitsOnTheOneMinuteTier. A detached run skips when its previous run is stillin flight — that is what makes fire-and-forget safe for a 5-minute collector, and what makes it
unsafe for a 1-minute one, where it converts starvation into guaranteed misses.
This is what would fire if someone addressed the residual ~1.5-minute floor ([#2841]) by detaching
procedure_statsrather than by changing how the body executes.Red-first, both directions
A throwaway
net10.0harness against the real built assembly (Darling.Testsisnet10.0-windowsand cannot run on macOS; the harness takes that assembly name so
InternalsVisibleToexposes thepredicates):
Controls resolve through the same predicate and catalog lookup and pass on every build, so a
predicate that stopped resolving would fail them too rather than passing silently.
The first mutation attempt was caught by its own
assert: the anchors used\nagainst a CRLF file,countcame back 0, and the harness then "passed" on unmutated code. Worth recording — that is thefalse-green shape in a red-first proof.
What this does NOT do
It does not fix the starvation #2840 measured. That has two remaining causes, neither of which is
body ordering:
collectors due per minute. The detach removed
query_storefrom the critical path; the work stillruns on the same box and the rest of the body is genuinely slower.
surplus at the 2304 cap, i.e. throttled to 40% baseline; they were resized to m7i.2xlarge at
13:37–13:44Z. Pre-resize use1 ran 6.7× worse than use2; post-resize 1.7×, against a body-duration
ratio of 3.1×.
The residual floor is [#2841], deliberately separate. The
Balancedpreset still promises 1 minuteand structurally cannot deliver it, but the honest number is unknown until the floor moves, and
CollectorSchedulePresetsis duplicated from Lite'sScheduleManager.s_presets, so it needs theparity treatment when it changes. Not touched here.
Refs #2840. Test-only; no behaviour change.