Skip to content

Collect and surface the ADR persistent version store (#1951) - #1985

Merged
erikdarlingdata merged 8 commits into
devfrom
feature/1951-pvs-collector
Aug 1, 2026
Merged

Collect and surface the ADR persistent version store (#1951)#1985
erikdarlingdata merged 8 commits into
devfrom
feature/1951-pvs-collector

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Closes #1951.

Accelerated Database Recovery keeps its version store inside the user database. When a long-running or aborted transaction pins version cleanup, the data files grow and every size surface in the product reported the growth without ever saying what it was. This adds the collector that explains it, in both apps.

Lane continuation. The first agent on this lane built the collector and opened this PR, then died before merging dev or doing final coordination. This description now separates what it built, what I re-verified from source, and what I fixed — including one live defect on the Azure path that the original docs pass missed.

What ships

  • pvs_stats — a catalog-driven collector over sys.dm_tran_persistent_version_store_stats, one row per database, all 20 DMV columns plus the database name, the ADR enablement flag (inventory, per the issue), and the online data-file size that makes "PVS as a share of the database" computable without a cross-collector join.
  • Cadence 60 min / retention 90 days, deliberately identical to database_size_stats. PVS size is the same slow-moving disk-pressure telemetry, it is read beside database sizes, and matching cadences is what lets "the database grew" and "its version store grew" share one time axis without resampling. The fast leading indicators — the long transaction, the snapshot scan — are already collected per-minute by dmv_blocking_snapshot and waiting_tasks; what this adds is the slow consequence.
  • FinOps > Version Store (PVS) tab in Lite and the Darling Viewer, placed directly after Database Sizes. Byte-identical 15-column sequences in both apps, pinned in ViewerGridPayloadColumnOrderPinTests.Twins so they cannot drift.

The defect I fixed: the Azure path returned zero rows, silently

The Azure SQL Database query text carried JOIN sys.databases AS d ON d.database_id = pvss.database_id, purely to read is_accelerated_database_recovery_on. On Azure SQL Database those two database_id columns are different id spaces.

Microsoft states it on the DB_ID page: "In Azure SQL Database, DB_ID may not return the same value as the database_id column in sys.databases ... These two views return database_id values that are unique within the logical server, while DB_ID and the database_id column in other system views return values that are unique within a single database or within an elastic pool." The DMV's own database_id note repeats it.

So on Azure the predicate compares across id spaces, the INNER JOIN drops the row, and the collector writes nothing — no exception, no log line, an empty result set every hour. Worse, it would match on some databases and not others, so a fleet fails intermittently rather than obviously. Microsoft's own Azure variant of this diagnostic never joins sys.databases, which is the corroboration.

The flag is now read by NAME in a scalar subquery — in a user database sys.databases returns only the current database and master, so d.name = DB_NAME() is exactly one row and never crosses id spaces — and the join is gone from the Azure text. The on-prem/MI path keeps the id join untouched (there both ids are instance-scoped and it is correct). AzureSqlDb_ReadsTheAdrFlagByName_NotByDatabaseId pins each path against the other so they cannot be collapsed.

Two comment claims were also stated more strongly than the docs support, and were written as sourced assertions, so they are corrected rather than left: the permission note had the implication backwards (VIEW SERVER STATE implies VIEW SERVER PERFORMANCE STATE, not the reverse — the conclusion "no new grant" was right, the stated reason was inverted), and "ADR cannot be disabled" on Azure is now "always enabled", which is what Microsoft actually writes.


Docs verification, re-run against primary sources

Every claim was re-checked, adversarially, against MS Learn — not taken from the inherited description.

Confirmed as claimed:

  • sys.dm_tran_persistent_version_store_stats — 2019 (15.x)+, all 20 column names correct and complete. The highest-risk claim held: fetching the raw docs markdown shows zero ::: moniker blocks, so no column is version-gated by moniker, and exactly one carries an inline note: pvs_off_row_page_skipped_oldest_aborted_xdesid is 2022+. A second gated column would have meant a broken query on 2019.
  • The off-row caveat, verbatim: "The size of the off-row versions in PVS, in kilobytes. Does not include the size of row versions stored in-row."
  • The counter-intuitive binding is right: MS attributes the long snapshot scan to min_useful_xts, while oldest_snapshot is "pages skipped during cleanup due to online index rebuild activities". The grids label min_useful_xts "Skipped: Snapshot" correctly.
  • Both of MS's LEFT JOINs are real and are as described, including the OR across two timestamps that can double a row.
  • Rows outliving is_accelerated_database_recovery_on = 1 — confirmed on the management page's "Change the PVS filegroup" procedure.

Corrections found:

  • The four cleaner-time columns are documented datetime2(7), not datetime. Zero blast radius (GetDateTime / CollectorColumnType.Timestamp handle both), noted for accuracy.
  • The permission split is a two-page composition — the DMV page states only the 2022+ form; the 2019 form comes from the system DMV index page. Flagged rather than presented as one source.
  • MS's prose says current_abort_transaction_count; the actual column is current_aborted_transaction_count. A docs bug on their side — the code uses the real column, and this is why the prose spelling was not copied.

Two of Microsoft's published joins are still NOT reproduced

That decision stands, and it is the important part of this collector. MS's diagnostic query resolves a wall-clock time and a session id with two LEFT JOINs; both were reproduced against a live ADR database on SQL Server 2025 (17.0.4045.5) with a genuinely open transaction and a genuinely open snapshot scan, and both returned NULLoldest_active_transaction_id sits in a different id space than sys.dm_tran_database_transactions.transaction_id (66881 against 5694862 in one reading), and min_transaction_timestamp is a cleanup low-water mark rather than any live transaction's sequence number (62903 against 62919).

Shipping those columns would have meant three permanently blank fields under headers promising "the transaction holding cleanup back" — worse than absent, because an operator reads a blank as "nothing is holding it". What ships instead is the comparison MS's own text calls for and which needs no join: the oldest aborted transaction against the oldest active one, as an Aborted Lag number rather than a verdict. Deliberately a number — MS's rule is "much lower ... and the count is large", neither has a documented threshold on dense internal sequence numbers, and a flag firing off an id one lower would be the folklore this PR removed.


Merging dev, and why the migration is renumbered 45 → 47

Dev moved four PRs ahead of this branch's base. Conflicts were resolved keep-both everywhere — the two collectors are unrelated and both belong in the catalog, both schedule-defaults tables, both golden schemas, and both viewer probe ladders.

The renumber is not cosmetic. #1987 took V46. PgMigrations.MigrateAsync reads MAX(version) from darling_schema_version and skips anything at or below it, so a store already stamped 46 would have skipped a late-arriving 45 forever — upgraded stores would silently lack pvs_stats while fresh stores got it from V1's generated schema. 45 is now permanently unused, and the comments at every site say so instead of describing a reservation that no longer exists.

Proven on a live PostgreSQL 18.4 + TimescaleDB rig, both paths, with a real cross-build upgrade:

Path Method Result
Upgrade Database migrated by a dev-base build (origin/dev @ 3259445f, separate worktree), verified at max_version=46, has_45=0, pvs_stats absent — then this build's migrator run against that same store max_version=47, stamps above 44 = 46,47, table + index + v_pvs_stats view present, 27 columns
Fresh This build's migrator against an empty database max_version=47, has_45=0, table + view present

The two provenances agree byte-for-byte: information_schema.columns for pvs_stats hashes to 293a8e6a7ee01bb0a30a73b4367502fb in both.

Two dev tests needed real fixes rather than renumbering. DarlingPlanCorrectionLiveMigrationTests rewound only V46's stamp row, which becomes a silent no-op once 47 sits above it — it now clears every stamp above 44 and expects both scripts to run. And the viewer probe ladder gained a V47 rung ahead of V46, with MapProbedSchemaVersion_PvsStatsAbsent_CapsAt46 pinning it, because the viewer reads v_pvs_stats by name and a V46 store would fail 42P01 rather than degrade.

Worker sizing 43 / 54

Re-derived from the product's own formula now that both collectors are in the catalog, not carried over:

TimescaleSupport.HypertableCount = 40-collector catalog + collection_log = 41
timescaledb.max_background_workers = HypertableCount + 2     = 43
max_worker_processes = 3 + (HypertableCount + 2) + 8         = 54

Set in build.yml and nightly.yml with the derivation comments updated (the merge had auto-combined both lanes' comment variants while leaving the stale 42/53 values). CiClusterWorkerSizingTests parses both files against the formula and passes. The local rig ran at 43/54 too.

Lite DuckDB v51

Dev's CurrentSchemaVersion is 50. The inherited work relied on GetAllTableStatements() creating the table with no version bump; every new-table collector since v37 bumps the version with a log-only arm (v37, v38, … v50 including #1987's), so this now matches the established pattern at 51. The table still comes from GetAllTableStatements() and the v_pvs_stats view the FinOps grid reads still comes from CreateArchiveViewsAsync via ArchivableTables — both catalog-derived, so no hand-maintained list.

Preset tables and #1949: verified N/A, explicitly

  • Schedule presets (Aggressive / Balanced / Low-Impact, three tables per app): pvs_stats is deliberately absent, matching database_size_stats — its declared cadence twin — which is also absent from all six. The preset tables cover the 30 tunable-cadence health collectors, not the hourly size/inventory tier. CrossAppPresetValuePinTests.ExpectedCollectors is unchanged for the same reason.
  • Audit every query screen in Darling and Lite: query text and query plan columns belong at the FRONT of the row, right of the time columns #1949 pin-table rows: N/A, confirmed by inspection. The PVS grid carries no query text and no plan column in either app — the 15 bindings are DatabaseName, AdrDisplay, PvsSizeMb, PvsPercentOfDatabase, DatabaseDataSizeMb, OnlineIndexVersionStoreMb, AbortedTransactionCount, OldestActiveTransactionId, OldestAbortedTransactionId, AbortedTransactionLag, CleanupState, LastCleanupEnd, SkippedLowWaterMark, SkippedMinUsefulXts, SkippedOldestAborted, identical in both. It is twinned without being pinned, and the twin-count assertion says so.

Live collector evidence, re-run on this tree

The earlier run's scratch database was dropped, so this was produced fresh against SQL Server 2025 (17.0.4045.5) with the query text extracted verbatim from PvsStatsCollector.OnPremQueryText, splices resolved as BuildQuery resolves them.

Scratch database with ACCELERATED_DATABASE_RECOVERY = ON, 60,000 rows x 3 KB, updated inside an open transaction:

database_name    | adr | pvs_off_row_mb | data_files_mb | aborted | oldest_active | oldest_aborted
PvsScratch1951b  |  1  |     234.45     |     520.00    |    0    |     31510     |       0
StackOverflow2013|  1  |      16.22     |  107296.00    |    0    |         0     |       0

234.45 MB of off-row PVS = 45% of the database's data files, from one open transaction. After ROLLBACK, the same database reported current_aborted_transaction_count = 1 with oldest_aborted_transaction_id = 31510 — the aborted-transaction accounting the issue asked for.

The inner join was measured, not assumed. In the same run: the DMV returned 18 rows, the join to sys.databases returned 15, over 18 distinct database_ids. The three dropped rows are the hidden internal databases (32761 model_msdb, 32762 model_replicatedmaster, 32767 unnamed) that a user connection cannot name — exactly the row-set determinism the inner join exists for, and no multiplication anywhere.

That run also validated the AbortedTransactionLag guard by accident: with oldest_active = 0 (the DMV's "none tracked" sentinel) and oldest_aborted = 31510, naive subtraction gives −31510. Both apps return null unless both ids are non-zero, so the nonsense value never reaches a grid. The scratch database was dropped afterwards (leftovers = 0); nothing else on the instance was touched.

Tests and gates, all fresh on the merged tree

Gate Result
Lite.Tests (rebuilt, not --no-build against a stale DLL) 2041 / 2041
Darling.Tests gated-live against PostgreSQL 18.4 + TimescaleDB 4158 passed / 0 failed / 11 skipped
Builds 0 Warning(s), 0 Error(s) on every project
CiClusterWorkerSizingTests 4 passed / 1 skipped, against the new 43/54

New tests: Lite.Tests/PvsStatsCollectorDefinitionTests.cs, Darling.Tests/PvsStatsStoreTests.cs, Darling.Tests/FinOpsSubTabIndexPinTests.cs, plus the two added in this round (AzureSqlDb_ReadsTheAdrFlagByName_NotByDatabaseId, MapProbedSchemaVersion_PvsStatsAbsent_CapsAt46).

The inherited work was watched-red across six mutations (2019 gate weakened to 2016; the 2022-only column made unconditional; OUTER APPLY weakened to CROSS APPLY; TOP (1) dropped; the KB→MB conversion dropped; two WritePayload columns swapped), each rebuilt against the test project first and each going red on exactly the intended test.

The Azure text now at least EXECUTES

I hand-edited that query, and nothing offline validates its syntax — the unit tests match strings. So I ran it. Every construct it uses (DB_ID(), DB_NAME(), sys.database_files, the new by-name subquery) is valid on box too, so the text was extracted as BuildQuery resolves it and executed against SQL Server 2025 in the context of a real database:

database_name    | adr | pvs_off_row_mb | data_files_mb | (23 columns, 1 row)
StackOverflow2013 |  1  |      16.22     |  107296.00

It parses, returns exactly one row (database-scoped as intended), binds all 23 columns including the 2022-only one, and the new by-name ADR lookup resolves to 1 rather than NULL — which is the specific thing the broken join was getting wrong. The sys.database_files denominator reads 107296.00 MB, identical to what the on-prem text's sys.master_files reported for the same database, so the two size denominators agree.

What this does not prove is the id-space behavior itself, which only reproduces on Azure. But the fix works by removing the id dependency, so what is left is now exercised rather than assumed.

Still not verified against real Azure

The Azure SQL Database query variant has never been run on Azure — and that caveat now matters more, not less, since this round found a real Azure-only defect in it by reading rather than running. Everything about it is docs-verified and shape-pinned by tests, but that is not the same as having executed it. Filed as #1989 with the exact checks to run, and it should land before the release that ships this.

Follow-up

Alerting and a PVS trend chart were scoped out by the issue itself ("collection and visibility come first") and are filed as #1984.

erikdarlingdata and others added 3 commits August 1, 2026 07:19
Accelerated Database Recovery keeps its version store inside the user
database, so a transaction that pins version cleanup grows the data files
with nothing in the size telemetry saying why. New catalog-driven
pvs_stats collector reads sys.dm_tran_persistent_version_store_stats
(2019+, always on for Azure SQL DB) hourly, 90-day retention to share a
time axis with database_size_stats, surfaced as a FinOps > Version Store
(PVS) tab in both apps.

Two of Microsoft's published diagnostic joins are NOT reproduced, because
they were measured against a live ADR database on SQL Server 2025 and both
returned NULL: oldest_active_transaction_id is a different ID space than
sys.dm_tran_database_transactions.transaction_id (66881 vs 5694862), and
min_transaction_timestamp is a cleanup low-water mark rather than any live
snapshot's transaction_sequence_num (62903 vs 62919). Shipping them would
have meant three permanently blank columns under headers promising the
transaction holding cleanup back. The comparison that needs no join --
oldest aborted against oldest active, MS's own documented read -- ships
instead as an "Aborted Suspected" flag.

The sys.databases join is INNER so the row set does not depend on
configuration: the DMV returns rows for hidden internal databases
(32761/32762/32767) that sys.databases cannot name, and under a LEFT JOIN
those rows vanished the moment any database was excluded, because the
exclusion compares d.name and NULL NOT IN (...) is UNKNOWN.

Store: Darling migration V45 (table + index + the v_pvs_stats passthrough
that keeps the two viewers' SQL byte-identical), StorageVersion 45 with the
matching viewer probe sentinel and gate arm. Lite needs no DuckDB version
bump -- the catalog-generated CREATE TABLE IF NOT EXISTS pass runs on every
startup, the same path the AG collectors took. CI worker sizing moves 41/52
to 42/53 in both workflows, derived from the grown hypertable count.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
database_size_stats carries Compose measures and dimensions, so the
collector sitting beside it should too -- otherwise the new data is
visible in the FinOps grid but uncomposable, which is the kind of gap
that only turns up when someone tries to build the chart.

Three gauges (off-row size, online-index version store, aborted
transaction count) over a single database_name dimension: pvs_stats is
per DATABASE, not per file, so it does not carry the file/recovery-model
dimensions its neighbour does. Max is the default time aggregate for the
sizes, matching database_size_stats -- a pressure peak inside the bucket
is the thing worth seeing, and averaging it away is how a spike
disappears at coarse grain.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review: #1985 — ADR persistent version store collector

Went through the collector implementation, both migration paths, Lite/Darling viewer parity, tests, and CI changes. Summary below; no blocking issues found.

Correctness

  • AppliesTo/has2022Column version gating follows the exact same OR-chain shape as QueryStoreCollector/QueryStatsCollector (SqlMajorVersion == 0 || >= N || IsAzureSqlDb || IsAzureManagedInstance), so it's consistent with established precedent rather than a one-off.
  • Dropping MS's two documented LEFT JOINs (transaction begin time, snapshot session) is well justified and backed by measured evidence in the DMV doc comment — and correctly avoids the row-multiplication hazard from MS's OR-based snapshot join.
  • sys.databases INNER JOIN vs. LEFT JOIN reasoning (hidden internal databases + NULL NOT IN (...) becoming UNKNOWN under exclusions) is correct and matches the UnsoundMicrosoftJoins_AreNotPresent... / SysDatabasesJoinIsInner... tests.
  • Nullable guards throughout (PvsPercentOfDatabase, AbortedTransactionSuspected, CleanupState) behave correctly under C#'s nullable-comparison semantics (decimal? > 0 / long? > 0 short-circuit to false on null), and the "NULL end time = cleanup in progress" MS semantic is preserved rather than coalesced.
  • OUTER APPLY (not CROSS APPLY) for the size denominator correctly keeps a database with no online data files in the result set.

Lite/Darling parity

  • Query text, payload columns, schedule defaults (60 min / 90 days), catalog registration, PvsStatsRow (including every derived member), grid column sequence/headers, and the FinOps SQL (FROM v_pvs_stats, same ORDER BY ... NULLS LAST) are byte-identical between Lite/Services/LocalDataService.FinOps.Pvs.cs and Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.FinOps.Pvs.cs.
  • ViewerGridPayloadColumnOrderPinTests.Twins and the migration/schema pin tests (PgSchemaGeneratorTests, DuckDbSchemaEquivalenceTests) are updated together, so the two front ends can't silently drift on this grid.
  • Darling's V45 migration is required (Lite gets the table automatically via GetAllTableStatements()), and the v_pvs_stats passthrough view is correctly added — confirmed this is necessary since ViewerDataService queries FROM v_pvs_stats unqualified the same way every other FinOps read does.
  • StorageVersion, RequiredStoreSchemaVersion, StoreSchemaProbeSql, and the newest-first MapProbedSchemaVersion arm are all updated in lockstep — a common source of "viewer permanently refuses a healthy store" bugs that's avoided here.

Security

  • The exclusion filter (DatabaseExclusionFilter.Build) is fully parameterized (@excl_db_N) on the on-prem path; no string interpolation of user-controlled database names into SQL. Azure path takes no exclusion input at all (single-database scope), so nothing to inject there either.
  • No secrets, file, network, or process handling introduced.

Tests / CI

  • The mutation table in the PR description (weakened version gate, unconditional 2022 column, CROSS vs OUTER APPLY, dropped TOP(1), dropped KB→MB conversion, swapped WritePayload columns) maps cleanly to specific tests in PvsStatsCollectorDefinitionTests.cs — good adversarial coverage.
  • CI worker-sizing bump (41→42, 52→53) is correctly derived from the new hypertable count and is enforced by CiClusterWorkerSizingTests parsing the workflow files against the formula, so this can't silently drift either.

Nothing here needs to change before merge as far as I can tell — this is very thorough work with strong self-verification (live SQL Server 2025 + live Postgres/Timescale round-trips) baked into the tests themselves.

Nine findings from an adversarial pass over the diff. The two that were
actually shipping wrong behavior:

- **The "Aborted Suspected" flag was folklore.** Its own doc comment
  quoted MS's "if the oldest_aborted_transaction_id is MUCH LOWER than
  oldest_active_transaction_id, and the count is LARGE" while the code
  tested `count > 0 && aborted < active` -- so an aborted id one lower
  than the active one rendered "Likely" under a header claiming a
  diagnosis. On dense internal sequence numbers that fires on benign
  state routinely. Neither "much" nor "large" has a documented
  threshold, and inventing one is exactly what this collector refused to
  do when it dropped MS's two non-resolving joins. It is now an
  "Aborted Lag" number and the operator makes the call MS asks them to.

- **The V45 view was invisible to the drift guard.** It emitted the
  collect.-qualified form, and DarlingObservabilityTests scans
  migrations for the BARE `CREATE OR REPLACE VIEW v_x AS SELECT * FROM x`
  -- the guard that exists so a collector view cannot be added without
  its V14 refresh. It also kept v_pvs_stats out of
  PgSchemaGenerator.AllPassthroughViews, which the MCP reader consults
  to answer "does this table have a v_* view". Now unqualified like
  V10-V13, with the collector registered in PostV8ViewCollectors.

Also: both tabs' help text promised a snapshot-scan column the grid does
not have (reintroducing in prose the exact blank the collector doc spends
28 lines explaining it refused to ship); the CI derivation comment still
said 38 collectors; the OUTER-vs-CROSS APPLY rationale claimed a
difference that does not exist here and was measured to be identical, in
a file whose premise is "measured rather than assumed"; "five" skipped-page
counters were six, and the doc now records that "Skipped: Snapshot" really
is bound to _min_useful_xts per MS, not the similarly-named
_oldest_snapshot, so nobody "fixes" it; CollectionTime was selected,
stored and read by nothing; and a WritePayload test asserted on a local
delta calculator that MakeContext never bound, so it passed regardless.

New FinOpsSubTabIndexPinTests closes the gap this change enlarged: the
Darling FinOps loader switches on POSITIONAL tab indices maintained in a
second file, and nothing enforced the two. Red-watched by inserting a
TabItem without renumbering -- the compiler catches a constant edit
(duplicate case labels) but not the XAML side, which is the direction
that actually happens.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Post-review update. An adversarial pass over the full diff turned up nine findings; all are fixed in 0d5e7c31. Two were shipping wrong behaviour rather than being cosmetic:

The "Aborted Suspected" flag was folklore. Its own doc comment quoted Microsoft's "if the oldest_aborted_transaction_id is much lower than oldest_active_transaction_id, and the current_abort_transaction_count value is large" while the code tested count > 0 && aborted < active. An aborted id one lower than the active one rendered "Likely" under a header claiming a diagnosis — and on dense internal sequence numbers that fires on benign state routinely. Neither "much" nor "large" has a documented threshold, and inventing one is precisely what this PR refused to do when it dropped Microsoft's two non-resolving joins. It is now an Aborted Lag number; the operator makes the call Microsoft asks them to make.

The V45 view was invisible to the passthrough-view drift guard. It emitted CREATE OR REPLACE VIEW collect.v_pvs_stats ..., and DarlingObservabilityTests scans every migration for the bare CREATE OR REPLACE VIEW v_x AS SELECT * FROM x form — the guard that exists so a collector view can never be added without its V14 refresh. The qualified form also kept v_pvs_stats out of PgSchemaGenerator.AllPassthroughViews, which Mcp/DarlingDataReader.cs consults to answer "does this table have a v_* view". Now unqualified like V10–V13, with the collector registered in PostV8ViewCollectors so the guard's set equality holds.

The rest: both tabs' help text promised a snapshot-scan column the grid does not have (reintroducing in prose the exact blank the collector doc spends 28 lines explaining it refused to ship); the CI derivation comment still said "38-collector catalog" next to the bumped 42/53; the OUTER-vs-CROSS APPLY rationale claimed a difference that does not exist for a scalar aggregate and was measured identical, in a file whose whole premise is "measured rather than assumed"; "five" skipped-page counters were six, and the doc now records that "Skipped: Snapshot" really is bound to _min_useful_xts per MS's own text, not the similarly-named _oldest_snapshot, so nobody "fixes" it later; CollectionTime was selected, stored and read by nothing after the derived age column was dropped; and a WritePayload test asserted Assert.Empty(deltas.Calls) on a local calculator that MakeContext never bound, so it passed regardless of what the code did.

New pin: FinOpsSubTabIndexPinTests. The Darling FinOps tab loads sub-tab data by switching on POSITIONAL indices maintained in a second file from the XAML that defines the order. This PR inserted a tab in the middle and hand-shifted six constants; nothing enforced the contract. Red-watched both directions — the compiler catches a constant edit (duplicate case labels), but not a XAML-side insert, which is the direction that actually happens, and which now fails with the tab name it resolved to instead.

Suites after the fixes: Lite.Tests 2020/2020; Darling.Tests 4149 passed / 0 failed / 11 skipped against a live PostgreSQL 18.4 + TimescaleDB store. All six projects build at 0 warnings. CI on the previous head (61b084aa) was 4/4 green including the gated-live darling-pg job; re-running now on 0d5e7c31.

erikdarlingdata and others added 3 commits August 1, 2026 08:45
Dev moved four PRs ahead of this branch's base. #1987 (plan_correction)
took Darling migration V46, Lite DuckDB schema v50, and CI worker sizing
42/53; #1983 moved both READMEs; #1949 reordered the query grids.

Every conflict is resolved keep-both -- the two collectors are unrelated
and both belong in the catalog, both schedule-defaults tables, both
golden schemas, and both viewer probe ladders.

The migration is renumbered 45 -> 47, which is not cosmetic. The applier
reads MAX(version) from darling_schema_version and skips anything at or
below it, so a store already stamped 46 by #1987 would have skipped a
late-arriving 45 forever: upgraded stores would silently lack pvs_stats
while fresh stores got it from V1's generated schema. 45 is now
permanently unused and the comments at every site say so.

Worker sizing re-derived from the product's own formula now that both
collectors are in the catalog: HypertableCount = 40 collectors +
collection_log = 41, so background workers = 43 and worker processes =
54, in build.yml and nightly.yml.

Lite DuckDB CurrentSchemaVersion 50 -> 51 with a log-only migration arm,
matching how every new-table collector since v37 has been registered
(the table itself comes from GetAllTableStatements and the v_pvs_stats
view from ArchivableTables, both catalog-derived).

Count assertions that both lanes had independently bumped 38 -> 39 now
read 40, and the ViewerGridPayloadColumnOrderPinTests twin count is 18.
Two dev tests needed real fixes rather than renumbering:
DarlingPlanCorrectionLiveMigrationTests rewound only V46's stamp row,
which becomes a no-op once 47 sits above it, so it now clears every
stamp above 44 and expects both scripts; and the probe ladder gained a
V47 rung ahead of V46.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…1951)

Docs re-verification of this collector turned up a live defect in the
Azure SQL Database query text, and its failure mode is silence.

On Azure SQL Database sys.databases.database_id is unique within the
LOGICAL SERVER, while DB_ID() and the database_id of every other system
view -- this DMV included -- are unique only within a single database or
elastic pool. Microsoft states it outright on the DB_ID page: "In Azure
SQL Database, DB_ID may not return the same value as the database_id
column in sys.databases", and the DMV's own column note repeats it.

The Azure text carried `JOIN sys.databases AS d ON d.database_id =
pvss.database_id` purely to read is_accelerated_database_recovery_on. On
Azure that predicate compares across the two id spaces, so the INNER JOIN
drops the row and the collector writes nothing -- no exception, no log
line, an empty result every hour. It would also match on some databases
and not others, so a fleet would fail intermittently rather than
obviously. Microsoft's own Azure variant of this diagnostic never joins
sys.databases, which is the corroboration.

The flag is now read by NAME instead, in a scalar subquery: in a user
database sys.databases returns only the current database and master, so
`d.name = DB_NAME()` is exactly one row and never crosses id spaces. The
join is gone from the Azure text entirely. The on-prem/MI path keeps the
id join untouched -- there both ids are instance-scoped and correct, and
a regression test now pins each path against the other so they cannot be
collapsed.

Two comment claims were also stated more strongly than the docs support
and are corrected rather than left as sourced-looking assertions: the
permission note had the implication backwards (VIEW SERVER STATE implies
VIEW SERVER PERFORMANCE STATE, not the reverse -- the conclusion "no new
grant" was right, the reason was inverted), and "ADR cannot be disabled"
on Azure is now "always enabled", which is what Microsoft actually writes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
"ADR cannot be turned off" on Azure SQL Database is one notch stronger
than the docs. Microsoft writes "always enabled" and never says the
setting cannot be disabled; the ALTER DATABASE surface is simply
documented for box only. Same correction as the collector comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Noticed while checking that my own README rows landed in the right files
after #1983 moved them. #1987 shipped plan_correction without adding it
to the root README's collector table or the Darling README's migration
table, so both tables documented one of the two collectors added this
week and the migration table jumped V30 -> V47 with V46 unexplained.

Both are one-line, same-format additions next to rows this PR already
touches. Nothing about #1952's behavior changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review

I read through the new collector, the Lite/Darling storage and viewer wiring, the migration renumbering, the CI worker-sizing changes, and the associated test suites (PvsStatsCollectorDefinitionTests, PvsStatsStoreTests, FinOpsSubTabIndexPinTests, PgSchemaGeneratorTests, the migration-ladder pin tests). Summary: this is a very carefully executed PR and I did not find correctness, security, or parity bugs worth blocking on.

What I verified

  • Azure database_id fix is real and correctly implemented. PvsStatsCollector.AzureSqlDbQueryText reads is_accelerated_database_recovery_on via a scalar subquery keyed on d.name = DB_NAME() rather than joining on database_id, matching the documented Azure SQL DB id-space split. AzureSqlDb_ReadsTheAdrFlagByName_NotByDatabaseId pins it, and the on-prem/MI path correctly keeps the id-based JOIN sys.databases.
  • Payload shape is consistent end-to-end. 23 PayloadColumns line up 1:1 with the ReadAsync reader-index reads and the WritePayload write order, and match the DuckDB (GoldenCollectorSchema) and Postgres (V47Sql/PgSchemaGenerator) table definitions column-for-column.
  • Lite/Darling parity holds. LocalDataService.FinOps.Pvs.cs and ViewerDataService.FinOps.Pvs.cs run byte-identical SQL and expose an identical PvsStatsRow (including derived members PvsPercentOfDatabase, AbortedTransactionLag, CleanupState, LastCleanupEnd). The two FinOpsTab.xaml grids have the same 15-column sequence in the same order, twin-pinned by ViewerGridPayloadColumnOrderPinTests.Twins. Schedule defaults (60 min / 90 day) and catalog registration are present and identical in both apps' schedule/catalog files.
  • Migration renumbering (45 → permanently unused, 47 for this PR) is handled correctly. PgMigrations.Scripts documents the gap, MapProbedSchemaVersion gates the new V47 arm ahead of V46 with a comment explaining why the viewer must refuse a V46 store (it reads v_pvs_stats by name), and DarlingPlanCorrectionLiveMigrationTests was updated to clear stamps above 44 rather than just rewinding 46.
  • CI worker sizing (43/54) matches the stated HypertableCount = 41 formula and the actual 40-entry CollectorCatalog.All + collection_log, consistently in both build.yml and nightly.yml.
  • Null/edge-case handling: AbortedTransactionLag is correctly guarded against the DMV's zero-sentinel (OldestAbortedTransactionId > 0 && OldestActiveTransactionId > 0) rather than doing an unguarded subtraction; PvsPercentOfDatabase guards against a zero/null denominator.
  • SQL injection: the exclusion filter uses DatabaseExclusionFilter.Build, which parameterizes excluded database names (@excl_db_N) rather than concatenating them — no injection surface introduced here.
  • Style: the T-SQL follows the repo's documented conventions (uppercase keywords, column_name = expression aliasing, AS on every table alias, /* */ comments only, trailing commas).

Minor, non-blocking observations

  • PvsStatsCollector.AppliesTo (PerformanceMonitor.Collectors/PvsStatsCollector.cs:338-342) ORs SqlMajorVersion == 0 with SqlMajorVersion >= 15 — the == 0 check is redundant since 0 >= 15 is false and the "unknown assumes newest" behavior would need to be true regardless, so as written it's just a slightly more verbose version of the identical pattern in QueryStoreCollector. Not a bug, just noting it mirrors existing style rather than simplifying it.
  • The Azure SQL DB query path is explicitly called out in the PR description as not yet executed against a real Azure SQL DB instance (tracked as Live-Azure smoke of the pvs_stats Azure SQL Database query variant (#1951) #1989). Given that this PR's headline fix is specifically about Azure behavior, that's worth landing before this ships, as the author already flagged.

No missing-index DMV suggestions, per the review instructions.

@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Green on 4b09329c, the merged head. Not merging — flagging for review.

Check Result
build SUCCESS (8m12s)
Darling PostgreSQL tests (gated-live) SUCCESS (3m12s)
review SUCCESS (5m47s)
check-branches SUCCESS

mergeable: MERGEABLE.

Local gates on the same tree, with both test projects rebuilt first rather than run --no-build against a stale DLL: Lite.Tests 2041/2041, Darling.Tests 4158 passed / 0 failed / 11 skipped against a live PostgreSQL 18.4 + TimescaleDB store, every project at 0 Warning(s).

Three things a reviewer should look at rather than skim:

  1. The Azure defect (commit 9f1a1190). The Azure SQL Database query joined sys.databases on database_id, which is a different id space there than DB_ID() and the DMV's own database_id. It returned zero rows silently, per database, forever. Found by re-reading the docs — every offline test passed both before and after.
  2. The migration renumber 45 → 47 (commit 35d65f64). Not cosmetic: the applier skips anything at or below MAX(version), so the 45 that Surface automatic plan correction in both apps (#1952) #1987 reserved for this lane was already unreachable once 46 was stamped. Proven with a real cross-build upgrade — a store migrated by a dev-base build, then this build run against it — and the fresh and upgraded shapes hash identical.
  3. DarlingPlanCorrectionLiveMigrationTests now rewinds WHERE version >= 46 instead of = 46. With 47 stacked above it the single-row rewind became a no-op that still asserted green.

Deferral filed as #1989 (live-Azure smoke), scoped down in a comment there to only what genuinely needs Azure — the query text itself now executes cleanly on box, including the new by-name ADR lookup.

@erikdarlingdata
erikdarlingdata merged commit f9932a2 into dev Aug 1, 2026
5 checks passed
@erikdarlingdata
erikdarlingdata deleted the feature/1951-pvs-collector branch August 1, 2026 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant