Query Store payload stages its aggregate — the fixed cost big catalogs couldn't pay

[erikdarlingdata]

Closes #2133 — and this is the actual root cause under the whole catch-up saga: #2102's death spiral, #2111's yield-to-live, and #2125's adaptive shrink were all mitigating a cost none of them touched.

The bug

The payload joined its slice aggregate straight into the query_store_plan/query/text catalog TVFs. The optimizer only has fixed-guess cardinalities for TVFs, and the shape it picked re-materialized a TVF per probe — a fixed ≥30-second cost on an 82k-plan catalog that no catch-up window width could reduce. That's why the fleet's big databases (echo, oak, Surge, spruce, insa…) pinned at the 15-minute shrink floor and never converged while smaller neighbors on the same servers stayed current.

Bisected live on the wedged store (echo, SQL 2022, 3 GB QS catalog)

shape	result
aggregate alone	7,346 groups, 81 ms
sys.query_store_plan bare scan	82k rows, 319 ms
aggregate JOIN plan (current shape)	>30s, hinted or not
staged: aggregate INTO #temp, join FROM it	524 ms (56 stage + 409 join)
the full emitted 55-column batch, plan capture ON, one-hour backlog	21.3 s / 8,111 rows / 55 cols — where the old shape never finished inside the 60s command timeout

The fix

• SELECT ... INTO #pm_qs_slice, then the plan/query/text joins run FROM the temp — real row counts instead of TVF guesses, each TVF scanned exactly once. sp_QuickieStore stages for exactly this reason.
• The LOOP JOIN hint is gone for good and pinned absent: looping from the temp into the TVFs is the same per-probe re-materialization by another name. The 524 ms join is unhinted, chosen from true cardinalities.
• The interval pre-filter resolves ids from the tiny interval catalog (20 ms) instead of scanning runtime_stats itself (426 ms) — same superset bound, the exact HAVING unchanged.
• Batch mechanics preserved: SELECT INTO emits no result set so the batch still returns exactly ONE (the reader/byte-budget contract); TOP … WITH TIES, ship order, and derived-watermark semantics live on the final SELECT unchanged; the leading DROP TABLE IF EXISTS covers Azure's pooled direct connections while the on-prem sp_executesql scope self-cleans. One shared body, so both SKUs and both engine arms (live + backfill) get the fix by construction.

Validation

All timings above are from read-only probes against the wedged field store, run today. Shape pins updated: staging present, catalog pre-filter, no-LOOP-JOIN (both live and backfill arms), HAVING indentation.

After merge: nightly → dogfood box → the eight wedged members should converge and the adaptive shrink demote to a rarely-needed safety net.

🤖 Generated with Claude Code

[claude]

Minor style nit: this column list is inconsistently indented now that it's a top-level SELECT ... INTO instead of a nested derived table. plan_id, runtime_stats_interval_id, and execution_type_desc sit at 4 spaces (correct per the style guide), but every aggregate column below them (first_execution_time = MIN(...) through max_rowcount = MAX(...)) is still indented 8 spaces — a leftover from when this block was one level deeper inside FROM (...). Worth dedenting the aggregate lines to 4 spaces to match the rest of the list and the file's "4 spaces indentation" convention.

[claude]

Reviewed the staged-join fix (QueryStoreCollector.cs + Lite.Tests/QueryStoreCollectorDefinitionTests.cs + CHANGELOG).

Correctness — the core change checks out:

• Superset proofs for the new interval-catalog pre-filter hold. Live: i.end_time > @cutoff_time is a valid superset of MAX(qsrs.last_execution_time) > @cutoff_time since a row's last_execution_time can never exceed its interval's end_time. Backfill: the (i.end_time > @floor_time AND i.start_time < @ceiling_time) overlap test is a valid superset of the exclusive (floor, ceiling) row-level window by the same containment argument. The HAVING clause remains the exact row-level filter in both cases, so shipped semantics are unchanged — this isn't a silent behavior change dressed up as a performance fix.
• Batch mechanics are sound: SELECT ... INTO #pm_qs_slice emits no result set, so the reader still sees exactly one result set from the final SELECT. The temp table's scope inside a single [db].sys.sp_executesql nesting is self-cleaning on-prem; the leading DROP TABLE IF EXISTS is a reasonable defensive measure for Azure's pooled direct-connection path.
• LOOP JOIN hint removal is well justified — forcing a loop from the temp table into the catalog TVFs would reproduce the same per-probe re-materialization the staging fix exists to eliminate.
• The AzureEligibilityGateText + BuildPayloadBody concatenation (mid-batch RETURN; followed by DROP TABLE/SELECT INTO/SELECT) is valid as a single batch.

Parity — no drift. QueryStoreCollector lives in the shared PerformanceMonitor.Collectors project referenced by both Lite and Darling, so both apps and both engine arms (live + backfill) pick up the fix from one edit, as the PR description claims. Darling.Tests/QueryStoreBackfillTests.cs only pins the backfill worker's pure contracts (hole codec, horizon derivation), not this SQL shape, so there's no stale counterpart assertion left behind.

Test coverage — the Lite.Tests updates track the new shape well: staging presence, no-LOOP JOIN (all four call sites: live, backfill, per-item, per-item-with-plan-capture), the catalog pre-filter columns/table, and the unindented HAVING.

One minor style nit left as an inline comment: the SELECT ... INTO #pm_qs_slice column list has inconsistent indentation (4 spaces for the first three columns, 8 spaces for the aggregate columns) — a leftover from de-nesting the old derived table.

No security, missing-index-DMV, or performance-regression concerns — the change is a well-documented, benchmarked improvement.

[claude]

The staging SELECT ... INTO #pm_qs_slice statement has no OPTION(RECOMPILE), so it's now exposed to exactly the parameter-sniffing problem this PR set out to fix.

Before this change, the aggregate (this GROUP BY/HAVING block) lived inside the single query-plan-plan-text join that carried OPTION(RECOMPILE, LOOP JOIN) — so the whole statement, including this aggregate, was recompiled fresh on every execution against the actual @cutoff_time/@floor_time/@ceiling_time values.

Now that the aggregate is its own statement, it has no query hint at all. It's invoked through sp_executesql (on-prem) with the same parameterized text every cycle — only the parameter values change — so SQL Server will cache a plan for this statement the first time it runs and reuse it on later executions. Live cutoff windows and backfill floor/ceiling windows (which per the PR description can span from a live catch-up window up to a 24h AdaptiveSpan slice) have wildly different selectivity against sys.query_store_runtime_stats_interval, so a plan cached from one shape can easily misestimate for the other — the same "fixed-guess cardinality" failure mode called out as the root cause here, just relocated one statement earlier.

Suggest adding OPTION(RECOMPILE) to this SELECT ... INTO statement as well, so both statements in the batch get fresh estimates from the actual parameter values every cycle.

[claude]

Indentation is inconsistent in the staged SELECT ... INTO block, and it looks like leftover formatting from when this list lived one level deeper inside FROM (\n SELECT ...\n) AS qsrs.

• SELECT list: qsrs.plan_id, qsrs.runtime_stats_interval_id, qsrs.execution_type_desc (lines 829-831) are now at 4-space indent, but every column starting with first_execution_time = MIN(...) at line 832 through max_rowcount at line 860 is still at 8-space indent.
• GROUP BY (lines 871-873): the first three items are 4-space indented, but replicaGroupKey (PerformanceMonitor.Collectors/QueryStoreCollector.cs:684, ",\n qsrs.replica_group_id") still hardcodes an 8-space continuation, so the emitted GROUP BY renders with the last item mis-indented relative to its siblings.
• The backfill form of intervalPreFilter (PerformanceMonitor.Collectors/QueryStoreCollector.cs:799, " AND i.start_time < @ceiling_time") is also still indented for the old nesting level; against the new 4-space WHERE {intervalPreFilter} at line 868, the AND continuation no longer column-aligns with WHERE's predicate the way the style guide's WHERE ... / AND ... alignment convention expects.

None of this is a behavioral bug, but the style guide calls for consistent 4-space indentation, and the tests were updated to pin the mixed indentation as-is (e.g. the GROUP BY assertion in Lite.Tests/QueryStoreCollectorDefinitionTests.cs) rather than catching it. Worth flattening the whole staged statement to one consistent indent level, including replicaGroupKey and the backfill intervalPreFilter/intervalHaving continuations, and updating the pinned test strings to match.

[claude]

Review

Reviewed the staged-aggregate rewrite of QueryStoreCollector.BuildPayloadBody (the shared body both Lite's on-prem sp_executesql/Azure-direct paths and Darling run — no parity drift, since it's one collector definition consumed by both apps, and no Darling test depends on the old query shape).

Two inline findings, left on the diff:

1. Missing OPTION(RECOMPILE) on the new staging SELECT ... INTO #pm_qs_slice statement. Previously the aggregate lived inside the single statement carrying OPTION(RECOMPILE, LOOP JOIN), so it always got a fresh plan from the actual @cutoff_time/@floor_time/@ceiling_time values. Split out on its own with no hint, it's now cacheable via sp_executesql's auto-parameterization and can get parameter-sniffed across live vs. backfill windows of very different selectivity — the same fixed-cardinality failure mode this PR is fixing, one statement earlier.
2. Inconsistent indentation in the emitted SQL for the staged block (SELECT list, GROUP BY, and the backfill intervalPreFilter's AND continuation) — leftover from when this text was nested one level deeper inside a derived table. Cosmetic, but violates the project's 4-space consistent-indent style rule, and the test pins were updated to match the mixed indentation rather than catch it.

Otherwise the staging approach itself is sound: SELECT INTO correctly emits no result set (preserves the one-result-set batch contract), the DROP TABLE IF EXISTS correctly covers Azure's pooled-connection reuse while the on-prem sp_executesql nesting scopes the temp table away on its own, and the interval-catalog pre-filter (sys.query_store_runtime_stats_interval instead of scanning runtime_stats itself) is a legitimate superset prune — the exact HAVING semantics are untouched. Test coverage for the new staging shape, the removed LOOP JOIN hint, and the interval-catalog filter looks thorough.

[erikdarlingdata]

Both taken, in 9289614:

1. Staging OPTION(RECOMPILE) — completely right, and a sharp catch: split out on its own, the staging statement would be cached via sp_executesql's parameterized text and sniffed across live vs backfill windows — the exact fixed-guess failure mode this PR removes, reintroduced one statement earlier. Both statements now carry OPTION(RECOMPILE), with the reasoning documented in the template comment.

2. Indentation — the staged block (aggregate lines, the spliced replicaGroupKey/gated-family fragments, and the backfill window continuations) now dedents to the file's 4-space convention, and the pins assert the clean shape instead of encoding the mixed one. Done with per-fragment edits and the diff hunks read before pushing — the earlier attempt at this exact cleanup is the reverted 82dea99, whose block-rewrite anchor matched the wrong marker 700 lines early.

[claude]

Reviewed the diff (CHANGELOG.md, QueryStoreCollector.cs, QueryStoreCollectorDefinitionTests.cs) against correctness, Lite/Darling parity, security, and performance. No blocking issues found. Notes from the pass:

Parity — QueryStoreCollector lives in the shared PerformanceMonitor.Collectors project; both Darling/PerformanceMonitor.Darling.Service/QueryStoreBackfill.cs and Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs call the same definition.BuildBackfillQuery/BuildBackfillPerItemQuery. No duplicated SQL to drift out of sync — both SKUs get the fix by construction, as the PR description claims.

Correctness checks I ran down explicitly:

• Staging→final column contract: the SELECT ... INTO #pm_qs_slice list is byte-for-byte the same 55-ish column set the old derived table produced (just relocated), and every qsrs.* reference in the final SELECT/JOIN/ORDER BY resolves against a column the staging statement actually populates, gated by the same isNew/hasReplicaAttribution flags in both places.
• Interval-catalog pre-filter as a superset: switching from sys.query_store_runtime_stats (f.last_execution_time) to sys.query_store_runtime_stats_interval (i.start_time/i.end_time) only stays safe because a row's last_execution_time is bounded within its own interval's [start_time, end_time]. Under that invariant the new i.end_time > @cutoff/overlap test is provably a superset of the old per-row test, and the unchanged HAVING MAX(qsrs.last_execution_time) ... remains the exact filter, so this can prune but never silently drop rows — confirmed the pre-filter only restricts by runtime_stats_interval_id, which is a GROUP BY key, so it can never truncate a qualifying interval's rows mid-aggregate.
• Batch/reader contract: DROP TABLE IF EXISTS and SELECT ... INTO don't produce a result set in SQL Server's TDS protocol, so the existing single-result-set reader path (ReadRowsAsync/ReadAsync) doesn't need a NextResult() and wasn't touched — consistent with the "still one result set" claim.
• On-prem temp table scope: #pm_qs_slice is created inside the nested [db].sys.sp_executesql call, so it's torn down automatically when that call returns regardless of SqlConnection reuse across retries in the backfill worker loop; the leading DROP TABLE IF EXISTS is there for the Azure per-database direct-connection path where pooled reuse is a real (if sp_reset_connection-mitigated) concern. Reasonable belt-and-suspenders, matches the IndexObjectStatsCollector staging precedent already in this codebase.
• OPTION(RECOMPILE) on both statements: verified this matters — the staging statement runs inside the same parameterized sp_executesql text, so without its own RECOMPILE it would get a cached plan sniffed on @cutoff_time (live, narrow) vs @floor_time/@ceiling_time (backfill, wide) — exactly the fixed-guess pathology this PR removes, one statement earlier. Good catch already folded in.
• No stray f./old-alias references left anywhere in the file after the rewrite.
• Self-exclusion (SelfQueryMarker check in ReadRowsAsync) still works correctly now that there are two marker-carrying statements per cycle instead of one — it matches on query_sql_text of joined result rows, not on which statement produced them, so the extra self-referential Query Store entry the staging statement now generates is filtered the same way the old monolithic query's entry always was.

Style — indentation dedent (8→4 spaces) brings the touched blocks in line with the project's 4-space T-SQL convention; comments stay /* ... */ block style throughout.

Didn't flag any missing-index DMV suggestions per the review brief. Nothing else stood out as a correctness, security, or performance regression.