Part of #2862

[erikdarlingdata]

Part of #2862 — the median finding only. The tail finding (the 120 s stall that is not capacity-shaped) is untouched and #2862 should stay open for it.

What changed

procedure_stats renders execution-plan XML on one cycle in N instead of on every cycle. The collector stays on its 1-minute schedule, so runtime statistics keep full resolution; only the expensive half is amortised.

The schedule is deliberately not touched. #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 converts starvation into guaranteed misses. And changing its default frequency is a CollectorScheduleDefaults change, duplicated into Lite's ScheduleManager.s_presets, needing the parity treatment that PR CI would not catch. Gating the render avoids both.

Why cadence, and not a dedup key

sys.dm_exec_text_query_plan is a server-side TVF, so the render is CPU on the monitored production server. Hashing at the source would remove the 26% transfer and leave the 74% render burning on customer hardware. The full A/B/C decomposition, the disproof of the plan_handle probe (8.5 distinct plan XMLs per handle per day), and the ABANDONED-cycle evidence are in #2862.

No schema rung — and how that was confirmed rather than assumed

The knob is procedureStatsPlanCycleInterval in darling.json, default 4, clamped [1,60], read through a live provider. No migration, no StorageVersion bump, no config_service column. Two things make that sound:

• It follows collectSchemaChangeEvents, which is already a file-only knob read through a live provider — the established precedent, named as such in its own doc comment.
• StoreConfigProvider.ApplyToConfig mutates the held DarlingConfig field by field. A property absent from StoreConfigView is never written, so a control-plane reload cannot clobber the file value. Verified by reading the method, not inferred.

The trade-off, stated plainly: a change needs a service restart rather than taking effect live. Promoting it to a store column later is a change to the source only — the runner already reads it through Func<int>, so the collector seam does not move.

The gating state also needs no storage. The cycle counter is an in-memory ConcurrentDictionary keyed (ServerId, Collector). collect.collector_state was considered and rejected: persisting the counter buys nothing, because the fleet stagger comes from server_id and not from accumulated drift, and it would cost a store write per server per cycle on the hot path.

Design decisions worth reviewing

The phase derives from server_id, and it is load-bearing. A bare ordinal % interval puts all 42 servers on the same capture cycle — three cheap cycles, then the whole fleet paying full render cost at once. That is a 4x spike, and peak is exactly what produces the 120 s PerItemWallClockBudget abandonments, so the naive form would be worse than collecting every time. Shipped [10, 10, 11, 11] against the counterfactual [42, 0, 0, 0], both reflected out of the built assembly.

That is also what makes the in-memory counter safe. A fleet-wide restart re-enters every server at ordinal 0 simultaneously, and the server-derived phase still spreads them across the interval. Pinned as the observable property (how much of the fleet captures on the first cycle back), not by calling a pure function twice.

The gated cycle omits the OUTER APPLY entirely rather than rendering and discarding — the difference between the measured 0.2% floor and paying the whole 73.8%. It rides the existing CollectorContext.CapturePlanXml seam, which erases both plan placeholders, and it is pinned on the SQL the collector actually builds.

The wiring is one named seam, not a bare &&. ShouldCapturePlanXmlFor composes the SKU flag and the cadence gate, and CollectorContext.CapturePlanXml is assigned from it. A conjunction inside an object initializer is droppable by a refactor with every pure-policy pin still green; that failure mode was proven red before this shape was chosen.

Only the storing path is gated. FetchRowsAsync — the on-demand live fetch an operator asked for by name, which writes nothing — still renders unconditionally.

Why N = 4, derived rather than picked. Two bounds meet there:

• Marginal return collapses as (N-1)/N. N=4 captures 75% of the achievable saving; N=8 captures 87.5% for double the staleness, and moves modelled cadence only ~82 s → ~77 s against ~113 s → ~82 s for going 1 → 4.
• A hard bound from the collector's own SQL: candidates are filtered s.last_execution_time >= DATEADD(MINUTE, -10, GETDATE()). If interval x cadence exceeds that 10-minute window, a module can enter and leave the candidate set between two captures and never have its plan rendered. At N=4 and ~82 s that is 5.5 min, comfortably inside; at N=8 and ~77 s it is 10.3 min, past it. So the knob is capped near 7 by design, independent of taste, and 4 leaves margin for cadence regression.

The first capture after a gap is not more expensive. The TOP (150) candidate set is already saturated on the servers that matter: two separate heavy servers reported rows_collected = 150 on every procedure_stats run sampled (8 consecutive runs on one, 3 on the other), and the fleet mean is 140.9 rows against the 150 cap. So a skipped cycle cannot enlarge the set. Stated precisely because the fleet mean is not 150 — some lighter servers return fewer, and for those the saving is smaller but the gap is also not a risk.

The cost paid, and the consumers checked

Plan-data granularity: worst-case plan age goes from ~1.9 min to ~5.5 min. Runtime statistics are unaffected — collected every cycle as before.

• ViewerDataService.Plans.cs and Mcp/DarlingStoredPlanReader.cs both carry the query_text/query_plan_xml stored inline per row: 94% of a field store — normalize into hash-keyed dimension tables (~135x measured) #1767 guard AND (COALESCE(ps.query_plan_xml, qpd.query_plan_xml) IS NOT NULL OR qpd.query_plan_gz IS NOT NULL) ORDER BY ps.collection_time DESC LIMIT 1 — they skip NULL-plan rows and return the latest row that has a plan, with no time bound, so a skipped cycle is invisible. This is what analyze_procedure_plan reads through.
• PgCollectorRowWriter.Value short-circuits before PayloadDimensions.Digest: "A NULL payload stays NULL — there is nothing to dedupe and no dim row to point at."
• One consumer worth naming that the earlier pass did not. ViewerDataService.ProcedureStats.cs computes bool_or(query_plan_xml IS NOT NULL OR query_plan_digest IS NOT NULL) AS has_query_plan over the grid's time window to gate the per-row Download button. Its own comment says it matches what GetProcedureStatsPlanXmlAsync fetches on, but that fetch has no time bound while this flag does — so the flag is the conservative one, and on a grid window shorter than interval x cadence (~5.5 min at N=4) it can read false for an object whose plan the fetch would still find. Pre-existing asymmetry, made marginally more reachable; not addressed here.
• PgFactCollector.QueryPerf.cs's ProcedureStatsSql uses only delta_* runtime columns — no plan columns — so the analysis layer is unaffected. Read only; that file belongs to another lane.

Verification

Darling.Tests cannot execute on macOS, so the pin's real source was compiled into a throwaway net10.0 xunit host (assembly-named Darling.Tests to reach InternalsVisibleTo) referencing the built DLLs, not the projects. 17/17 green.

Every pin was proven red first, each variant failing a different assertion:

mutation	assertion that failed	observed
gate removed from the wiring seam	TheInstanceGate_LetsProcedureStatsCaptureExactlyOneCycleInN	expected 10, actual 40
gate inverted	ExactlyOneCycleInN_Captures	expected 20, actual 40
server_id phase dropped	TheFleetIsStaggered... / ...StaggersTwoServersOntoDifferentCycles	per-cycle 42, 0, 0, 0
clamp ceiling pushed 60 → 600	TheKnobClampsToItsDocumentedRange...	expected 60, actual 600
gated path still renders	AGatedCycle_OmitsThePlanApplyEntirely...	DoesNotContain — sub-string found
shipped default 4 → 1	TheShippedDefault_IsFourCycles	expected 4, actual 1

Two method notes, because one of them nearly produced a false result: the first mutation attempt silently failed to apply (a CRLF anchor mismatch) and the suite reported green — a failed mutation reads exactly like a passing test, so each mutation now asserts its anchor count as a positive control before rewriting. And touch + rebuild is not a staleness control: .NET builds deterministically, so the MVID was unchanged. The real control mutates content — MVID aa774a98… → ee078864… with MaxProcedureStatsPlanCycleInterval reading 61, then both returning on restore.

Reflected off the built assembly: PlanCadenceGatedCollector = procedure_stats, clamps [1,60] with clamp(0) = clamp(int.MinValue) = 1, DarlingConfig default 4, fleet-of-42 distribution [10, 10, 11, 11].

Measured, and what is only modelled

Fresh from get_collector_cost, last 24 h on us-east-1: procedure_stats 129.9M ms over 32,093 runs, avg 4,048 ms, 42 servers — the top collector, 2.4x the next. One correction to the record while I am here: it is not true that both fleets sit at the 150 cap. us-east-1's mean is 140.9 rows/run; us-east-2's is 71.2. #2847's original wording had this right ("pegged at 150 every run … use2 p50 85"); the shorter "both report 150 because both are at the cap" framing does not survive measurement, and it matters because the cap being saturated is what makes a skipped cycle free on us-east-1 specifically. Per-day averages against us-east-2 for the same collector: 5,389 vs 567 ms (9.5x) and 3,706 vs 424 ms (8.7x). On a sampled sweep it was 66% of the whole 1-minute body (4,735 ms of 7,221 ms), and summing the fleet's per-sweep cost gives a body of 9,578 ms against #2849's independently measured 9.77 s.

The resulting cadence cannot be measured before deploy, and the numbers below are the model, not a measurement. Against #2849's cadence ≈ N x R / C + tick/2, calibrated so the current point reproduces the observed 113.1 s:

	modelled cadence (C=4)
today	~113 s
N=4 (this change)	~82 s
N=8	~77 s
N=∞ (never render)	~72 s

This does not clear 60 s at max_concurrent_sweeps = 4, and #2849's "~58 s" for a halved R is not reachable. That figure assumed procedure_stats was ~50% of the gate-held body; measured today it is 44.7% (4,281 of 9,578 ms per sweep) with a 364 ms floor it cannot go below, so the achievable reduction is ~41% of R, not 50%. The floor at C=4 is ~72 s.

What the change does deliver is ~30 s of cadence at no CPU cost, and that is derivable rather than hoped for: collector work per unit time is N x R / cadence, which goes 3.74 → 3.63 concurrent-body-equivalents — flat to marginally down, because each collection is cheaper by as much as the rate rises. Raising C has no such property; it buys rate at proportional CPU.

Worth re-measuring against collection_log after the roll rather than trusting any of this.

Review response

The review bot raised exactly two things at c33be22e, both acted on rather than dismissed:

1. Stacked <summary> blocks on DarlingCollectorRunner.cs:393 — real, and it also turned build and Darling PostgreSQL tests red on DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks. Fixed in 7d94c10f the way that test's own failure message directs: not by deleting the first summary, which belongs to ShouldCapturePlanForCollector, but by moving the new member above it so the displaced block is reunited with the method it describes. Re-verified by reimplementing the rule (openings per doc run, attributes not ending a run) and scanning all 2,080 non-bin/obj .cs files: 0 offenders, with a synthetic stacked pair as a positive control and a summary + para block as a negative control.
2. Undocumented knob — correct, and I had only done half of it. darling.sample.json landed in 7d94c10f and the Darling/README.md config-reference row in 87de9ce8, matching what collectSchemaChangeEvents carries. The sample was validated with the loader's own JsonCommentHandling.Skip + AllowTrailingCommas, with the unpatched origin/dev copy as a control. Darling/compose/darling.sample.json deliberately gets nothing — it is a 36-line minimal file that carries none of these knobs.

a50276f6 adds the [#2849] CHANGELOG link reference, which my entry cited without defining. Noted in passing and not fixed: [#2860], [#828] and [#887] each have duplicate reference lines, and they predate this branch — left for #2889, which is rewriting that block.

[claude]

Two stacked <summary> blocks are attached to ShouldCapturePlanXmlFor here, and only the second one actually describes it. The first block ("advances this (server, collector) cycle counter and asks the pure policy") describes what ShouldCapturePlanForCollector does below — ShouldCapturePlanXmlFor itself never touches _planCadenceCycles, it just ANDs the SKU flag with the private helper. Looks like a doc comment meant for ShouldCapturePlanForCollector (which currently has none) got left dangling above the correct one instead of being moved down. Worth moving the first <summary> onto ShouldCapturePlanForCollector at line 397.

[claude]

This follows collectSchemaChangeEvents as a file-only knob per the PR description, but that precedent also got a darling.sample.json entry (with an explanatory comment, currently lines 145-152) and a Darling/README.md config-reference row (### collectSchemaChangeEvents, README.md:420-424). procedureStatsPlanCycleInterval has neither — an operator reading the sample config or the README today has no way to discover this knob exists. Worth adding both to match the established pattern for file-only knobs.

[claude]

Reviewed the plan-cadence-gating change (procedure_stats XML render on 1-in-N cycles). The core logic checks out:

• ShouldCapturePlanThisCycle's server-id-derived phase is correct and does spread the fleet instead of bunching it (verified the modulo-of-unsigned-cast handles negative server_id without throwing, matching the test's expectation).
• interval <= 1 correctly short-circuits to always-capture in both the pure policy and the instance gate, keeping the knob byte-identical-reversible as claimed.
• The SKU flag (_capturePlans()) still ANDs with the cadence gate, so capturePlans: false continues to suppress everything.
• Confirmed ProcedureStatsCollector.Instance.Name == "procedure_stats" matches the PlanCadenceGatedCollector constant exactly, and that query_stats/query_store/deadlocks/blocked_process_report are indeed the only other CapturePlanXml consumers, so "only this one collector is gated" is accurate.
• No schema rung needed, and no Lite parity gap: grepped Lite and confirmed it never sets CapturePlanXml at all, so this collector-cadence concept has no Lite counterpart to keep in sync (consistent with capturePlans itself being Darling-only per README).

Left two inline notes:

1. A doc-comment mix-up in DarlingCollectorRunner.cs — two stacked <summary> blocks land on one method, and the first describes the other method.
2. The new procedureStatsPlanCycleInterval knob skips the darling.sample.json + README.md documentation that its own stated precedent (collectSchemaChangeEvents) got — both are one-time file-only knobs and got treated identically everywhere else, so this looks like an oversight rather than a deliberate omission.

No correctness, security, or Lite/Darling parity issues beyond those two.

[claude]

Reviewed the diff (CHANGELOG, ProcedureStatsPlanCadenceTests.cs, DarlingCollectorRunner.cs, DarlingConfig.cs, DarlingWorker.cs, StoreConfigProvider.cs, darling.sample.json, Darling/README.md) against CONTRIBUTING.md's conventions, correctness, Lite/Darling parity, security, and performance.

Correctness

• ShouldCapturePlanThisCycle's phase math ((uint)serverId % (uint)interval, then (cycleOrdinal + phase) % interval == 0) is safe against negative serverId and against interval <= 1 (both short-circuit to "always capture" in two places — the pure function and its caller). Verified against the fleet-of-42 staggering tests by hand; the [10,10,11,11] vs. bare-modulo [42,0,0,0] claim checks out.
• AddOrUpdate((serverId, collectorName), 0L, (_, previous) => previous + 1) correctly yields ordinal 0 on first touch and increments thereafter — matches the "first cycle is ordinal 0" doc comment.
• The gate is wired through the single named seam (ShouldCapturePlanXmlFor) rather than an inline &&, and only the storing sweep path uses it — FetchRowsAsync (the on-demand fetch) still reads _capturePlans() directly and ungated, confirmed at DarlingCollectorRunner.cs:2115.
• Minor, non-blocking observation: because ShouldCapturePlanXmlFor is _capturePlans() && ShouldCapturePlanForCollector(...), the per-(server, collector) cycle counter does not advance on sweeps where the SKU flag is off. That's consistent with "capture 1 in N enabled cycles" rather than "1 in N calendar cycles," and doesn't break the staggering guarantee, but worth confirming it's the intended semantics rather than an oversight.
• _planCadenceCycles is an unbounded ConcurrentDictionary with no eviction for decommissioned servers, but that matches the existing pattern of every other per-server dictionary in this file (_azureMasterInaccessibleSince, _consecutiveQueryStoreItemFailures, _observedPlanSize, etc.), so not a new problem introduced by this PR.

Lite/Darling parity

• No Lite files are touched, and that's correct here: Lite never sets CollectorContext.CapturePlanXml (grep confirms zero references in Lite/), so the flag defaults to false there regardless of this change. procedureStatsPlanCycleInterval is a file-only knob on DarlingConfig, not surfaced through StoreConfigView/ApplyToConfig (grep confirms no other references), consistent with the PR's claim that this needs no schema rung and can't be clobbered by a control-plane reload.

Security

• No new user input path; the knob comes from darling.json (operator-controlled config file) and is clamped [1,60] before use. No SQL text changes in this PR — the gating only toggles the pre-existing CapturePlanXml flag that ProcedureStatsCollector.BuildQuery (untouched by this diff) already branches on.

Performance

• The change is the performance fix itself (amortizing the sys.dm_exec_text_query_plan render); the gated path fully omits the OUTER APPLY rather than rendering-and-discarding, which is pinned by AGatedCycle_OmitsThePlanApplyEntirely....

Style

• C# XML-doc comments and constant/clamp patterns (Min/MaxProcedureStatsPlanCycleInterval) mirror the existing ClampConcurrentSweeps/ClampTextBudgetMb style. darling.sample.json comment block matches the format of collectSchemaChangeEvents immediately above it.

No blocking findings. This is a tightly-scoped, well-tested change (mutation-tested per the PR description) and the parity/no-schema-change claims all check out against the code.

[claude]

Reviewed the diff (all C#, no T-SQL in this PR). Traced the load-bearing claims in the PR description against the actual code rather than taking them at face value:

• Phase/cadence math (ShouldCapturePlanThisCycle): phase = (uint)serverId % (uint)interval, (cycleOrdinal + phase) % interval == 0. Verified by hand for the 42-server fleet at interval=4 → per-cycle counts [10,10,11,11], matching the PR's claimed distribution and the test's fairShare bound. int.MinValue as serverId casts to 2147483648u, which is divisible by 4 → phase 0, no throw, no bias — matches ANegativeServerId_DoesNotThrowAndStaysInPhaseRange.
• ShouldCapturePlanXmlFor short-circuit: _capturePlans() && ShouldCapturePlanForCollector(...) means the per-(server,collector) cycle counter does not advance while the SKU flag is off. Benign — cadence phase is irrelevant when nothing is being captured anyway — but worth knowing if anyone later reasons about the counter as "always advancing."
• Only procedure_stats is gated: confirmed PlanCadenceGatedCollector == ProcedureStatsCollector.Instance.Name == "procedure_stats", and the dictionary is only touched for that name — other collectors' ShouldCapturePlanForCollector returns true before ever allocating a counter entry.
• FetchRowsAsync intentionally ungated: confirmed at DarlingCollectorRunner.cs:2115 — still CapturePlanXml = _capturePlans(), matching the PR's "only the storing path is gated" claim.
• No control-plane clobber: StoreConfigProvider.ApplyToConfig mutates DarlingConfig field-by-field and never references ProcedureStatsPlanCycleInterval (nor CollectSchemaChangeEvents, its stated precedent) — confirmed neither property exists on StoreConfigView. A store reload genuinely can't reset the file value.
• Reversibility: interval <= 1 short-circuits to true in both the pure policy and the instance gate, and the clamp floors at 1 (not the default) for nonsense/negative values — matches the "never silently starts skipping plans" intent.
• Constructor backward-compat: procedureStatsPlanCycleInterval is appended as the last optional parameter (null → () => 1); every other call site (production and the ~10 existing test call sites) is unaffected.

Lite/Darling parity: no drift. CapturePlanXml is a Darling-only field on CollectorContext — Lite never sets it (CollectorContext.CapturePlanXml defaults false, and DuckDbInitializer.cs:1079 already documents Lite as never setting it for exactly this reason: BlockedProcessReport/Deadlocks plan columns are Darling-only too). So a Lite-side counterpart genuinely isn't needed here, consistent with the PR's own reasoning.

Security / correctness boundaries: no new external input handling — the only new input is a JSON int clamped [1,60] before use. No SQL text changes; the gated query is the existing CapturePlanXml=false branch, just reached on a cadence instead of a static flag. No injection surface, no secrets, no new file/network/process use.

Performance: this is the intended improvement; the added ConcurrentDictionary lookup/update is O(1) per cycle per gated server and negligible next to the collector cost it's amortizing.

Didn't find any correctness bugs, parity gaps, or security issues. The test suite (ProcedureStatsPlanCadenceTests.cs) already pins the properties I'd otherwise flag as under-tested (phase distribution, restart safety, SKU-flag interaction, clamp boundaries, reversibility at interval=1).