AG collection: document the VIEW ANY DEFINITION grant, make the lag trap filterable, pin V34 for real

[erikdarlingdata]

Review follow-ups to #1688 (the AG health collectors). An adversarial review of that diff cleared the five correctness categories I asked it to attack — column ordering, migration-vs-generated DDL equality, DuckDB golden schema, type mapping, null handling — but turned up four things worth fixing. All four are here.

1. The grant — the one that actually bites in the field

Both AG collectors join the sys.availability_groups / sys.availability_replicas catalog views to the sys.dm_hadr_* DMVs. The DMVs are covered by the VIEW SERVER STATE the product documents. The catalog views are not — they require VIEW ANY DEFINITION, and catalog views enforce that by hiding rows, not by raising an error.

So on a fully configured AG cluster, a monitoring login with only the documented grant got zero rows — byte-identical to what an AG-less server returns, which is the case the collectors deliberately treat as normal. The collectors would have looked healthy indefinitely while collecting nothing, with no error anywhere to notice.

Verified against MS Learn, which states the split directly on Monitor availability groups using Transact-SQL:

Always On availability groups catalog views require VIEW ANY DEFINITION permission on the server instance. Always On availability groups dynamic management views require VIEW SERVER STATE permission on the server.

Corroborated by the SSMS AG dashboard's own documented requirement (CONNECT, VIEW SERVER STATE, and VIEW ANY DEFINITION), and by Metadata Visibility Configuration ("Queries on system views might only return a subset of rows, or sometimes an empty result set"). VIEW ANY DEFINITION is not implied by VIEW SERVER STATE — per the GRANT server-permissions hierarchy they are separate branches meeting only at CONTROL SERVER, so a login in ##MS_ServerStateReader## alone will not see AG rows.

Now documented in:

• README.md — added to the Lite/Darling grant script, plus a short note explaining the silent-zero-rows symptom and naming it as the fix when AG dashboards are empty.
• Darling/README.md — added to the grant script and as its own row in the permission table, with "silently zero rows" in the If-missing column (every other row there describes a visible failure; this one needed to say it doesn't produce one).
• Both collector headers — so the constraint sits next to the query that depends on it.

I also recorded the fingerprint that identifies the condition, in case it's ever worth detecting automatically rather than documenting: the DMV returning rows while sys.availability_replicas returns none is unambiguous, and SERVERPROPERTY('IsHadrEnabled') is readable by every login ("All users can query the server properties"). I did not build that detection — turning "collected zero rows but shouldn't have" into a health signal is a new pathway through the collection-health subsystem in both apps, which is a design decision rather than a mechanical fix.

2. The documented lag trap was unreachable through the surface that ships

secondary_lag_seconds reads 0, not NULL, while data movement is suspended, so a suspended replica charts as perfectly healthy. The original PR documented that carefully and said is_suspended was collected alongside "precisely so a reader can tell the two apart" — but nothing exposed the suspension state to a panel, so the misread the comment warned about was unavoidable in practice.

synchronization_state_desc and suspend_reason_desc are now compose dimensions on ag_database_replica_states, so a lag panel can filter suspended replicas out or group by suspend reason. New test compiles exactly that filter and asserts the value is bound as a parameter rather than inlined.

is_suspended itself can't be a dimension: ComposeCompiler binds filter values as NpgsqlDbType.Text, which wouldn't match a boolean column. The two *_desc columns were already being collected and carry the same information.

3. V34 was not actually pinned — and a comment claimed it was

The migration's test asserted only that each column name appeared somewhere in the DDL:

Assert.Contains($"    {column.Name} ", v34, StringComparison.Ordinal);

A V34 with the columns reordered, is_local typed text, or a spurious NOT NULL passed every test — while a comment right above it asserted the shape equality "is what PgSchemaGeneratorTests pins". It wasn't; that file had zero AG assertions.

Now pinned properly by extending the existing Migrations_JobHistoryAndAgentStatus_MatchGeneratedFreshShape (the idiom already used for V24/V25), which compares the whole generated CREATE TABLE against the migration body. The weak assertion and the false comment are gone.

Detection power confirmed, not assumed: planted is_local text in V34 and the extended test went red; restored and re-ran green. The old assertion would have passed the planted defect.

4. That test's rationale was wrong

The pre-existing comment justified the pin with "the positional binary COPY would mis-bind". That isn't right for the Postgres side — PgCollectorRowWriter.CopyCommandFor emits an explicit column list (COPY t (a, b, ...)) built from PayloadColumns, so Postgres binds by name. I verified that in the source rather than taking the review's word for it.

Corrected to state the real stakes: names and types are the hazard on that side (a renamed/missing column fails the COPY, a retyped one mis-stores), order is pinned so the two provenances stay physically comparable, and the genuinely positional appender is Lite's DuckDB one.

Also

Two CHANGELOG claims in #1688 were slightly wrong and are corrected in place: "in all three cadence presets" implied per-minute everywhere (Low-Impact is 5 min), and "the data is reachable from Custom Views and MCP straight away" is true only of the database-grain table — ag_replica_states is all state strings with nothing numeric to aggregate, so it's stored for the coming viewer tab rather than queryable today.

Not changed, deliberately

• ag_replica_states has no read path. Correct for this cut and honestly allow-listed as Tier-1 debt in both apps' coverage ratchets; the viewer tab is the follow-up.
• DuckDbInitializer.CurrentSchemaVersion stays at 48. Prior new-table PRs bumped it with a log-only block, but Schema.GetAllTableStatements() runs unconditionally with IF NOT EXISTS and both ArchivableTables lists are catalog-derived, so a bump would be a pure no-op.
• Lite/config/collection_schedule.json was already a stale partial seed missing ~15 shipped collectors; ScheduleManager.MergeNewDefaults backfills from GetDefaultSchedules() regardless, so the two AG entries there are harmless either way.

Tests

Lite.Tests      1494 passed, 0 failed
Darling.Tests   3159 passed, 0 failed (149 skipped - the live-Postgres gated set)
Solution build  0 warnings, 0 errors

Installer.Tests not run (live-DB categories clobber real Agent jobs on SQL2022).

🤖 Generated with Claude Code