query_store: attribute replica role on Azure SQL DB, on live GP + Hyperscale evidence

[erikdarlingdata]

Flips Query Store replica attribution ON for Azure SQL Database, on the live evidence the gate's own comment asked for, and corrects the stale reader-ordinal count in the same file.

The decision

QueryStoreCollector.BuildPayloadBody's gate becomes:

bool hasReplicaAttribution = productVersion >= 16 || context.Target.IsAzureSqlDb;

(was productVersion >= 16 && !context.Target.IsAzureSqlDb)

Azure reports ProductMajorVersion 12 but carries the 2022+ catalog, so the version probe cannot speak for it. This is the same "Azure means newest" rule hasPlanType on the line above already uses.

Why now: the probe the old comment demanded was run, on both tiers

The #1836 carve-out was about bind SAFETY, not taste: a missing column is not a NULL, it fails the whole SELECT for that database, and Azure collects per database, so a wrong guess would have meant every database going dark rather than one column. The docs still do not settle it. Live runs do, on both tiers, 2026-07-31 UTC, each on Microsoft SQL Azure (RTM) - 12.0.2000.8, EngineEdition 5:

	GP_S_Gen5_1 (#1848)	HS_S_Gen5_2 (#1872)
OBJECT_ID('sys.query_store_replicas')	-660	-660
COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id')	8	8
view contents	4 role rows, Primary..Geo HA Secondary	identical
collector's LEFT JOIN	every row -> Primary	every row -> Primary
full payload with attribution ON	not run	55/55 columns bound, exit 0
replica_role returned	n/a	Primary

The Hyperscale run is the decisive one: it composed BuildPayloadBody from origin/dev with hasReplicaAttribution = true and the Darling shape (plan XML on), diffed the text against the already-verified 55-column Azure file to confirm the only differences were the three intended ones, and executed it.

A per-tier gate was considered and rejected as unbuildable: Hyperscale reports EngineEdition 5, the same as General Purpose, so the collector cannot tell the tiers apart at query-build time. Since both bind identically, it does not need to.

Managed Instance deliberately keeps the pure version gate — #1844's reasoning preserved. MI under-reports PRODUCTVERSION too, but its feature set follows a per-instance update policy rather than being evergreen, so "Azure means 2022+" is not sound there, and MI has no per-database path this could have been verified on. IsAzureSqlDb is false for MI, so it falls through to productVersion >= 16 and stays off.

Read side: verified, zero changes needed

Every Query Store dedup key already partitions on replica_role and every aggregate already groups on it, so populated Azure values slot straight in:

• Lite/Services/LocalDataService.QueryStore.cs — five PARTITION BY ... execution_type_desc, replica_role dedup windows plus GROUP BY database_name, query_id, plan_id, query_hash, replica_role
• Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryStore.cs — the same shape, twinned
• Darling/.../Mcp/DarlingDataReader.cs, Compose/ComposeCompiler.cs, and the TimescaleSupport corrected-rollup CAGG

The column itself has existed in the Darling store since migration V28, and in Lite's schema since #1546. The reader's ordinal 53 is unchanged.

Lite's own comment already spelled out the case this lands in: "On a standalone/non-AG server every row shares one value (NULL, or 'Primary' on 2025), so the grouping is a no-op and the grid is unchanged." An Azure database goes from one NULL group to one Primary group — still one group, still a no-op, grids render identically. The value simply stops being a lie by omission.

#1871, riding along

BuildPayloadBody's XML doc said the two execution paths select "53 reader ordinals"; the real count is 55. The class-level doc and ReadRowsAsync (whose highest ordinal is 54) already agreed on 55 — the count was just never updated when #1841 added the interval-identity pair. It matters because that comment is the drift guard between the two paths' column sets, so a wrong number sits in exactly the place a future audit would trust.

Swept the repo for other propagations: there are none. Every remaining 53 in the codebase is a real reader ordinal (reader.IsDBNull(53), names[53], row[53]), not a column count. One adjacent doc correction is folded in: Row.ReplicaRole's summary named Azure SQL DB as a NULL source, which this PR makes false — it now names Managed Instance instead and records why Azure changed.

Test plan

• Lite.Tests: 1729 passed, 0 failed
• Darling.Tests: 3656 passed, 0 failed, 201 skipped (the live-PostgreSQL set, gated on env vars exactly as the per-PR CI job gates them)
• Both projects built with -t:Rebuild: 0 Warning(s), 0 Error(s)

Pins changed in Lite.Tests/QueryStoreCollectorDefinitionTests.cs:

• BuildQuery_Azure_GatesReplicaAttributionOff_ButKeepsTheColumn -> BuildQuery_Azure_AttributesReplicaRole_ProvenLiveOnBothServiceTiers. This pin is deliberately inverted — it previously asserted the join and column absent. Its comment now carries the evidence (both tiers, both probe values, the 55/55 Hyperscale payload run) and an explicit instruction not to re-flip it without new live evidence, because a bare inversion is indistinguishable from a regression to the next reader.
• New BuildQuery_Azure_AttributesReplicaRole_EvenWhenTheVersionProbeFails — the Azure arm is an OR, not an AND. If attribution depended on the version at all it would never turn on there, so the edition alone has to decide it.
• BuildPerItemQuery_AzureWithFailedProbe_StillNewColumns gains the MI pin: replica_role OFF and no sys.query_store_replicas, beside its existing plan_type = NULL assertion, so MI's carve-out is pinned rather than merely commented.

Unchanged and still green: the pre-2022 / 2017 on-prem shapes still exclude the join, PayloadColumns_MatchSchemaOrder_56Columns and the 56-column write-order pin are untouched, and the shared-body drift guard (BuildQuery_Azure_AndPerItemWrapper_ShareTheSinglePayloadBody) still proves both builders emit one body.

Filed rather than folded in

Nothing in the implementation was deferred — the gate, the comment, the #1871 propagation sweep, the test pins and the MI pin are all in this PR. Two things this change leaves unverified are filed so they do not vanish when #1872 closes:

• Query Store replica attribution on Managed Instance: still gated off on an unverified basis after #1872 #1886 — Managed Instance is still gated off on an unverified basis. Unchanged behavior, and query_store: collect per database on Azure SQL Database (#1836) #1844's reasoning for excluding it is preserved and still sound (MI is not evergreen and has no per-database path), but this PR's comment now names an MI probe that nobody had scheduled. The issue is that probe, and it argues for a real capability probe over a second edition guess.
• Release gate: confirm #1872's Azure replica attribution through the SHIPPED collector, plus the three axes the validation did not vary #1887 — the evidence came from composing the collector's SQL and running it by hand, not from the shipped collector storing a row. Carries the live-smoke item plus the three configuration axes Query Store replica attribution on Azure SQL DB: General Purpose probe now passes; needs one Hyperscale probe to lift the gate #1872 deliberately did not vary (Hyperscale with HA replicas, provisioned rather than serverless Hyperscale, and readonly_reason & 8), which would otherwise be buried by closing Query Store replica attribution on Azure SQL DB: General Purpose probe now passes; needs one Hyperscale probe to lift the gate #1872.

Closes #1872
Closes #1871

🤖 Generated with Claude Code

[claude]

Review

Reviewed the diff (gh pr diff 1884) against PerformanceMonitor.Collectors/QueryStoreCollector.cs, Lite.Tests/QueryStoreCollectorDefinitionTests.cs, and CHANGELOG.md.

Overview

Flips hasReplicaAttribution for Azure SQL DB from productVersion >= 16 && !IsAzureSqlDb to productVersion >= 16 || IsAzureSqlDb, mirroring the existing hasPlanType "Azure means newest" pattern one line above it, plus a doc-comment ordinal-count fix (53 → 55).

Correctness

• The new gate is a straightforward OR, matching hasPlanType's existing shape exactly — no operator-precedence issue (>= binds tighter than ||).
• MI is correctly unaffected: IsAzureSqlDb is false for Managed Instance, so it still falls through to the pure productVersion >= 16 check. Confirmed by the new MI-specific pins in BuildPerItemQuery_AzureWithFailedProbe_StillNewColumns.
• On-prem/RDS behavior is unchanged: IsAzureSqlDb is false there too, so the OR collapses to the old productVersion >= 16 check.
• The LEFT JOIN (not INNER JOIN) is preserved, which is the right call — an inner join would silently drop rows on any target where sys.query_store_replicas doesn't resolve.
• Ordinal-count fix (53 → 55) in the BuildPayloadBody doc is correct and consistent with ReadItemAsync's highest-read ordinal (54) and the class-level summary; I swept the file for other stale "53" references and found none — the remaining 53s are genuine reader-ordinal indices (IsDBNull(53), etc.), not counts.

Test coverage

• Good use of an explicit probeResult: null case (BuildQuery_Azure_AttributesReplicaRole_EvenWhenTheVersionProbeFails) to pin that the Azure arm is OR, not AND — this is the right edge case to lock down, since it's the one a future refactor to && would silently break.
• The inverted pin's comment explicitly documents why it flipped and asks future editors not to re-flip without new live evidence — good practice for a test whose assertions look, out of context, like a regression.
• BuildPerItemQuery_AzureWithFailedProbe_StillNewColumns now separately pins the MI carve-out (plan_type = NULL and replica_role = CONVERT(nvarchar(1), NULL), plus absence of sys.query_store_replicas), closing the gap where MI's behavior was previously only asserted in a comment.

Lite/Darling parity

No parity drift: QueryStoreCollector lives in the shared PerformanceMonitor.Collectors project used by both apps, so both SKUs pick up the gate change automatically. Verified the read side needs no changes — Lite/Services/LocalDataService.QueryStore.cs, Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryStore.cs, and Darling/.../Mcp/DarlingDataReader.cs all already read ReplicaRole off the same fixed ordinal and already group/partition on it, so a NULL→'Primary' value change requires no reader or dedup-key changes on either side.

Security / Performance

• No new user input reaches string interpolation here — the gate only touches which pre-built, quote-free SQL fragment gets spliced into the query text; no injection surface.
• Performance impact is limited to an additional LEFT JOIN against a 4-row catalog view on Azure SQL DB targets, which the PR's own live-run evidence (55/55 columns bound on Hyperscale) covers adequately.

Minor note

Not a blocker, but the Row.ReplicaRole XML-doc and the hasReplicaAttribution inline comment are both very long-form/narrative (evidence tables, dates, issue numbers) compared to the terser style elsewhere in the file. That's consistent with this file's existing (already-verbose) documentation style for exactly this kind of gate, so I'm not asking for a trim — just flagging it as a style outlier relative to the rest of the codebase.

Overall: correct, well-tested, no parity drift, no security or performance concerns.

[claude]

Review

Reviewed the diff (CHANGELOG.md, PerformanceMonitor.Collectors/QueryStoreCollector.cs, Lite.Tests/QueryStoreCollectorDefinitionTests.cs). This is a tightly-scoped, well-evidenced change and I don't have any blocking findings.

Correctness

• The gate flip (productVersion >= 16 && !IsAzureSqlDb → productVersion >= 16 || IsAzureSqlDb) is logically sound and mirrors the existing hasPlanType pattern one line above it (same "Azure means newest" rule, same >= 16 threshold, same MI carve-out via the version-only fallback since IsAzureManagedInstance isn't part of the OR).
• Verified the SELECT-list ordinal math by hand: replica_role lands at ordinal 52, runtime_stats_interval_id at 53, interval_start_time_utc at 54 — 55 total (0–54), matching the corrected doc comment and the unchanged ReadRowsAsync ordinals (QueryStoreCollector.cs:828-833). The "53 → 55" doc fix is accurate, not just a re-guess.
• LEFT JOIN (not INNER JOIN/filter) is preserved for the new Azure path, consistent with the existing on-prem rationale (standalone servers with an empty sys.query_store_replicas must not lose rows).
• MI stays correctly excluded: IsAzureSqlDb is false there, so it falls through to the version probe, which under-reports and keeps it off. This is now pinned by a real assertion (BuildPerItemQuery_AzureWithFailedProbe_StillNewColumns) rather than just a comment, which is a good strengthening.

Parity (Lite/Darling)

• QueryStoreCollector lives in the shared PerformanceMonitor.Collectors project and is consumed by both Lite/Services/RemoteCollectorService.QueryStore.cs and Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs, so this fix applies to both apps from one edit — no drift risk.
• Read-side claim ("zero changes needed") checks out: replica_role already exists in both stores' schemas and both apps' dedup/rollup logic already groups on it, so an Azure row moving from NULL to 'Primary' is a no-op grouping-wise.
• No Darling-side unit tests reference this gate directly, but that matches the pre-existing pattern — QueryStoreCollectorDefinitionTests in Lite.Tests has always been the sole unit-level pin for this shared collector's query-building logic, so nothing new is undertested relative to before.

Security / Performance

• No user input touches this path — the gate is driven entirely by server-metadata booleans (IsAzureSqlDb, product version probe), not by anything attacker-controlled, so no injection surface here.
• Enabling the LEFT JOIN on Azure adds one join against a 4-row static catalog view (sys.query_store_replicas) per database — negligible, and the PR body's live Hyperscale run backs that up empirically.

Minor / non-blocking

• Cosmetic only: when hasReplicaAttribution is false, replicaJoin is spliced in as an empty string, leaving a blank line before WHERE in the generated SQL (QueryStoreCollector.cs around line 660). Harmless — T-SQL doesn't care — and it's the same pattern the file already used pre-PR, just flagging for awareness rather than asking for a change.

Test coverage for the new behavior looks solid: the inverted pin explicitly documents why it flipped and warns against a bare re-flip, and the new EvenWhenTheVersionProbeFails test correctly isolates that the Azure arm is an OR (not gated by version at all). Nice use of an actual assertion for the MI carve-out instead of relying on comments alone.

No missing-index DMV concerns apply here (not that kind of change). Nothing else stood out as a correctness, parity, security, or performance risk.

[claude]

Risk: this flips the gate ON for every Azure SQL DB (all engine-edition-5 tiers), but the live evidence only covers General Purpose and Hyperscale, neither with real secondary replicas.

The PR's own follow-up (#1887) admits Business Critical / Hyperscale-with-HA-replicas / provisioned Hyperscale are unverified — but Business Critical is exactly the tier replica_role exists to serve (it has real AG-style secondary replicas), and it's untested here.

If sys.query_store_replicas / replica_group_id don't bind on some tier, the failure isn't loud. I traced the per-database loop that runs this query (Lite/Services/RemoteCollectorService.DefinitionRunner.cs:135-213, mirrored in Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs): a SqlException on one database is caught and logged at Debug level, then that database is silently skipped (RemoteCollectorService.DefinitionRunner.cs:206-213). Only a 100%-failure-across-every-database-in-the-cycle case surfaces a Warning and rethrows.

So on a server with a mix of tiers (e.g. some GP, some Business Critical databases), a bind failure on the untested tier would quietly drop Query Store collection for just those databases, with no operator-visible signal at default log levels — the exact "every database going dark" scenario the removed comment warned about becomes instead a silent partial loss, which is arguably harder to notice.

Given the comment's own framing of the risk, might be worth verifying against a database with actual secondary replicas (Business Critical, or provisioned Hyperscale with HA) before this ships broadly, rather than deferring that check to #1887 after the gate is already on in production for all tiers.

[claude]

Review

What it does: Flips hasReplicaAttribution in QueryStoreCollector.BuildPayloadBody from productVersion >= 16 && !IsAzureSqlDb to productVersion >= 16 || IsAzureSqlDb, so Azure SQL DB now populates replica_role instead of a NULL placeholder — the same "Azure means newest" rule hasPlanType already uses one line above. Managed Instance is intentionally left on the pure version gate. A stale "53 reader ordinals" doc comment is corrected to 55 (#1871), and Row.ReplicaRole's XML doc is updated to match. Test pins in Lite.Tests/QueryStoreCollectorDefinitionTests.cs are inverted/added accordingly, and a CHANGELOG entry is added.

Parity: No concern — PerformanceMonitor.Collectors is the shared project both Lite and Darling build against, and the read side (LocalDataService.QueryStore.cs, ViewerDataService.QueryStore.cs, etc.) already partitions/groups on replica_role, so no code changes were needed there. Confirmed only one collector file and one test file reference QueryStoreCollector — no Darling-specific test twin was left out of sync.

Correctness: The gate logic itself is sound and well-isolated — the LEFT JOIN shape (vs. an INNER JOIN or filter) is unchanged from the existing on-prem 2022+ path and is deliberately not tightened, avoiding the "empty sys.query_store_replicas silently deletes all rows" trap. DefaultProductVersion = 13 combined with the new || correctly keeps attribution on for Azure even when the version probe fails, matching the new BuildQuery_Azure_AttributesReplicaRole_EvenWhenTheVersionProbeFails test.

Flagged inline: the gate turns on for all Azure SQL DB tiers (all engine-edition-5 SKUs), but the live-evidence validation described in the PR only covers General Purpose and Hyperscale, neither with actual secondary replicas — Business Critical (which has real AG-style secondaries and is exactly the tier this feature targets) is explicitly filed as unverified in the PR's own follow-up (#1887). I traced the per-database error handling (RemoteCollectorService.DefinitionRunner.cs, mirrored in DarlingCollectorRunner.cs): a bind failure on one database is caught and logged at Debug level and silently skipped — only a 100%-failure cycle surfaces a Warning. So a mixed-tier server with an untested SKU could quietly lose Query Store collection for just those databases, with no visible signal by default. See the inline comment for details — might be worth a quick verification pass against a database with real secondary replicas before merging, given the change is otherwise unconditional.

Minor nit: the CHANGELOG diff introduces one stray blank line into the [#nnnn]: reference-link block (before [#1877]:) that doesn't match the rest of that block's formatting. Cosmetic only.

Security: No new input handling, external calls, or secrets touched — this is a boolean gate change over an existing, already-parameterized query shape.

No missing-index DMV suggestions offered, per instructions.