query_store: force HASH JOIN on the plan and text fetch statements

[erikdarlingdata]

Fixes #2791

The defect

sys.query_store_plan's view definition unions the on-disk table with the in-memory TVF QUERY_STORE_PLAN_IN_MEM. SQL Server has no statistics for that TVF and falls back to a fixed guess — on OMEGA, 1,000 estimated against 14,633 actual, 1,463% off. That guess is what makes Nested Loops look cheap, so the TVF lands on the inner side and is re-executed once per candidate plan_id, up to the 512 MaxCandidatePlans cap, each execution scanning the whole in-memory Query Store before a single plan is decompressed.

Constant Scan (the IN list)
  -> Nested Loops (Inner Join) -> Clustered Index Seek [plan_persist_plan]
    -> Compute Scalar
      -> Nested Loops (LEFT OUTER JOIN)
           inner side: Filter <- Table Valued Function [QUERY_STORE_PLAN_IN_MEM]

The estimate shows the TVF at 1–3% of cost, which is why this plan looks innocent and why it survived this long. Same class as #2676, one layer deeper.

sp_QuickieStore against OMEGA ordered by average CPU puts these statements at the top of the entire instance, 55,000–61,000 ms CPU each. For contrast, the TOP (50000) runtime-stats payload this investigation spent weeks suspecting averages ~2 s and was never the cost.

The fix

The joins live inside the view definition and cannot be hinted individually, so the lever is the query-level hint. Measured on OMEGA with actual plans:

	before	with OPTION(RECOMPILE, HASH JOIN)
SELECT ... INTO #plan_fetch FROM sys.query_store_plan	~60,000 ms CPU	0.508 s

The hinted plan is Hash Match (Right Outer Join) at 0.508 s fed by the TVF at 0.427 s, executed once.

The trade-off, measured rather than assumed. A query-level hint applies to every join in the statement, including the Clustered Index Seek into plan_persist_plan at 91% of estimated cost. Forcing hash there risks turning that seek into a full scan — and it does: under the hint that access becomes a Clustered Index Scan. It is a non-issue, measured rather than argued:

operator	actual time
Clustered Index Scan [plan_persist_plan]	0.032 s
Table Valued Function [QUERY_STORE_PLAN_IN_MEM]	0.373 s
Hash Match (Right Outer Join)	0.436 s
statement total	0.508 s

Worth stating in that direction rather than as "the risk didn't happen": the estimate puts the seek at 91% of cost and the actual plan puts the scan at 6% of half a second. The operator the estimate points at is not the operator that matters — the same lesson as the TVF showing at 1–3% of estimated cost while carrying the real work.

Why the query is not restructured

Erik also measured a rewrite that materializes candidates into a #temp with a unique clustered index on plan_id, so the running-total window streams in index order and its Sort disappears — on OMEGA that Sort was 5.262 s against 0.436 s for the Hash Match feeding it, and the rewrite took that variant from ~6.5 s to ~2.5 s.

That rewrite is real and the numbers are his, but it is not shipped here, for a reason that has nothing to do with whether it works: it restructures a shape we do not ship. The WITH candidates CTE it improves exists only as an ad-hoc experiment against OMEGA (see the repo-wide search in the comments below — every shipping path was checked). The form we actually ship already stages into #plan_fetch, and with the hint it runs in 0.508 s, which is comfortably better than the 2.5 s the rewrite reaches. So there is nothing to port over.

The text fetch joins sys.query_store_query to sys.query_store_query_text — both TVF-backed unions, driven by the same kind of IN list. Same defect, same fix. The OMEGA measurement is the plan fetch's and is not claimed for the text fetch; what is verified for it here is the join-strategy change and the identical-rowset property.

The second statement of each builder reads only the #temp and joins nothing, so it keeps plain OPTION(RECOMPILE). Checked, not assumed — and pinned by a test, because a query-level hint on a joinless statement is exactly the kind of thing that gets copied forward and later defended as load-bearing.

Correctness — the rowset is unchanged

The hint must change the join strategy and nothing else. Verified against a live SQL Server (container, QSTEST, 40,882-plan Query Store) driving the shipped builders, with the baseline derived from the shipped text by removing only , HASH JOIN:

      [info] ARITHABORT = 0  (the collector's setting, not SSMS's)

plan[16]:  IDENTICAL rowset  rows=16/16   sha=03E3E1AAF4943D72/03E3E1AAF4943D72
plan[128]: IDENTICAL rowset  rows=128/128 sha=ADF9E94D1AAA7F99/ADF9E94D1AAA7F99
plan[512]: IDENTICAL rowset  rows=184/184 sha=97F0ABD384B5AB43/97F0ABD384B5AB43   <- budget cut, identical both ways
text[16]:  IDENTICAL rowset  rows=16/16   sha=2E33BFB0A0A6A669/2E33BFB0A0A6A669
text[128]: IDENTICAL rowset  rows=128/128 sha=842F5AA804583C30/842F5AA804583C30
text[512]: IDENTICAL rowset  rows=512/512 sha=FC11989E1B41D166/FC11989E1B41D166

SHA256 over every column of every row in order, not a row count. Note plan[512] returning 184 rows — the 12 MB budget cut fires there and cuts identically both ways.

Mechanism confirmed from actual plans on the same instance:

without hint : NestedLoops=True   HashMatch=True
with hint    : NestedLoops=False  HashMatch=True

The container cannot reproduce OMEGA's 512× TVF re-execution (its in-memory Query Store is small — TVF_ActualExecutions=1 both ways), which is why the performance verdict above comes from OMEGA and not from here. What the container establishes is correctness and the join-strategy change.

The obvious next idea, and why it does not work

Evaluating the byte budget against a compressed length, so plans the budget discards are never decompressed, is not available through this view. Over 512 candidate ids:

	ms
SELECT plan_id alone	14
DATALENGTH(qsp.query_plan)	321
full CONVERT(nvarchar(max), qsp.query_plan)	331

DATALENGTH costs what full decompression costs, because the view decompresses on any access to the column. The compressed blob lives in the undocumented sys.plan_persist_plan, which is not a surface to ship against. So the budget bounds what is shipped, not what is decompressed — a property of the catalog, not a shortcoming of this query. Recorded in the doc comment so it is not re-proposed.

Not deployed

No deploy — there is an active measurement window on use1.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

Nit: this entry describes a performance/correctness fix (wrong join strategy causing 55-61s CPU per fetch), not new functionality, but it's filed under ### Added. Every other entry of this exact shape in the current Unreleased section — the very similar Query Store catalog-view join fixes at #2764, #2766, and #2759 a few lines below — lives under ### Fixed. Suggest moving this bullet there for consistency with the project's own precedent.

[erikdarlingdata]

Good catch, fixed in c8ffaaa — moved to ### Fixed. You're right on the precedent: #2764, #2766 and #2759 are the same class of Query Store catalog-view join fix and all sit under Fixed.

Cause was my own insertion logic: it looked for ## [Unreleased] followed immediately by ### Fixed, which never matched because ### Added comes first in that section, so it silently fell through to the ### Added anchor instead of failing. The corrected version asserts the ### Fixed it finds is inside the Unreleased span before inserting.

[claude]

Review summary

Reviewed the diff (CHANGELOG.md, PerformanceMonitor.Collectors/QueryStoreCollector.cs, Darling/Darling.Tests/QueryStorePlanFetchTests.cs) against CONTRIBUTING.md's T-SQL style section, Lite/Darling parity, correctness, security, and performance.

Parity: No drift. BuildPlanFetchByIdsQuery / BuildTextFetchByIdsQuery live in PerformanceMonitor.Collectors, which both Darling and Lite/Lite.Tests reference — the fix lands in the one shared code path both apps call. Confirmed no other caller of these two builders exists outside DarlingCollectorRunner.cs, and confirmed Lite.Tests' existing OPTION(RECOMPILE) substring assertions still pass against the new two-statement text (they match the second, still-plain-RECOMPILE statement) — no test breakage there.

T-SQL style: Compliant. OPTION(RECOMPILE, HASH JOIN); keeps the required OPTION(RECOMPILE) on both new statements (one hinted, one plain), matches the no-space-after-OPTION convention used throughout this file, and there's in-repo precedent for combined query-level hints (OPTION(RECOMPILE, HASH GROUP) in the deprecated dashboard code). The new doc comments follow the "measurements + issue numbers" convention this codebase already uses heavily.

Correctness: The change is narrowly scoped — only the two by-ids fetch statements that actually join gain the hint; the sibling statements that read only #plan_fetch/#text_fetch correctly keep plain RECOMPILE, and a new test (TheBudgetStatementsKeepPlainRecompile_BecauseTheyJoinNothing) pins exactly one hinted / one plain statement per builder. Since the query-level hint has to apply through the inlined catalog-view joins (they can't be hinted individually per the PR description), this is the only lever available, and the PR documents that the risk case it identified (forcing hash onto the plan_persist_plan seek) was checked and measured, not assumed.

Nit posted inline: the CHANGELOG entry is filed under ### Added but reads as a bug/perf fix, inconsistent with several near-identical Query Store join-fix entries (#2764, #2766, #2759) that are filed under ### Fixed a few lines down in the same Unreleased section.

No security concerns (id lists are host-computed longs, not string-interpolated user input; escapedDbName bracket-escapes as elsewhere in the file) and no missing-index-DMV suggestions made.

[erikdarlingdata]

Confirmed: nothing we ship emits the WITH candidates CTE

Erik's sp_QuickieStore output from OMEGA shows two WITH candidates AS (...) entries at 78,681 ms and 85,691 ms alongside the shipped SELECT plan_id = qsp.plan_id ... entries at ~60,000 ms. Searched the whole repo, not just QueryStoreCollector.cs:

1. Literal WITH candidates — two hits, both PostgreSQL, neither touches SQL Server Query Store:

file	what it is
Darling/PerformanceMonitor.Darling.Storage/DarlingPgBlockingReader.cs:314	PostgreSQL blocking read
PerformanceMonitor.Collectors/PgIndexBloatCollector.cs:108	PostgreSQL index bloat collector

2. Every file mentioning query_store_plan that also contains a CTE — two candidates, both cleared:

• Lite/Services/LocalDataService.QueryStore.cs:849 — FetchQueryStorePlanAsync, a single-plan on-demand viewer fetch: SELECT TOP (1) query_plan = CONVERT(nvarchar(max), qsp.query_plan) FROM sys.query_store_plan AS qsp WHERE qsp.plan_id = @plan_id. One parameterised equality predicate, no CTE, no IN list. There is no multi-row outer input for a loop to re-execute a TVF against, so the query_store plan fetch: TVF re-executed per candidate plan_id (55-61s CPU); force HASH JOIN #2791 defect cannot arise. CommandTimeout = 30, fired when a user clicks a plan.
• Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanMap.cs — the Postgres store table collect.query_store_plan_map. It never queries SQL Server's catalog; the only occurrence of the name is a doc comment explaining that query_plan_hash reads without decompressing.

3. Every emitter of CONVERT(nvarchar(max), qsp.query_plan): the two in QueryStoreCollector.cs (line 1293, the statement this PR hints; line 694, a doc comment), the Lite single-plan fetch above, and three assertions in QueryStorePlanFetchTests.cs.

Conclusion: no shipping code path emits a CTE over sys.query_store_plan. Those two OMEGA entries are ad-hoc experiments run against that database and captured by its own Query Store — which is exactly what Query Store is for, and harmless. Nothing is uncovered by this PR.

The plan_correction observation is filed separately as #2793, unmeasured and explicitly not folded in here.

[erikdarlingdata]

Two process notes worth keeping, both the same failure shape

pending == 0 is not "green"

While waiting on this PR I nearly merged it on a single check. Right after a push, gh pr checks returned exactly one row — check-branches, passing, 4 seconds — because the other five workflows had not registered yet. My waiter tested pending == 0, which was true at that moment, so it would have reported "settled, all green" and merged.

The bug is that pending == 0 is true in two states: after everything passes, and before anything starts. A waiter keyed on it reports success at exactly the moment it has the least information.

It is also indistinguishable at a glance from the conflicted-PR false green, where a CONFLICTING PR dispatches only check-branches and never runs build, verify, review or the PostgreSQL tests. The only thing separating the two here was mergeable = MERGEABLE rather than CONFLICTING.

The pin is "six present and green", never "nothing pending". Corrected waiter:

N=$(echo "$OUT" | grep -cE "pass|fail|pending")   # checks that exist
P=$(echo "$OUT" | grep -c pending)
if [ "$N" -ge 6 ] && [ "$P" = "0" ]; then ... fi  # both, not either

The CHANGELOG anchor that fell through instead of failing

claude[bot] caught this entry filed under ### Added when every sibling of its shape (#2764, #2766, #2759) is under ### Fixed. The cause was mine: my insertion script anchored on ## [Unreleased] followed immediately by ### Fixed, which never matches because ### Added comes first in that section — so it silently fell back to the ### Added anchor and wrote to the wrong place. The corrected version asserts the ### Fixed it finds is inside the Unreleased span and fails otherwise.

Same shape as the waiter, and as a silently-ignored SET option: a check that quietly succeeds when it should have refused to answer. In all three the wrong answer is indistinguishable from the right one at the call site.

The reusable lesson from the investigation itself

On Query Store's TVF-backed views the estimated cost percentages point at the wrong operator, every time:

operator	estimated	actual
Clustered Index Seek/Scan [plan_persist_plan]	91% of cost	0.032 s — 6% of the statement
Table Valued Function [QUERY_STORE_PLAN_IN_MEM]	1–3% of cost	0.373 s — carries the real work, 1,463% cardinality miss

Read actuals on these views, or do not read the plan at all. Reasoning from the estimate is what kept this defect hidden while the TOP (50000) payload — averaging ~2 s — absorbed weeks of suspicion.

[claude]

Nit / worth a sentence in the comment above rather than a blocker: OPTION(RECOMPILE, HASH JOIN) forces a Hash Match, which (unlike the Nested Loops it replaces) requires a workspace memory grant. That's clearly the right trade on the OMEGA case this PR fixes (60s CPU → 0.5s), but this collector can run once per database per cycle across a fleet of concurrently-collected databases. On an instance already under memory pressure, an unconditional hash-join grant on every invocation — even ones with only a handful of candidate ids where Nested Loops would've been cheap and grant-free — is a small but real behavior change versus letting the optimizer choose per-statement. Given how carefully everything else here is measured, it'd be worth either a one-line note confirming this was considered (e.g. observed grant size on OMEGA), or confirming it's a non-issue given typical candidate-set sizes. Same applies to the text fetch at line 1417.

[erikdarlingdata]

Fair, and you are right on the mechanism — I had not measured it. Now I have, and the answer is "real but bounded". Recorded in the comment on both builders in 3ea41f5.

Grants on a 40,882-plan Query Store, no spill in any case:

candidate ids	without hint	with hint
4	0 KB — pure Nested Loops, grant-free	1,264 KB
512	1,760 KB — optimizer already picked a hash	3,624 KB

Three things fall out of that:

1. Your small-set case is exactly right. At 4 ids the unhinted plan is grant-free and the hint introduces a grant where none existed. That is a genuine behavior change, not a rounding error in the argument.
2. The magnitude is ~1.2 MB. And at the top end MaxCandidatePlans caps the input at 512, which caps the grant at ~3.6 MB — of which the optimizer was already paying 1,760 KB on its own, because at that size it picks a hash anyway. So at the large end the hint roughly doubles an existing grant rather than introducing one.
3. The cap is what makes this bounded. The grant is a function of the candidate list, not of Query Store size, so it does not grow with the 85%-full catalogs that motivated the fix.

On fleet concurrency: sweeps are concurrent across servers but the per-item loop is sequential within a server on one connection, so the worst case is a few concurrent sweeps' worth — single-digit MB, against a statement that was burning 55,000–61,000 ms of CPU per invocation.

I also named the symptom in the comment for the case where this is wrong somewhere I have not measured: RESOURCE_SEMAPHORE waits or a hash spill on the monitored instance. Neither appears in anything measured here.

Applied to the text fetch as well, as you asked.

[claude]

Reviewed the diff (CHANGELOG.md, PerformanceMonitor.Collectors/QueryStoreCollector.cs, Darling/Darling.Tests/QueryStorePlanFetchTests.cs).

Style / conventions: clean. Both fetch statements get their own OPTION(RECOMPILE, HASH JOIN) per CONTRIBUTING's "a statement added to an existing batch needs its own hint" rule, the joinless second statement of each builder correctly keeps plain OPTION(RECOMPILE), and the WHY-heavy comment style matches the rest of the file.

Lite/Darling parity: no drift. QueryStoreCollector lives in the shared PerformanceMonitor.Collectors project referenced by both Lite/PerformanceMonitorLite.csproj and the Darling projects, so the SQL fix is automatically shared — there's no separate Lite copy to fall out of sync. The new tests only landing in Darling/Darling.Tests/QueryStorePlanFetchTests.cs (not mirrored in Lite.Tests) matches existing precedent: Lite.Tests/QueryStoreCollectorDefinitionTests.cs already only covers BuildTextFetchByIdsQuery, not BuildPlanFetchByIdsQuery, so this PR isn't introducing a new asymmetry. Confirmed the existing Lite.Tests assertion Assert.Contains("OPTION(RECOMPILE)", sql, ...) still passes since it matches the second (unhinted-join) statement's OPTION(RECOMPILE); substring.

Correctness: the test suite (PlanFetch_ForcesHashJoin_..., TextFetch_ForcesHashJoin_..., TheBudgetStatementsKeepPlainRecompile_...) correctly pins exactly one hinted statement and one plain-RECOMPILE statement per builder. idList/budget are formatted from long/culture-invariant conversions only, no string interpolation of external input — no injection surface here or in the pre-existing surrounding code.

One thing worth a sentence in the PR/comment, not a blocker: left as an inline comment on QueryStoreCollector.cs:1299 — HASH JOIN forces a memory-grant-bearing Hash Match on every invocation regardless of candidate-set size, where Nested Loops was grant-free. Given this collector runs per-database per-cycle across a fleet, it'd strengthen an already well-measured PR to note whether that was considered (e.g. observed grant size) for the common case of small candidate sets, alongside the OMEGA/512-candidate numbers already given.

No missing-index DMV suggestions, no security issues, no performance regressions beyond the note above.

[claude]

Reviewed the diff, the two edited builder methods, their new tests, and the surrounding doc comments.

No correctness, security, or parity issues found.

Checks performed:

• OPTION(RECOMPILE, HASH JOIN) is valid T-SQL syntax and is applied only to the statement that actually joins (sys.query_store_plan / the query_store_query ⋈ query_store_query_text pair); the second statement in each builder reads only from the #temp table and correctly keeps plain OPTION(RECOMPILE) — matches the CONTRIBUTING.md rule that a hint on one statement doesn't cover a neighbour.
• BuildPlanFetchByIdsQuery/BuildTextFetchByIdsQuery are Darling-only (only referenced from DarlingCollectorRunner.cs); Lite never calls them, so the absence of an equivalent Lite.Tests fact for the new hint isn't a parity gap — it's shared infra exercised by different test suites for different reasons.
• TheBudgetStatementsKeepPlainRecompile_BecauseTheyJoinNothing correctly distinguishes the hinted vs. plain statement by matching the literal "OPTION(RECOMPILE);" (with the trailing );), which does not accidentally match "OPTION(RECOMPILE, HASH JOIN);" — verified against the existing Lite.Tests assertion at QueryStoreCollectorDefinitionTests.cs:622 too (Assert.Contains("OPTION(RECOMPILE)", ...) still passes since it matches the second, unhinted statement).
• Checked the other TVF-backed join in this file (the #pm_qs_slice → sys.query_store_plan join in the runtime-stats query, ~line 1050) that wasn't touched here — its neighbouring comment block (Adaptive catch-up shrink: a member that can't fit the window in the timeout halves it until it can #2125) already explains why a join hint was deliberately rejected there (the temp table gives the optimizer real cardinalities, so forcing a strategy would reintroduce the old regression). Not a gap, just adjacent context worth confirming.
• Style matches CONTRIBUTING.md: AS on table aliases, column_name = expression, OPTION(RECOMPILE) present, no missing-index-DMV suggestions needed.

Nice level of measurement backing the trade-off (seek→scan, memory grant deltas bounded by MaxCandidatePlans).

[erikdarlingdata]

Post-merge note, because this PR undersold its own blast radius.

Production phase timings that arrived after merge show plan_correction spending 96–97% of its cost in plan_fetch:

[omega-01] plan_correction [OMEGA] => 93 rows
  sql:90473ms = wm:0ms + open:2472ms + drain:1127ms + plan_fetch:86874ms + text_fetch:0ms
[epsilon-01] plan_correction [Epsilon] => 74 rows
  sql:61753ms = wm:0ms + open:1852ms + drain:11ms   + plan_fetch:59890ms + text_fetch:0ms

That plan_fetch phase runs this exact statement. Traced rather than assumed:

• PlanCorrectionCollector.cs has no plan-XML fetch of its own.
• PerItemPlanFetchMs (the field printing plan_fetch:) is assigned in one place, DarlingCollectorRunner.cs:1035.
• It sits under the gate at DarlingCollectorRunner.cs:1027, context.CapturePlanXml && targetConnection is SqlConnection — generic, no collector-name check, and CapturePlanXml is a global setting.
• That branch calls FetchAndStorePlansAsync, whose only query is QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery(...) — the sole non-test caller of the builder hinted here.

So this PR fixes the first and second largest measured collector costs on the use1 fleet, not query_store alone. They were the same statement reached through two collectors, which is why plan_correction read as an independent ~3 s problem (an average dominated by idle runs with no tuning recommendations) while query_store read as a 33–42 s one.

#2793 closed with that evidence. Its original hypothesis — hinting the three staged SELECT ... INTO #pm_plan_correction_* catalog reads — is disproved by the same numbers: those live in open:, 1,664–2,472 ms, about 2% of the run.