Wire the plan fetch's adaptive candidate sizing (#2312 Finding 1)

[erikdarlingdata]

What

Finding 1 from today's #2312 diagnosis: QueryStorePlanXmlState.CandidatePlanCount carries a complete adaptive design — learn each database's real average plan size from its own shipped passes, with a catch-up floor for the window where the sample is provably biased small — and the single call site passed observedAvgPlanBytes: null. Every pass on every database sized its decompression window from the 160KB first-contact seed: K ≈ 116, always, despite the 11x fleet spread the design documents.

How

• QueryStorePlanXmlState.Learn (pure, pinned): folds one pass's outcome into a carried PlanSizeEstimate. Empty pass → catch-up clears, average stands (the walk is provably caught up). Pass cut by either bound (window consumed OR byte budget reached — >= because the budget predicate admits the plan that crosses the line) → catch-up sets. Ordinary pass → learns its average, clears catch-up. All-NULL-plans pass keeps the previous average (ObservedAvgPlanBytes yields null).
• The runner carries the estimate per (server, database) in memory — the same pattern and the same rationale as the adaptive-shrink failure counters: a restart forgetting it costs exactly one seed-sized pass.
• Shipped bytes are computed client-side (PlanXml.Length * 2 — DATALENGTH of nvarchar exactly), learned BEFORE the empty-pass early return so the caught-up signal isn't lost.

Effect

Measured with the shipped math: a genuine 15KB-average database re-sizes from K=116 to K=1,259 and walks its plan backlog ~10x faster per pass; during catch-up the floor pins K back at seed size (the biased-sample guard working as documented). Verified locally 7/7 via a net10.0 harness including the downstream CandidatePlanCount effect; QueryStorePlanSizeLearnTests pins the fold's decision table in CI.

Darling-only: Lite runs no separate plan fetch. Finding 2 (activity-driven fetch — the structural fix) is design-gated on the issue.

🤖 Generated with Claude Code

[claude]

fetched.Count includes rows whose PlanXml is null. BuildPlanFetchQuery's own doc (QueryStoreCollector.cs, around line 1166) says NULL query_plan "counts as ZERO bytes and STILL SHIPS, as a row with NULL text" — deliberately, so the watermark can advance past unpersistable plans.

That's fine for the catch-up/window comparison (plansShipped >= candidateWindow legitimately wants the raw row count). But it's also used as the divisor in ObservedAvgPlanBytes(bytesShipped, plansShipped) here. Any pass that ships a mix of real and NULL-XML plans divides real bytes by a count that includes the zero-byte NULL rows, which biases the learned average down.

Per this module's own stated safety rule (FirstContactAvgPlanBytes doc, a few hundred lines up in QueryStorePlanXmlState.cs): "over-estimating plan size yields a SMALL candidate window, and small is the safe direction... one that is too large decompresses plans it will never ship." An average biased low pushes K in the unsafe direction next pass.

Worth at least confirming this is negligible in practice (how common are NULL query_plan rows on the measured fleet?), since forced-plan-failure paths and oversized plans are exactly the cases mentioned as producing NULLs. If it's not negligible, the fix would be to divide by the count of rows that actually carried bytes, while still using the raw fetched.Count for the window/catch-up comparison.

[erikdarlingdata]

Right, and in the worst direction — a NULL-inflated divisor understates the average, which INFLATES the next window, exactly what the estimator is built to err away from. 68abe25 splits the counts: plansShipped (raw, the window/catch-up comparison — NULL rows genuinely shipped and the watermark passes them) vs plansMeasured (non-NULL, the average's divisor). The mixed-pass case is pinned (10 rows, 5 with XML, 500K bytes → 100K average, not 50K), and the all-NULL pass now flows plansMeasured: 0 so the previous average stands via ObservedAvgPlanBytes' existing null.

[claude]

Reviewed. Traced the Learn fold and CandidatePlanCount catch-up floor by hand against all 5 pinned tests plus the actual FetchAndStorePlansAsync call site — the math checks out (e.g. the PR's 15KB→K=1,259 claim: 12,582,912 × 1.5 / 15,000 = 1,258.3 → ceil 1,259). Left one inline note on a possible bias in the learned average.

One doc issue outside the diff hunk, so noting it here instead of inline: the XML doc on FetchAndStorePlansAsync (Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs, roughly lines 1370–1377) still says candidate sizing is "seeded conservatively rather than adapted, DELIBERATELY, and this is the one piece of the ratified design not yet wired... there is nowhere to keep it [the adaptive input]... Passing null means K comes from FirstContactAvgPlanBytes... Slower convergence, never unsafe." That's exactly what this PR fixes — the method now reads _observedPlanSize and passes a real average when one's been learned. Worth updating or deleting that paragraph so it doesn't contradict the code three lines below it; it also references CollectorContext field-sharing-with-Lite as the blocker, but the PR sidestepped that entirely by carrying the estimate in a runner-local ConcurrentDictionary instead, so the old reasoning no longer applies either.

Lite/Darling parity: no drift — confirmed Lite has no plan-XML fetch path (QueryStorePlanXmlState.CandidatePlanCount/Learn aren't referenced anywhere under Lite/), matching the PR description. PerformanceMonitor.Collectors/QueryStorePlanXmlState.cs is shared, but the new members are additive (a new record struct + a new pure static method) and don't touch anything Lite's tests already pin.

No SQL changes in this PR (the query text/parameterization in BuildPlanFetchQuery is untouched), so no new injection surface. Learn is a pure function operating on already-in-memory longs — no new I/O.

[claude]

Reviewed the diff (CHANGELOG.md, QueryStorePlanXmlState.Learn, DarlingCollectorRunner.FetchAndStorePlansAsync, QueryStorePlanSizeLearnTests). No blocking issues found.

Correctness — Learn's decision table checks out against BuildPlanFetchQuery's server-side SQL:

• The plansShipped (raw rows, includes NULL-XML) vs plansMeasured (XML-carrying rows only) split for the averaging divisor is correct and matches the "review catch" comment — averaging over a NULL-inflated count would understate plan size and inflate the next window, which is the unsafe direction.
• Client-side shippedBytes (summing only non-null PlanXml.Length * 2) matches the server-side plan_bytes = COALESCE(DATALENGTH(...), 0) exactly — both treat NULL plans as zero bytes, so the bytesShipped >= budgetBytes catch-up check is measuring the same quantity the SQL used to cut the pass.
• >= for the budget-cut catch-up trigger is right: the server's running_bytes - plan_bytes < budget predicate admits the plan that crosses the line, so the shipped total can land at or past budget, not just under it.
• Learning happens before the empty-pass early return and before the Postgres write — correct per the doc comment (an empty pass is itself proof of catch-up), and a failed write further downstream still leaves a legitimate size measurement in memory even though the watermark correctly stays unmoved.
• Verified the arithmetic in all 6 new QueryStorePlanSizeLearnTests cases by hand — all correct.

Parity — confirmed Lite has no CandidatePlanCount/FetchAndStorePlansAsync/plan-fetch equivalent at all, so this being Darling-only is genuine, not drift.

Security/perf — no new input-trust boundaries, no injection surface touched; the added ConcurrentDictionary lookup and per-row loop are O(rows) and negligible next to the query itself. This is a pure win over the previous always-160KB-seed behavior.

One thing I couldn't do in this sandbox: run dotnet build/dotnet test (blocked on approval), so I verified by careful reading + hand-checked the test math rather than executing the suite.