Darling monitors PostgreSQL and Amazon Aurora PostgreSQL - #2213
Open
erikdarlingdata wants to merge 43 commits into
Open
Darling monitors PostgreSQL and Amazon Aurora PostgreSQL#2213erikdarlingdata wants to merge 43 commits into
erikdarlingdata wants to merge 43 commits into
Conversation
docs/how-collection-works.md described only the deprecated Full/Dashboard edition (SQL Agent + T-SQL procs) and an out-of-date view of Lite in which the DMV queries lived in RemoteCollectorService partials. Neither matches the code: the 41 collector definitions live in the shared PerformanceMonitor.Collectors library, Lite's partials are thin delegations to those definitions, and Darling was not covered at all. Rewritten around the actual shape - one collection brain, two storage engines - documenting the definition model and its opt-in members, the three registration tables, Darling's sweep loop and error/health derivation, the store's column and partitioning conventions, schedule override precedence, and the three retention mechanisms. Full/Dashboard is now called out as deprecated with pointers to its own docs. Also corrects counts that had drifted from the catalog: - README.md and Darling/README.md said 38 collectors; the catalog and CollectorScheduleDefaults both hold 41 - README.md's collector table was missing database_states (1 min, feeds the database offline/unhealthy alert) - Darling/README.md said schema v29; StorageVersion.SchemaVersion is 59. Points at the generated migration-ladder fixture as the complete schema rather than enumerating every rung, since that list is what drifted - Darling/README.md's background-worker sizing said 41/52 for 39 hypertables; HypertableCount is CollectorCatalog.All + 1 = 42, so the derivation gives 44 and 55 - llms.txt said 32 collectors and named Full + Lite as the two editions Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Groundwork for monitoring PostgreSQL targets. Nothing about the current behaviour changes: both the definition side and the target side default to CollectorTargetEngine.SqlServer, so every one of the 41 definitions and every target the probes classify today gate exactly as before. - CollectorTargetEngine: SqlServer / PostgreSql. Deliberately about SQL dialect, not hosting - Azure SQL DB, Managed Instance, and RDS for SQL Server are all SqlServer, since their differences are already carried as flags on CollectorTargetInfo and they run the same T-SQL. - CollectorTargetInfo.Engine: what the target actually is. - ICollectorSchemaInfo.TargetEngine: what dialect the definition speaks. A default interface implementation rather than a required member, so the two test doubles that implement this interface directly, and all 41 definitions, need no change. - CollectorCatalog.AppliesTo(definition, target): the composed gate - engine match AND the definition's own AppliesTo. The runners now call this instead of AppliesTo directly, which is what makes it impossible to dispatch a T-SQL definition at a non-SQL-Server target. Individual definitions stay free to reason only about hosting flavour and version floors within their own engine, and the 27 existing AppliesTo overrides are untouched. Verified: PerformanceMonitor.Collectors, Darling.Service, Darling.Tests, and Lite all build clean. The test suites target net10.0-windows and cannot be executed on macOS (no WindowsDesktop runtime), so they were compile-verified only and still need a run on Windows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the runner Two gaps in the engine discriminator, both found by asking what a second engine in one catalog actually does to the sweep. 1. A gated collector was still being logged. The runner's AppliesTo check returns zero rows, and RunOneAsync records that as SUCCESS - fine for the handful of Azure-gated collectors, but with two engines every target would log a fake success per foreign collector per cycle, most of them at a 1-minute cadence. That floods collection_log (60-day retention) and feeds phantom successes to the health bands and analysis, which key on status rather than row count. RunDueCollectorsAsync now drops wrong-engine collectors before the due-check via CollectorCatalog.EngineMatches: no dispatch, no log row, no NextDue churn. This is Darling's equivalent of Lite's pre-dispatch SKIPPED path. 2. TargetEngine defaulting to SqlServer is right for the 41 existing definitions but a silent footgun for the next one: a Postgres definition that forgot the override would advertise itself as T-SQL and be dispatched at SQL Server targets, failing every cycle. PostgresCollectorDefinitionBase seals TargetEngine to PostgreSql, so the dialect is structural rather than a line to remember, and a drift-guard test asserts every catalog definition is SqlServer today and names any that isn't. Tests added in Lite.Tests/CollectorTargetEngineGateTests.cs: the drift guard, the target default, both directions of the cross-engine gate, that the composed gate still honours within-engine gating (agent_status on RDS), and that an unknown name is not filtered. Full solution builds clean. The test projects target net10.0-windows and were compile-verified only - execution still needs Windows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Postgres The collector definitions were already engine-neutral - they return query text plus parameters and read through DbDataReader, and the collectors library has zero PackageReferences. What was missing was the runner: it constructed SqlConnection directly and mapped parameters to SqlDbType, so nothing but SQL Server could ever execute. - ITargetProvider (in the collectors library, System.Data.Common only, so its zero-dependency property holds): CreateConnection, CreateCommand, Classify. - CollectorTargetFault names failures the way the collection loop reasons about them - Permissions, LockTimeoutYield, SessionMissing, ObjectMissing, FeatureDisabled, CommandTimeout, ConnectionFatal - instead of in one engine's error numbers. Unclassified is the default so an unexpected failure stays loud. - SqlServerTargetProvider is a lift of the existing inline code; the parameter mapping and its throw-on-unmapped-type are unchanged, and Classify reproduces the error numbers the existing catch filters use. It deliberately does NOT produce SessionMissing: whether a 297 means "XE session gone" or "permission denied" is collector context, and the worker already raises its own exception type for that case. - PostgresTargetProvider classifies by SQLSTATE, not message text. Every code was observed while probing our Aurora fleet: 42501 from a function needing rds_replication, 42P01 from pg_stat_statements in a database where the view was never created, 0A000 from pg_stat_wal (Aurora blocks it outright), 55-class from a feature that raises rather than returning empty when disabled. Npgsql was already a dependency - it is the store driver - so this needed no new package. DateTime2 maps to timestamp WITHOUT time zone to match the store's naive-UTC convention; timestamptz would make Npgsql reject the DateTimeKind.Unspecified values this product uses everywhere. The two engine-agnostic runner paths now go through the provider. The Azure per-database and master-enumeration paths stay SqlConnection-typed because they are SQL Server features by definition, but they share the one parameter mapping so a type cannot be mapped two ways. Error-classification catch sites are NOT rewired here - that is a separate change where catch ordering has to be reasoned about carefully. Until then a Postgres failure falls through to the generic handler and is recorded as ERROR, which is honest if coarse. Tests: provider resolution for every declared engine, correct driver types, the parameter mapping is total on both engines, null becomes DBNull, wrong-engine connections are rejected, timeouts apply, and the Postgres SQLSTATE table. Full solution builds clean; the test projects target net10.0-windows so they were compile-verified only and still need a run on Windows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the path from config to a probed target, which is what actually
makes the engine discriminator fire: nothing set CollectorTargetInfo.Engine
to PostgreSql before this, so no Postgres collector could ever dispatch.
- MonitoredServer.engine ("sqlserver" default, or postgres/postgresql/pg/
aurora-postgresql/aurora). This is configuration rather than something
probed because it has to be known BEFORE connecting - it decides which
driver builds the connection string and which detection query runs. An
omitted or misspelled value resolves to SQL Server rather than throwing,
so every darling.json in the field keeps its exact present behaviour and
one bad entry cannot stop the service from starting.
- MonitoredServer.port, for Postgres targets on a non-default port. SQL
Server keeps its host,1433 convention; nothing there changes.
- A Postgres branch in the connection-string builder holding the same
posture as the SQL Server path: 15s connect, 60s command, TLS
fail-closed, and an application name visible in pg_stat_activity. MARS,
ApplicationIntent, and MultiSubnetFailover are absent because the
concepts do not exist - a Postgres read replica is its own endpoint, so
point the entry at the reader's host. TrustServerCertificate maps to
Require rather than disabling TLS, since the case it covers is Aurora
presenting an RDS CA a stock trust store does not know. Integrated auth
is rejected loudly instead of producing a string that cannot
authenticate and failing further from the cause.
- A Postgres detection query built only from surfaces a pg_monitor login
can read, verified against live Aurora 16.11 and 17.7: server_version_num
(a division, not version() text parsing - that formatting has changed
across releases), pg_is_in_recovery(), and a pg_proc lookup for
aurora_version so stock PostgreSQL reads as "not Aurora" instead of
failing the probe.
- Four Postgres facts on CollectorTargetInfo. PostgresMajorVersion gates
the real 16->17 breaks; PostgresVersionNum exists because some gates are
minor-level (aurora_stat_resource_usage needs 16.9+/17.5+ and is absent
on 17.4, so a major-only check would call a function that is not there);
IsAurora gates the proprietary surface, most importantly the cumulative
wait counters core PostgreSQL does not have; IsInRecovery marks a reader,
which on Aurora is a distinct monitoring identity with its own statistics
rather than a shadow of the writer.
The SQL Server-only facts stay at their defaults on a Postgres target
because no Postgres definition consults them, and the engine check keeps
every T-SQL definition away regardless.
Tests: the SQL Server default when engine is absent, every accepted
spelling, typo fallback, the built connection posture, port handling,
the two TLS relaxations, both auth rejections, shared storage identity,
and that no T-SQL leaked into the Postgres detection query.
Full solution builds clean. Test projects target net10.0-windows so they
were compile-verified only and still need a run on Windows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cumulative Aurora wait counters with deltas computed on write - the Postgres counterpart of wait_stats, and the signal Postgres monitoring normally cannot provide at all. Core PostgreSQL has no cumulative wait accounting in any version: only the instantaneous pg_stat_activity.wait_event, with accumulation proposed and rejected twice on overhead grounds. The usual workaround is a sampling extension, and Aurora permits exactly thirteen preloadable libraries, none of which are pg_wait_sampling, pgsentinel, or pg_stat_kcache. Aurora instead exposes aurora_stat_system_waits() as a built-in, so on Aurora it is not the convenient source, it is the only one - hence the gate is IsAurora rather than a version. Signatures in the query are verified against live 16.11 and 17.7, and the AWS reference is wrong about one: aurora_stat_wait_event() returns THREE columns with type_id first, not the four documented. Aliasing it wrong does not error - the join matches nothing and every event name returns NULL, which is how it was found. LEFT JOIN throughout, never the NATURAL JOIN the AWS example shows, because the documented type list omits type 2 and Limitless adds 12; an unnamed wait is still a wait worth recording, and the tests pin that such rows survive. Columns are cast explicitly so reader types are deterministic - Npgsql throws on GetInt64 over an int4 column. wait_time_us carries its unit in the name because the AWS docs contradict themselves (microseconds for system_waits, milliseconds for backend_waits). Settled by measurement instead: read as milliseconds, the observed totals imply tens of thousands of concurrent waiters against a max_connections of 5,000. The name means nobody relitigates it. Deltas key on the numeric event_id, NOT the event name, because wait-event name casing differs between Aurora majors - AutoVacuumMain on 16.11 versus AutovacuumMain on 17.7 - so a name-keyed delta would break its own history across an upgrade, reading as one series ending and another beginning. Same 300-second gap policy as wait_stats. A type-level ignore list drops Activity, Client, and Timeout. Measured on prod, Client:ClientRead alone accumulated 565,758,023 seconds and every Activity event grows at ~1 second per second of uptime forever; left in they are over 99% of the chart. Filtering by type rather than by event name means a new background worker in a future release is excluded automatically. An undecodable type is deliberately kept. Registered in all three tables (catalog, schedule at 1 min / 30 days matching wait_stats, dispatch) plus migration V60 and a StorageVersion bump to 60. Verified with the ladder generator that the generated fresh-store table is column-for-column identical to the hand-written rung, index included - the invariant that otherwise silently breaks the binary COPY on an upgraded store. The catalog-count pins move 41 -> 42, and the engine drift guard is rewritten to key on the pg_ naming convention rather than asserting every definition is SQL Server, so it now catches both directions of mistake. Also fixes generate-ladder-fixture.csproj, which has been unloadable since it was added: its XML comment contained the CLI argument separator, and two consecutive dashes are illegal inside an XML comment. The release-cut tool could not build at all. The frozen v3.3.0 fixture is deliberately NOT regenerated - it represents the previous release, and V60 applying on top of it is exactly what the upgrade test should exercise. Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The viewer/MCP element that ships with the collector, per the one-with-each rule. A separate tool from get_wait_stats rather than a widened one, because the two engines' wait models do not line up: PostgreSQL has a two-level type/event taxonomy where SQL Server has one flat name, has no signal-wait concept at all, and reports microseconds where SQL Server reports milliseconds. Folding them together would mean either lying about a unit or emitting mostly-null columns. DarlingPgWaitReader aggregates the DELTA columns, never the raw cumulative ones - summing cumulative counters across snapshots multiplies the whole history by the snapshot count, which produces a plausible-looking number that is wrong by orders of magnitude, and a test pins it. Microsecond -> millisecond conversion happens once here so no consumer has to remember the stored unit, and the division is float so sub-millisecond events do not collapse to zero. HAVING excludes events that did not move in the window, which otherwise pad the result with every event the instance has ever seen. Unnamed events are surfaced, not filtered. wait_type and wait_event are nullable because their lookups are LEFT JOINed in the collector, and an event Aurora reports but cannot name is exactly the new-wait-type case worth seeing, so it gets a synthetic label from the numeric ids rather than being swallowed by the GROUP BY. The tool reports each event's share of window wait time alongside the absolute figure, since the absolute number alone does not say whether an event is the story or a rounding error. An empty result is explained rather than reported as "no waits": it means either no data, or that the server is not an Aurora PostgreSQL target, and the message says which tool to use instead. Registered in the MCP host's explicit tool-type chain as well as the web endpoint catalog and dispatch tables. Missing the host registration would have left the tool working over HTTP but invisible to every MCP client, which the catalog/endpoint parity test now covers. Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Postgres counterpart of query_stats, reading Aurora's extended
aurora_stat_statements(). Two column groups have no SQL Server equivalent
at all:
- The Aurora I/O source split (storage_blks_read / orcache_blks_hit and
their times) decomposes an otherwise opaque block read into "came from
the distributed storage volume" versus "hit the local NVMe Optimized
Reads tier". This is why a cache-hit ratio computed the community way
is arithmetically misleading on Aurora: a "read" may have been a fast
local hit.
- total_exec_peakmem_bytes / max_exec_peakmem_bytes are the nearest
thing PostgreSQL has to memory-grant data, which core PostgreSQL has
no concept of. Verified populated on prod with no config change.
Per-query wal_bytes is likewise a signal no SQL Server DMV offers.
The query is built per major version rather than SELECT *-ed, because our
fleet spans both and the columns genuinely differ: 16.11 has blk_read_time
/ blk_write_time where 17.7 has shared_blk_read_time /
shared_blk_write_time. A SELECT * would not error on either version - it
would silently shift every ordinal, which is exactly how several
monitoring tools shipped broken PG17 collectors. Casts pin the reader's
types since wal_bytes is numeric and Npgsql's checking is strict.
Delta key is the full (queryid, dbid, userid, toplevel) identity, not
queryid alone: the same normalized statement run by a different user or
against a different database is a separate pg_stat_statements entry with
its own counters, so keying on queryid alone would interleave several
series and produce nonsense. queryid is not stable across major versions,
so a mass reset after an upgrade is expected and the existing
counter-regression handling covers it.
NO query text column, and that was a correction mid-change rather than an
omission. Text belongs in the shared query_text_dim rather than inline -
inline payload was 94% of a 250 GB field store - but registering a new
dim-feeding table cannot be done from a rung this late. V38 is GENERATED
from PayloadDimensions.All, so adding an entry made V38 emit
"ALTER TABLE pg_statement_stats ADD COLUMN query_text_digest", and on an
upgraded store V38 runs long before V61 creates that table: the ALTER
would hit a nonexistent table and fail the whole migration, bricking the
upgrade. Caught by running the ladder generator and diffing its output
against the hand-written rung. Retrofitting a dim-feeding table needs
either an existence-guarded V38 or a rung-aware registry - a design
change, not a drive-by. queryid is the identity meanwhile, which is the
join key anyway, and text is better served by a dedicated low-cadence
collector storing each statement once rather than once per snapshot.
Verified with the generator that the fresh-store emission is
column-for-column identical to V61 (34 columns each) and that no stray
digest ALTER remains. StorageVersion 61, catalog pins 42 -> 43.
Full solution builds clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The viewer/MCP element paired with the collector, per the one-with-each rule. Separate from get_top_queries_by_cpu because the two engines report different things: no signal-wait split, an I/O source breakdown SQL Server has no concept of, and identification by queryid rather than text. The read is deliberately honest about a limitation of the collector's first cut. Only calls, total time and rows have per-interval deltas in the store, so those are SUMmed; the block, WAL and peak-memory columns are cumulative-only and are therefore read as MAX - the latest reading, which is at least a true number - rather than summed across snapshots, which would be arithmetically meaningless. The tool states that distinction in its own output so a caller cannot mistake a cumulative figure for a windowed one, and a test asserts SUM() never appears over a cumulative column, since that is the mistake most likely to be made when someone extends this query. Surfaces Aurora's I/O split as a ratio, not just raw counts: orcache_hit_pct_of_reads separates cheap local NVMe hits from network round trips to the cluster volume. That distinction is the whole reason the collector reads aurora_stat_statements instead of the vanilla view, and it is what makes a community-style cache-hit ratio misleading on Aurora. Grouped by (queryid, database_id) to match the collector's identity, and HAVING excludes shapes that did not execute in the window - pg_stat_statements retains an entry long after its last execution, so without that the list is padded with idle shapes showing zero. The empty-result message names the likeliest real cause rather than just saying no data: on some of our clusters pg_stat_statements exists only in the application database, not in postgres, so a collector pointed at the wrong database returns nothing while looking healthy. Registered in the MCP host's tool-type chain plus the web endpoint catalog and dispatch tables. Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first Tier 0 outage predictor, and the first PostgreSQL collector that is not Aurora-only: it reads core catalog surfaces, so it populates on any PostgreSQL target. Gating it on Aurora would have silently dropped the single most consequential PostgreSQL signal everywhere else, and a test pins that it applies to non-Aurora targets. PostgreSQL transaction ids are 32-bit and wrap, and the consequences escalate on a documented ladder: forced anti-wraparound autovacuum at autovacuum_freeze_max_age (200M default), failsafe mode abandoning cost limits and index cleanup at 1.6B, warnings near 40M ids remaining, and at 3M remaining the server refuses to assign new transaction ids - writes and DDL stop while reads continue. That last state is a write outage no failover fixes, because every replica shares the condition. MultiXact ids are collected as a first-class second counter because they are independent and separately fatal, and are the thing almost nobody monitors. They are consumed when a row is locked by several transactions at once, so a SELECT FOR UPDATE-heavy or foreign-key-heavy workload burns them far faster than plain transaction ids: a server can look comfortable on XID age while being in trouble on MultiXact age. Both live on pg_database, so collecting one and inferring the other would be a choice to be wrong. Two percentages per counter, against deliberately different denominators: distance to an emergency vacuum (autovacuum_freeze_max_age) and distance to the wraparound ceiling (~2^31). Conflating them would make a routine anti-wraparound vacuum read as an imminent outage - about a tenfold overstatement at defaults - and a test pins both denominators. An unreadable setting yields 0 rather than manufacturing a percentage, and therefore cannot manufacture an alert. Percentages are STORED rather than computed on read because their denominators are per-server settings: recomputing later against whatever autovacuum_freeze_max_age happens to be then would silently rewrite history the moment someone tunes it. Uses age()/mxid_age() rather than arithmetic on the raw xid, since both handle the modular wrap that makes naive subtraction wrong precisely near the boundary - the only region where being wrong matters. Reads the shared pg_database catalog, so no per-database fan-out. No deltas: age is a distance from a wall, not accumulated work, and it falls when autovacuum freezes. V62, StorageVersion 62, catalog pins 43 -> 44. Verified with the ladder generator that all three PostgreSQL tables now match their rungs column-for-column (pg_wait_stats 12, pg_statement_stats 34, pg_wraparound_stats 16). Per-relation attribution - which table is holding the freeze floor - is a per-database read and belongs in its own collector; this one answers how much time is left, which is the alerting question. Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Read as a LEVEL, not a rate — the opposite discipline from the wait and statement readers, and deliberately so. Freeze age is a distance from a wall, not accumulated work: averaging it would blur the only number that matters and summing it would be meaningless. So this takes the latest reading per database (DISTINCT ON) plus the window PEAK for each counter, and a test asserts SUM/AVG never appear over an age column. The peak is what makes the pair useful. A current age below the window peak means freezing has clawed age back at least once - the healthy sawtooth. Equal to the peak means age has only ever climbed within the window, which is the shape that ends in a write outage. The tool reports that comparison as freezing_is_keeping_up rather than leaving a reader to eyeball two numbers. Severity comes from the documented escalation ladder, not from round numbers, because each boundary is a real behaviour change: vacuum_failsafe abandoning cost limits and index cleanup around 1.6B ids, server warnings near 40M remaining, writes stopping near 3M remaining. Crucially, a database past its emergency-vacuum threshold but far from the ceiling is classified INFO, not warning - a forced anti-wraparound vacuum is normal operation, and alerting on it is exactly how wraparound monitoring earns a reputation for crying wolf. At defaults that state is 100% of autovacuum_freeze_max_age and only ~9.3% of the way to the ceiling. Both independent counters are surfaced with their own percentages and remaining-headroom figures, since MultiXact exhaustion is separately fatal and a server can look fine on transaction IDs while being in trouble on MultiXacts. The response leads with the worst database and its severity, because one database hitting the wall stops writes for the whole instance - the worst database IS the server's state. The thresholds themselves are included in the payload so a consumer can see what the percentages mean without consulting documentation. Registered in the MCP host chain plus the web endpoint catalog and dispatch. Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reason autovacuum can run, report success, and reclaim nothing. Four unrelated causes present IDENTICALLY from the symptom side - dead tuples accumulate, autovacuum logs success, the table never shrinks - and the remedy is completely different for each: kill a session, drop a replication slot, disable standby feedback, or resolve an orphaned prepared transaction. Doing the wrong one is at best useless. So attribution is the deliverable, not a nicety. The collector emits the oldest holder PER SOURCE and stamps which source is actually setting the horizon, rather than a single aggregate age that would leave a reader exactly where they started. Five sources, and two of them come from pg_replication_slots on purpose: a slot's xmin holds back ordinary row cleanup while its catalog_xmin holds back CATALOG cleanup specifically - what a logical decoding slot pins - and the two can differ by a lot. Collapsing them would misattribute one as the other. A test asserts every source is queried, because missing one does not degrade the answer, it inverts it: the collector would crown a different winner and send someone to fix the wrong thing. DISTINCT ON bounds output to the oldest holder per source, at most five rows a cycle. pg_stat_activity can carry hundreds of backends with an xmin, and storing all of them every minute would be a great many rows saying one thing. is_winner is stamped at collection, not derived on read, so a stored row names the winner as of the moment it was measured. Deriving it later would depend on which rows a query happened to select, and a filtered read could crown a holder that never held the horizon. Exactly one winner is stamped even on a tie, so a stored row is never ambiguous. Zero rows is the HEALTHY state and must never read as a collection failure, so nothing throws or synthesizes a placeholder. Every branch is independently empty-tolerant, which also covers Aurora: pg_stat_replication is expected to be empty there, since replicas read the same storage volume rather than streaming WAL. Per-minute cadence, unlike its wraparound sibling at five: an xmin holder is the fast-moving leading indicator, and the useful catch is the session or slot that appeared minutes ago, before it has cost anything. Not Aurora-gated - core catalog surfaces only. Worth noting the two Tier 0 collectors compose: a pinned horizon also blocks freezing, so an unattended xmin holder is an upstream CAUSE of the wraparound risk pg_wraparound_stats measures. V63, StorageVersion 63, catalog pins 44 -> 45. All four PostgreSQL rungs verified column-for-column against the generator (12, 34, 16, 9). Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reports the current holder per source joined to that source's PERSISTENCE across the window, because presence alone does not tell you what to do. A slot that won 58 of 60 samples is a standing problem someone must own; a session that won twice was a query that ran long and finished. Reporting only current state makes those identical; reporting only the window hides which cause holds the horizon right now. Each source carries its own concrete remedy, which is the entire payoff of attributing the cause: kill or bound a session versus drop an abandoned slot versus chase a stalled logical consumer versus fix a replica query versus resolve a two-phase transaction by gid. Tests assert all five remedies are genuinely distinct rather than one message with the noun swapped, and that an unrecognized source says so instead of guessing. The no-holder case gets a real finding rather than an empty envelope. An operator reaches this tool BECAUSE bloat is growing, so "nothing is holding the horizon" is informative: it redirects the investigation from "vacuum is blocked" to "vacuum is not being triggered", and the response says exactly that instead of leaving a dead end. The response leads with the winning source, its holder, and the recommended action, since that triple is the whole answer. Registered in the MCP host chain plus the web endpoint catalog and dispatch. Fixed a missing System.Linq using in the new test file. Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Worth its own collector even though pg_xmin_horizon already reads slots, because an abandoned slot has a SECOND, independent failure mode that the horizon view cannot see. A slot retains every WAL segment its consumer has not confirmed, and with max_slot_wal_keep_size at its default of -1 that retention is UNBOUNDED: the slot holds WAL until the volume fills, and a full WAL volume stops the server. Same slot, two outages on different axes - disk exhaustion and vacuum starvation - so both xmin counters are recorded here as well and slot state is legible without a join. retained_wal_bytes is COMPUTED from restart_lsn rather than read from safe_wal_size, because safe_wal_size is NULL whenever max_slot_wal_keep_size is -1. Depending on that column would mean reporting nothing on a stock server, which is precisely where retention is unbounded. -1 is stored as the not-applicable sentinel so a consumer cannot mistake "no limit configured" for "no data collected". The LSN reference switches on pg_is_in_recovery(), because pg_current_wal_lsn() ERRORS on a standby and Aurora readers are legitimate targets - without the switch the whole collection would fail on a reader rather than degrading. A test pins it. inactive_since and invalidation_reason are PG17+, conflicting is PG16+; on older majors the collector substitutes NULL/false so the payload shape is identical across a mixed-version fleet and a chart does not change shape at an upgrade. A test asserts both versions select the same number of expressions. inactive_since is the column that turns "this slot is inactive" into "this slot has been inactive for three weeks" - the difference between a consumer between polls and an orphan. Per-minute, 90-day retention: retained WAL grows at whatever rate the server generates WAL, which on a busy writer fills a volume in hours rather than days, and the question after an incident is how long the slot was orphaned, which has to outlive the incident. V64, StorageVersion 64, catalog pins 45 -> 46. All five PostgreSQL rungs verified column-for-column against the generator (12, 34, 16, 9, 20). Full solution builds clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the fifth vertical slice. The read pairs the latest state per slot with that slot's earliest reading in the window, because retained WAL is only actionable as a trend: a slot holding 45 GB steadily is a consumer that is behind but keeping pace, while one that grew from 2 GB to 45 GB in an hour is a volume filling in front of you, and a single current figure cannot tell them apart. Severity turns on that conjunction — WAL retained because of this slot, nobody consuming it, and the pile still growing — rather than on size. Also fills the documentation debt the last five commits accumulated. The Darling README's collector count, schema version, and the TimescaleDB background-worker derivation were all stale, and the worker numbers are load-bearing: an unmanaged store sized to 44/55 while carrying 47 hypertables starves the policies that compress and expire data. Verified against the catalog rather than extrapolated (HypertableTables is CollectorCatalog.All, so it is 46 collector tables plus collection_log). The new PostgreSQL Targets section documents what the seam actually does, including the pg_monitor grant and the two Aurora-gated collectors, and states plainly what is still missing (per-database fan-out, alerting, analysis). Writing it surfaced a real gap: the README claimed integrated auth on a PostgreSQL target is rejected at config validation, but it was only rejected where the connection string is built — which for a service means the misconfiguration appears in a log after deployment instead of in --test-connection before it. Validate() now catches it, and range-checks the optional port while it is there. Clears the five build warnings this branch introduced.
The fan-out seam first. Per-database collection on SQL Server has two possible shapes — switch the connection's catalog, or stay put and prefix the query with EXECUTE [db].sys.sp_executesql — and PostgreSQL has only the first, because a connection there is bound to one database for its lifetime. So ITargetProvider gains the one shape both engines support (WithDatabase) plus BuildDatabaseListPlan, which packages "where to ask for the database list" with "what to ask": SQL Server hops to master, PostgreSQL reads pg_database where it already is. The Azure enumeration now goes through that same plan, so the sys.databases query lives in exactly one place instead of two. What deliberately did NOT move into the provider is the failure policy. An inaccessible master on Azure SQL DB has a real fallback (collect the one connected database) and a re-probe throttle; a PostgreSQL login that cannot read pg_database cannot monitor the server at all, so inventing a fallback there would turn a permissions problem into a quiet partial collection. pg_autovacuum_stats is the collector that exercises it. Dead-tuple counts are the standard PostgreSQL autovacuum metric and they are close to useless alone, because autovacuum fires at a threshold derived from the table's own row count: 500,000 dead tuples is routine on a 50-million-row table and urgent on a 10,000-row one. This computes each table's threshold and stores it alongside the count, honouring per-table reloptions overrides rather than only the GUCs — those overrides are common on exactly the big hot tables where the global default is wrong, so reading the GUC alone would report a threshold the server is not using. The read surface ranks by that ratio, and sorts a table with autovacuum switched off above everything regardless of its count. Two traps worth naming. The activity filter needs the insert clause, not just dead tuples: an append-only table has no dead tuples at all, so a dead-tuple filter drops precisely the tables that never get vacuumed and therefore never get frozen. And reltuples is -1, not 0, on a never-analyzed table, which unfloored yields a negative threshold and makes such a table read as permanently overdue. Cadence is hourly rather than per-minute because on PostgreSQL a per-database collector means one connection per database per cycle. Verified V60-V65 against the ladder generator: all six identical, 24 columns for the new rung. Fleet numbers in the README re-derived rather than incremented (48 hypertables, so 50/61 for an unmanaged store).
This read was mixing two time bases in one row. calls, total_exec_time_ms and rows_returned came from the stored delta columns and covered the requested window, while the block, storage/orcache and WAL figures were MAX() — the latest LIFETIME cumulative reading, since the last pg_stat_statements_reset(), possibly weeks earlier. Nothing in the output distinguished them, so a consumer reading total_exec_time_ms for the last hour beside shared_blks_read since forever would derive per-call I/O ratios that are pure nonsense. The old note admitted the limitation, which does not help: a caller that reads the note still cannot recover the windowed number. Fixed at read time rather than by storing more deltas. The data is already there, sampled per minute, so the difference is computed with a window function instead of adding eight delta series per query shape to the store. GREATEST(value - LAG(value), 0) is what makes that safe, and the reason a plain last-minus-first would not do. A counter reset — an explicit reset, an eviction and re-entry, or a major-version upgrade, since queryid is not stable across majors — makes one interval negative, and last-minus-first reports that as a large negative figure. Clamping each interval at zero drops exactly the reset interval and keeps the rest, which is the same rule the stored delta machinery already applies. The LAG partition is the full series identity (queryid, database_id, user_id, toplevel), matching how the stored deltas are keyed, with the roll-up to (queryid, database_id) happening after. Differencing at the coarser grain would interleave separate pg_stat_statements entries and produce garbage intervals. max_exec_time_ms and max_exec_peakmem_bytes stay MAX: they are high-water marks, not counters. Verified against real Aurora PostgreSQL 16.11 and 17.7 (stage, read-only), not just by text assertion — probe_validate_reader_sql.py substitutes a synthetic VALUES table for the store table, keeps the query body byte-identical, and checks the arithmetic. It confirms the reset case sums to 100 rather than -10, and that a series with one sample in the window reports 0 rather than its lifetime total. The same harness covers the autovacuum reader and proves its ranking: a table 50x past its threshold with 50k dead tuples sorts above one 2.5x past with 500k, which is the inversion the ratio ordering exists to fix.
Two defects found by probing live Aurora before building the next collector, neither of which the build or a text assertion would have caught. The replication-slot collector's PG17+ branch selected inactive_since bare, and that column is `timestamp with time zone`. Npgsql 10 maps a timestamptz read to DateTime with Kind=Utc and refuses to write a Kind=Utc DateTime into the store's `timestamp without time zone` column, so collection would have failed at COPY time on any Aurora 17 target holding a slot that had ever gone inactive — which is to say on exactly the servers the collector exists for. The PG16 branch substituted NULL::timestamp and was correctly typed all along, which is what hid the asymmetry. The autovacuum collector used `x::timestamp` on four timestamptz columns. That form converts, but it renders the instant in the SESSION's TimeZone before dropping the offset, so it agrees with UTC only while every parameter group says UTC. Verified that all probed instances do say UTC today — which is precisely what would keep the bug invisible until one of them didn't. Both now use AT TIME ZONE 'UTC', the only form that is correctly typed AND timezone-independent, pinned by test. The bigger find: pg_autovacuum_stats now gates off standbys. Not for permissions or availability — pg_stat_user_tables reads fine on an Aurora replica and reports ALL ZEROS. Same cluster, same database, same 15 tables, on 17.7: the writer reported 13,654,458 dead tuples and 150,790,506 live tuples while the reader reported 0 for n_dead_tup, n_mod_since_analyze, n_ins_since_vacuum and n_live_tup. Those are the writer's stats-collector numbers and they are not replicated. Ungated, a replica target returns no rows, the activity filter reads that as "nothing has pending work", and the tool reports perfect autovacuum health for a cluster 13 million dead tuples behind. A confidently wrong healthy answer is worse than no answer. The same reasoning applies more weakly to slots, so get_pg_replication_slots' empty result now says it is per-instance and points at the writer, instead of claiming neither WAL retention nor pinned vacuum is possible. Verified the fixed queries execute on live 17.7 and that no store column declared naive comes back carrying a timezone. Ladder regenerated and confirmed byte-identical: this changes SQL and a gate, not schema.
pg_stat_io attributes I/O to a (backend_type, object, context) triple rather
than to a file, so "the database is doing 40k reads/sec" becomes "autovacuum
workers are reading relations in the vacuum context". The context dimension has
no SQL Server counterpart and is the one that changes the remedy: it separates
ordinary buffer-pool misses, where more shared_buffers or a better index helps,
from sequential scans that deliberately bypass the pool through a small ring
buffer, where neither will. High bulkread volume looking like memory pressure and
not being memory pressure is the standard misreading of this view, so the tool
says so per row.
NULL is preserved end to end and never coalesced, which drove most of the design.
PostgreSQL uses NULL for "this counter does not apply to this combination" — the
checkpointer performs no reads or hits, bulkread never extends, the normal
context has no ring buffer to reuse — and on Aurora the ENTIRE write side is NULL
because backends there do not write data files, the storage layer does. Probed
before writing any of it: on 17.7 writes/write_time/writebacks/writeback_time/
fsyncs/fsync_time all come back NULL, and on 16.11 writebacks and fsyncs do. A
zero in any of those places would claim a measurement nobody took, and a consumer
averaging write latency would divide by it. So this is the one Postgres collector
here that uses NULL rather than a -1 sentinel: -1 suits a level a consumer reads
directly, but these are cumulative counters that get differenced, and -1
differenced against a real value is a garbage interval.
The read therefore reports write_counters_tracked alongside the numbers, so a
caller can tell "no writes happened" from "writes are not measured here". Differencing
uses the same clamped positive-interval rule as the statement read.
Two things the probe settled that a guess would have got wrong. The enum values
differ between majors — 17.7 showed a walreplay context and Aurora-specific
backend types ('aurora cache receiver process', 'aurora wal replay process',
'slotsync worker') that 16.11 did not — so nothing filters on them; a whitelist
would silently drop rows. And PG18 REMOVED op_bytes, so it is substituted there
rather than left to fail with "column does not exist"; the replacement per-operation
byte counters are deliberately not added speculatively, since they measure something
different and deserve their own columns decided against a real PG18 target.
Per-minute cadence, unlike the autovacuum collector: this is cluster-wide, so one
connection and no fan-out, and it returned 25-37 rows per snapshot on the fleet —
the same order as pg_wait_stats.
Verified rather than assumed. The collector SQL executed on live stage 16.11 and
17.7 returning 37 and 25 rows with stats_reset arriving naive. The reader SQL ran
on both majors against a NULL-bearing fixture and computed every case: the reset
clamp summing to 100 instead of -10, NULL writes reporting tracked=false, the
checkpointer's real writes reporting tracked=true at 600, bulkread's NULL extends
summing to 0 while its reuses summed to 150, and an idle combination filtered out
rather than returned as zeros. All seven PG rungs diffed identical to the ladder
generator (22 columns for this one).
…R forever The engine seam has had PostgresTargetProvider.Classify since the first commit on this branch, but nothing in the worker consulted it — every SQLSTATE-bearing failure fell through to the general handler, which logs ERROR and records ERROR. That is worst for the conditions that never resolve on their own. pg_statement_stats against a database where the extension was never created raises 42P01 on every cycle; a source Aurora does not implement raises 0A000 on every cycle; a feature gated off in the parameter group raises 55006 on every cycle. At a one-minute cadence each of those is 1,440 identical errors a day, which is how a real finding becomes noise nobody reads. These are the exact PostgreSQL analogue of the 8189 sys.traces denial that already degrades to PERMISSIONS, and for the same stated reason: a legitimate least-privilege or platform reality should not scream every cycle. The store has five statuses and none of them means "this feature is not installed", so those cases take the non-fatal-degradation bucket and the MESSAGE carries the truth — following the AzureDmvPermissionHint precedent, and saying explicitly "NOT a missing grant" plus the fix (CREATE EXTENSION, or the parameter group), because PERMISSIONS on its own would send someone hunting a GRANT that cannot help. Two discriminations worth calling out. A statement_timeout (57014) is an ERROR but is NOT connection-fatal, so it does not trip the reconnect-and-reprobe path — dropping the connection over a slow query would turn a tuning problem into a reconnect storm. And the general handler's reconnect trigger now recognises a Postgres connection failure (08 class, 57P0x) where before it only knew about SqlException, so a dead socket on a PG target went unnoticed and poisoned every subsequent collector on that server. An unrecognized SQLSTATE stays loud, deliberately: the quiet bucket is for conditions we have identified, not a catch-all. Verified against the real shipped code path rather than a copy — a console harness referencing the service project ran the full SQLSTATE truth table through DarlingWorker.PostgresFaultOutcome and PostgresTargetProvider.Classify: 12 states mapped as intended, every emitted status one the store already understands, the yield branch reachable for a collector that opts in, and 57014 confirmed distinct from the five connection-fatal codes.
The three Tier 0 outage predictors are collected, stored and readable, and nothing pages anyone about them. That is the biggest remaining gap for replacing DBM, so this writes down what the work actually is instead of leaving it as "add alerting". The finding is that it is NOT another vertical slice. The previous seven collectors were purely additive; the alert engine is SHARED WITH LITE and is what SQL Server monitoring alerts through today, so one new PostgreSQL alert changes a contract two SKUs implement, four test files reference, and the viewer's Settings window exposes — plus a migration for the new settings columns. Highest blast radius on the branch. That surfaces a real architecture question rather than a coding one: does a PostgreSQL-only signal extend the shared IAlertReadAdapter, forcing Lite to implement three methods for an engine it cannot monitor, or sit behind a separate adapter the engine consults only for PostgreSQL targets? The note recommends the second, for the same reason the collector seam gates by engine instead of having every definition claim every target — but it touches the shared brain, so it is Erik's call, and starting the implementation before that is decided would mean guessing at a seam and rewriting it. The note also records the thresholds each alert should use, derived from what the collectors already measure rather than invented: wraparound against autovacuum_freeze_max_age instead of a raw XID count, xmin against the winning holder's age AND persistence (the collector already attributes the cause, and the four causes need different fixes), slots against wal_status plus whether retained WAL is growing. Each maps to a severity the read surface already computes, so the engine's job is threshold, edge-trigger and dedup — not re-deriving the finding. And the trap an alert would otherwise inherit: anything reading autovacuum state must keep the standby gate, because pg_stat_user_tables reports all zeros on an Aurora reader.
The three Tier 0 predictors were collected, stored and readable, and silent. Now they page. Wraparound risk, a blocked vacuum horizon and replication-slot retention are evaluated on the alert cadence and delivered through the SAME deliverer, history and mute rules as every SQL Server alert, so they land in the same places and obey the same suppression rather than becoming a second notification path nobody configured. Option B as chosen: a separate IPostgresAlertReadAdapter consulted only for PostgreSQL targets. Extending the shared IAlertReadAdapter would have forced Lite — which has no PostgreSQL target and no PostgreSQL collectors — to implement three methods that can only ever return empty, leaving permanent dead code in a shipping SKU to satisfy a contract it has no stake in. This mirrors what collection already does: CollectorCatalog.AppliesTo gates by engine instead of having every definition claim every target. B turned out cheaper than the scoping note estimated. AlertEngine was not touched at all — the evaluator is a pure function of (rows, settings) beside it, and the host calls it after the shared sweep behind an engine check. So Lite, IAlertReadAdapter, IAlertEngineSettings and all four existing alert test files are untouched, and the blast radius collapsed to new files plus one gated call site. Failure-isolated too: a broken PostgreSQL read must not cost a server its CPU or blocking alerts, so it cannot. The thresholds are derived from PostgreSQL's own mechanics rather than picked, which is what makes them defensible as constants for now. Wraparound grades against the SERVER'S OWN autovacuum_freeze_max_age, not a fixed number: warning at 90% of it (before autovacuum force-starts its own prevention vacuum, while a planned one is still an option) and critical at 2x. That scaling matters — 400 million transactions is critical on a stock 200-million server and completely unremarkable on one tuned to 1.5 billion, and a constant would either never fire for the second or constantly for the first. A missing or non-positive setting silences instead of firing, because every derived threshold would otherwise be zero and alert on every database forever. xmin gates on persistence as well as age, which is the whole difference between a chronic holder and a report that ran long — without it this fires on any slow query, which is how an outage predictor earns a mute rule and stops being one. The message carries the remedy for the specific cause, since the five causes are indistinguishable by symptom and need completely different fixes. Slots fire at any size for lost/unreserved (failures that have already happened) and grade the inactive-plus-growing-plus-over-the-line conjunction as the disk-fill emergency, with each part alone a warning. Thresholds are NOT configurable yet: no new config_alert_settings columns, no migration, no Settings-window work. Deliberate first cut, and the design note says so plainly — the moment someone wants a different number, that is the work. Verified against the real shipped evaluator (harness kept alongside the probes): 29 checks over the boundary values, the scaling behaviour, the silencing cases, all five xmin remedies, and the slot grading conjunction. Every boundary asserted on both sides.
Settling the decisions before writing code, because doing it that way for alerting is what made that implementation fast — and this one has more traps than it looks. The load-bearing ones. pg_blocking_pids() takes ShareLock on the lock manager partitions per call, so calling it per row of pg_stat_activity on a 5,000-connection instance makes the monitoring query the incident; filter to wait_event_type = 'Lock' first, which is the only population that can have blockers, so the filter costs nothing. Store the edge list rather than a rendered tree, since root blocker, chain depth and fan-out are all cheap over edges and expensive to recover from a string. Capture the BLOCKER's own state and not just its pid — a chain rooted in "idle in transaction" is an application bug and one rooted in a long query is a tuning problem, and the pid alone does not say which, which is the most common gap in homegrown PostgreSQL blocking monitoring. Also written down: this is a SAMPLING collector, not a blocked-process-report equivalent. PostgreSQL has no ring buffer and no server-side threshold that materialises a report, so blocking shorter than the cadence is invisible. That belongs in the collector's own docs or it will be mistaken for the SQL Server surface it resembles. Two gates NOT to inherit: it should run on standbys (recovery conflicts are real blocking and pg_stat_activity reports the standby's own backends — the autovacuum collector's IsInRecovery gate exists for a reason that does not apply here), and it should not declare YieldsOnLockTimeout, since reading pg_stat_activity takes no table locks and the branch could never fire. And the trap that has already bitten twice on this branch: pg_stat_activity's timestamps are timestamptz, so AT TIME ZONE 'UTC' or store server-computed durations instead.
--test-connection is the deployment gate, and against a healthy Aurora cluster it
printed "SQL major version 0, Unknown (0), msdb access: yes". Every field in that
line is a SQL Server fact a Postgres target does not have: the major version and
edition are zero because nothing probed them, and HasMsdbAccess is true only
because that is its default. A PASS that reads like a misconfiguration is worse
than a FAIL on the one verb whose job is to be believed.
The probe already knew better -- ConnectPostgresAsync fills in the major, the
version_num, Aurora detection and pg_is_in_recovery -- but ProbeAsync dropped all
four on the floor, so nothing downstream could see them.
So carry them, and branch on engine. A Postgres target now reports version, writer
vs reader, Aurora vs not, and then the number that actually answers "will this
target give me what I expect": how many of the seven PostgreSQL collectors clear
the gate, naming the ones that do not.
[PASS] aurora-reader: PostgreSQL 17 (server_version_num 170007), reader (in
recovery), Aurora - 6 of 7 PostgreSQL collectors apply (skipped:
pg_autovacuum_stats)
That count is computed by asking CollectorCatalog.AppliesTo the same question the
runner asks, via ConnectionProbeResult.ToTargetInfo(), rather than by keeping a
parallel list that can rot. A stock-PostgreSQL 15 reader clears three of seven,
and finding that out at pre-flight is the difference between "this is configured"
and "this will collect" -- otherwise the first symptom is an empty table someone
has to explain weeks later.
The two format sites that had each grown their own copy of this string -- the CLI
PASS line and the add_servers MCP detail -- now call one describer, so they cannot
drift; that also settles the small existing divergence in their msdb wording. The
new facts ride alongside the old ones in the test_connect result_json rather than
replacing anything, so an existing consumer keeps working, plus a ready-made
`facts` string for the Viewer dialogs when they get there.
The PostgreSQL fields are trailing optional record parameters, so every existing
construction site still compiles and still means "a SQL Server target".
Verified: solution builds clean; harnesses/probecheck (new) exercises the real
describer against the four target shapes a real fleet has -- Aurora 16/17 writer,
Aurora reader, stock PG 15 reader -- and independently recomputes each count from
the catalog gate. 7/7, 6/7, 7/7, 3/7, all matching. The xUnit assertions are in
DarlingCliCommandsTests and remain unexecuted on macOS.
Writing the first-target runbook found the defect the runbook would have died on at step 5: none of this worked at all against a real store. config.config_monitored_servers is the AUTHORITATIVE server list once seeded -- darling.json seeds it when empty and is ignored afterwards, which is deliberate and documented. Every MonitoredServer field had a column there except Engine and Port. So a PostgreSQL entry was written without its engine, read back as the "sqlserver" property default, and connected to with SqlConnection. Not on a later restart -- on the FIRST start, because SeedIfEmptyAsync is immediately followed by the LoadViewAsync whose result replaces the file's list. Nothing failed to compile, no test covered the round trip, and the seven collectors, the fan-out, the fault classifier and the alerting on top were all built above a target that could never connect. V67 adds both columns, NOT NULL with defaults that make every existing row mean exactly what it means today; the seed writes them, the read restores them. Engine is stored as the raw string the operator wrote, alias and all, so MonitoredServer.TargetEngine stays the single place that interprets it. add_servers can now onboard one, which matters more than it sounds: it is the ONLY path into an already-seeded store, so without this a PostgreSQL target was unaddable to every existing install regardless of the columns. It validates engine STRICTLY, unlike the file parser that resolves anything unrecognized to SQL Server -- the parser's leniency stops one bad line from taking a fleet down at startup, while onboarding is a single deliberate act where "postgress" silently becoming a SQL Server target yields a connection failure against 5432 with nothing naming the cause. Then the pins. Three "the newest rung is N" assertions were still at 59, so the seven PostgreSQL rungs had been breaking DarlingObservabilityTests, PvsStatsStoreTests and StoreSelfMetricsTests since V60 and could not say so on a Mac. Four more in PgSchemaGeneratorTests were literals where the test's own name states an invariant: uniqueness of tables and names asserted as 45 while the real figure tracked the catalog count, and "emits every table" asserted as 46 of 48, which had quietly become a subset check. Those four now assert against CollectorCatalog.All.Count, so they cannot rot again; the catalog count itself stays a deliberate literal. harnesses/pincheck is new and exists because of that class of bug: it evaluates every literal pin against the real assemblies and prints pinned vs actual. It is the only thing here that catches a number a test file three directories away asserts, since the test projects cannot execute on this machine. Also: --test-connection's PASS line and add_servers' detail now report the probe's PostgreSQL facts through one shared describer (a Postgres target read as "SQL major version 0, Unknown (0), msdb access: yes"), and both name how many of the seven collectors clear the gate for that target -- 7 for an Aurora writer, 6 for a reader, 3 for a stock PostgreSQL 15 reader. docs/postgres-first-target-runbook.md is the procedure with a proof point per step, and says plainly that it has never been run end to end. Verified: solution builds 0 errors, 17 warnings all pre-existing (the CA1859 on _alertDeliverer was mine and is fixed). Ladder generates 66 rungs, top V67, with the ALTER ordered after the CREATE it depends on. All six harnesses pass, incl. pincheck's 15 pins. avcheck/iocheck no longer abort after passing when SQLOUT is unset. Nothing here has run against a live store or a live Aurora instance.
dev had moved 53 commits and taken V60 for database-state-edge-memory (#2166) while this branch was using V60 for pg-wait-stats. Two different rungs with the same version number: the applier would run one and skip the other depending on where a store already was, and PgMigrations.Scripts would carry a duplicate version. So dev's V60 keeps its number and the eight rungs here shift up one: 60 database-state-edge-memory (dev, unchanged) 61 pg-wait-stats 62 pg-statement-stats 63 pg-wraparound-stats 64 pg-xmin-horizon 65 pg-replication-slots 66 pg-autovacuum-stats 67 pg-io-stats 68 monitored-server-engine SchemaVersion is 68. The three "newest rung is N" pins conflicted three ways -- dev said 60, this branch said 67, the answer is 68. Nothing else conflicted semantically. dev's #2188 per-database collector_state prune looked like it might: it keys the existence list on database_states, which is a SQL Server collector gated off PostgreSQL targets, so an empty list could have retired pg_autovacuum_stats' per-database state every cycle. It cannot -- the prune is scoped to QueryStorePerDatabaseState.PrunableKeys and runs once per query_store cycle, query_store never runs on a PostgreSQL target, no PostgreSQL collector writes collector_state at all, and the SQL no-ops on an empty snapshot anyway. Checked rather than assumed because it is exactly the shape of thing that would have looked like a collector bug months later. Verified on the merged tree: builds 0 errors / 16 warnings, all pre-existing. Ladder generates 67 rungs, top V68, strictly ascending, no duplicate versions, and dev's V60 ALTER lands before pg_wait_stats' CREATE. All six harnesses pass, including pincheck's 15 pins now reading 68. Still nothing run against a live store or a live Aurora instance.
The connect-time gate refuses a store BELOW RequiredStoreSchemaVersion, which is StorageVersion.SchemaVersion. MapProbedSchemaVersion's newest arm was V60, so a fully-migrated v68 store probed as 60 and the viewer would have refused it with "older than this viewer -- upgrade/restart the service so it migrates the store" on a store that was already current. Every store this branch touches. dev's own V60 arm spells out why the arm has to exist even when nothing in the viewer would 42703: the invariant is that a fully-migrated store maps to EXACTLY SchemaVersion. So V68 gets an arm sensing config_monitored_servers.engine, plus the probe column and reader ordinal that go with it. V61-V67 get no arms on purpose -- they add tables in `collect` that no viewer read names, so a store sitting between them is only ever transient mid-migration, and the invariant that must hold is about the top rung. Three more pins the merge moved: RequiredStoreSchemaVersion is derived from SchemaVersion on dev, so the literal 60s in CollectorMemoryKnobTests, PvsStatsStoreTests and StoreSelfMetricsTests all read 68 now. pincheck grew three checks for this, because the probe is a second place a version has to be updated and nothing links the two: the newest arm must equal SchemaVersion, the reader ordinals must be contiguous from 0, and the probe SQL's top-level column count must equal the ordinal count. It reads the viewer's source rather than referencing the project -- the Viewer is net10.0-windows + WPF and cannot be referenced from a net10.0 console app, which is noted in the harness so nobody retries it. Verified: builds 0 errors / 16 pre-existing warnings; pincheck 18/18.
…ase note PostgresFaultOutcome was inserted directly above IsPermissionError's doc block in a8a98bd, which pushed that block up onto PostgresFaultOutcome and left IsPermissionError undocumented. DocCommentHygieneTests forbids exactly this (#1745, #2190) and would have failed the nightly. Per that test's own instruction, the fix is to MOVE the block back rather than delete the first summary -- seven of the eight found in #1745 were displaced blocks whose real member had been left undocumented, and deleting them lost the documentation instead of deduplicating it. IsPermissionError has its block again and nothing was rewritten. Verified by reproducing the detector's StackedSummaryRuns scan over every .cs in the tree: 1 offender before, 0 after. While in the same sweep, reproduced the reflection-based MCP parity test the same way: 99 tools, 80 /api/read endpoints plus 19 documented exclusions, zero tools without an endpoint, zero endpoints without a tool, and all seven PostgreSQL tools present in the dispatch AND the descriptor catalog. (My first pass at this reported 73 false positives because it sliced from BuildReadDispatch's first MENTION rather than its definition -- worth knowing, since the wrong answer looked alarming.) CHANGELOG gets the release note this branch never had, which for a nightly is the only place any of it is user-visible. No issue number: nothing on the branch cites one, and filing is not mine to do.
The status note said no instance had been monitored end to end, which read as "no Aurora instance has been touched" and undersold what is actually proven: every collector's generated SQL and every MCP reader's SQL has been executed against live Aurora 16.11 and 17.7, confirming shape, no timezone on naive columns, and correct windowed differencing. The real gap is one layer: the service has never done connect probe -> dispatch -> COPY into a store -> read back out. Naming the layer matters more than the disclaimer, because that is where the last defect was -- a PostgreSQL target could not survive its own registration while all of its SQL was correct and live-proven.
First real run of the suite: 561 of 4760 failed. Five causes, and the first is the one worth reading. 1. The collector table was named pg_replication_slots -- which is also pg_catalog.pg_replication_slots, the system view it reads. pg_catalog is searched implicitly and FIRST, ahead of every search_path entry, so an unqualified reference to that name resolves to the system view whatever the store holds. Two failures with very different characters: LOUD: V1's generated schema emits an unqualified CREATE INDEX ... ON pg_replication_slots. Indexing a view is 42809, so MigrateLockedAsync threw, the store never came up, and 531 tests failed behind the collection fixture. QUIET, and much worse: DarlingPgSlotReader and DarlingPostgresAlertReadAdapter both did FROM pg_replication_slots against the STORE. That would have returned the monitoring store's OWN slot list -- normally empty. get_pg_replication_slots would have reported "no slots" forever and the retention alert would never have fired. A silently muted outage predictor is worse than none, and it is exactly what the alerting note says must not happen. Renamed the table to pg_replication_slot_stats; the collector keeps the name pg_replication_slots after the view it reads, which is established practice here (query_store -> query_store_stats, and three more). Schema-qualifying every reference instead would have fixed the loud half and left the quiet half one forgotten qualifier away. Verified on live Aurora 16.11 and 17.7 (probe_catalog_name_collisions.py, new) that pg_replication_slots is the ONLY one of the seven that collides and that pg_replication_slot_stats is clear on both majors. A live-store test now asserts no collector table shadows a catalog object, checked against the real catalog rather than a hardcoded reserved list, with a teeth test so it cannot pass by matching nothing. 2. CI's throwaway cluster is sized off HypertableCount, and seven new tables moved 44/55 to 51/62. build.yml and nightly.yml carry those literals -- a 14th registration point I did not know about. pincheck now checks both workflows and the README, so the next collector cannot miss them. 3. ViewerCollectorCoverageTests: the seven PostgreSQL tables have no WPF reader. Allow-listed as UNBUILT UI with the reason -- they are read through MCP and /api/read today, and the viewer's surfaces are SQL-Server-shaped. One per line so removing one is a one-line diff. 4. RequiredStoreSchemaVersion_TracksTheBuildSchemaVersion_AndTheProbeCoversIt passed 43 hand-counted `true`s; my V68 sentinel was the 44th and defaulted to false, so "a fully-migrated store" quietly meant "one rung short" and the assertion failed on the version instead of on the call site. Now built from the method's own arity by reflection, which cannot drift from the signature. 5. My own TargetProviderTests handed one connection string to both engines; SqlConnectionStringBuilder rejects Host=. Split into a Theory with an engine-appropriate string each, plus a separate every-engine-has-a-provider case for the part that genuinely must loop the enum. Verified: builds 0 errors / 16 pre-existing warnings. Ladder 67 rungs, top V68, with the renamed table created and moved correctly on the fresh-store path. Six harnesses pass incl. pincheck's now-23 pins. Doc-hygiene scan 0 offenders, MCP parity 99 tools / 80 endpoints / 19 excluded / 0 unregistered.
…ore SQL-Server-only Darling PG tests went 561 failures -> 1, and the one left was the fail-closed security gate doing its job: config_monitored_servers.engine and .port were unclassified in DarlingManagedRoles.ViewerRestrictedConfigTables, so the live split test refused them rather than letting a new column reach `viewer` by default. Both are non-secret -- an engine name and a port are exactly as sensitive as the host, which is already readable -- so they join NonSecretColumns. The build job (Lite.Tests, which had not finished on the previous run) was the other red: 10 failures, all one question. Lite's DuckDB schema is generated from the shared engine-mixed catalog, so the seven PostgreSQL tables started appearing in it. They should not. Lite has no PostgreSQL target and cannot acquire one -- the engine gate never dispatches a PostgreSQL definition there -- so those tables would be seven permanently-empty tables in every seat's local DuckDB file forever. Darling makes the opposite call deliberately, because its central store may gain a PostgreSQL target at any time; Lite's file is per-seat and disposable. So DuckDbSchemaGenerator grew StoredCollectors (the SQL Server subset) and the three generation loops walk it instead of the catalog -- filtered in ONE place, so table, index and name generation cannot drift apart the way three independent walks eventually would. Both ArchivableTables lists follow, or Lite would archive tables it never created. Note this makes Lite's emitted schema IDENTICAL to dev's. The PR says no Lite behaviour change and that is still true: filtering preserves today's behaviour, whereas letting the tables through would have changed it. The test pins moved from literals to the derived set, except one deliberately left alone: DuckDbSchemaEquivalenceTests' golden stays pinned at 41 by hand, because it is a frozen historical oracle and is SUPPOSED to fail when a SQL Server collector is added. Also: - CollectorGateSurfacePinTests compared the RAW definition.AppliesTo against the COMPOSED by-name gate. Equivalent only while every collector was SQL Server: the composed overload also requires the engine to match, so a PostgreSQL definition whose own gate returns true unconditionally (slots) legitimately disagrees with it for a SQL Server target. Now compares composed-to-composed, which is the claim that matters -- Lite gates by name, Darling's runner by definition. - The seven MCP tools joined KnownLiteMissingMcpTools as architectural entries, not to-dos, with the reason. - Lite's schedule table is compared against the SQL Server subset of the shared defaults; a phantom row for a collector Lite can never run would surface in its own schedule UI. Verified: 0 errors, 0 warnings. Six harnesses pass.
The viewer column ACL is expressed twice on purpose -- once in C#
(DarlingManagedRoles.ViewerRestrictedConfigTables, applied by the service on a
managed start) and once in Darling/tools/provision-roles.sql (the script an
operator runs by hand against a bring-your-own store) -- and
ProvisionRolesAclDriftTests compares them so neither can quietly diverge. I
updated the C# and not the SQL, so the drift test failed exactly as designed.
engine and port added to the GRANT SELECT list. Non-secret, like host.
pincheck now compares the two ACLs both directions, since this is the second
registration point in two runs that only CI could tell me about. Writing that
check found a bug in itself first: IndexOf("SecretColumns:") also matches inside
"NonSecretColumns:", so the block ended before it began and the C# side parsed as
zero columns -- which reads as 'the C# list is empty' rather than as a parse error.
Anchored past the opening label instead.
Owner
Author
|
Not touching this PR — it's yours — but flagging something about its review before anyone merges on the strength of a green check. The review ran for real and posted nothing. Run 31582450960: 09:20:25 → 09:26:04, conclusion So this is not the green-but-skipped case we already know about (where the action self-skips because the workflow differs from the default branch and reports success anyway). This is a new variant: the review executed, spent five and a half minutes and three and a half dollars on 14,666 changed lines, and its output did not land. Either it genuinely found nothing to say across eight schema rungs and a new engine target, or the posting step failed silently. Worth knowing because the discriminator we've been using — skip signature plus token evidence — says "genuine review" here, and it would have been reasonable to read that as reviewed. The check that actually catches it is cruder: did any comment appear? I'd want the review output before this lands, given what's in it: Suggest re-running the review workflow on this head and checking a comment actually appears. Happy to leave that to you. |
erikdarlingdata
added a commit
that referenced
this pull request
Aug 12, 2026
Numbered 69 rather than 61, and gapped deliberately. #2213 claims V61-V68 and moves StorageVersion.SchemaVersion to 68, and the number is readable from its diff now -- it does not have to land first for me to know it. The gap costs nothing, because the runner skips any rung at or below the store's current version and stamps what it applies; a COLLIDING number is the expensive mistake, since it would be silently skipped rather than rejected and a map table that never got created reads as 'plan not yet collected' on every lookup. The cutover would look healthy and hold nothing. If #2213 grows past V69 before it merges, that surfaces as a rebase conflict on this list, which is a visible failure rather than a silent one. DDL is byte-identical to QueryStorePlanMap.CreateTableSql -- checked, not assumed, since a migration that drifts from the class the code reads through is the same silent-empty-table outcome by a different route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
Full review: three dimensions plus the first live run of the featureRepo-side session. Review ran as three parallel deep passes (engine seam/dispatch, storage/migrations V61-V68/data contract, collectors' SQL/alerting/read surfaces - every finding below was verified in source at head f4dfe26 before inclusion) plus the thing the PR body says nobody had done: the service was pointed at a real PostgreSQL target. Local rig, this branch's build: PostgreSQL 18.4 store (BYO, fresh), stock PostgreSQL 18.4 target on a second cluster, Verdict: the foundation is genuinely strong and the feature is currently non-functional against a live target. The defects cluster in exactly the layer the honesty section predicted - "the queries are proven; the wiring around them is not." The live run, first everWhat passed, with the runbook's own proof points:
What failed, verbatim: Two runbook corrections from executing it (the runbook asks to be corrected from what you actually see): a non-SSL lab target cannot connect at all until BLOCKINGB1. Six of seven PG collectors are handed a HIGH (alert correctness - the wraparound alert is wrong in three independent ways)H1. MultiXact age is graded against the XID GUC. H2. No absolute floor - Critical is unreachable on tuned clusters. H3. H4. PostgreSQL alerts bypass mute rules entirely. H5. A dead PostgreSQL connection never triggers reconnect. The reconnect arm is MAJOR
MEDIUM (summary - full detail available on request)
MINOR / LOW
VERIFIED CLEAN (attacked, held)The catalog gate composition (fail-closed, one surface, all three runner entry points); engine declarations pinned by tests with no typo surface (sealed on the PG base, string parse lenient-in-config/strict-in-add_servers, both tested); V68 registry round-trip (seed $16/$17, load ordinals 13/14, add_servers $15/$16, NOT NULL DEFAULT, non-secret classification, provision-roles grants); GateBefore merge: B1 (with the runner-level connection-type test), the on-load engine gate, XE gating, H1-H5, the mute/cooldown/dedup cluster, the xmin self-attribution + persistence pair (together - fixing one alone worsens the other), the three The honest close: the probe layer, the migration ladder, the catalog gating, and the store contract are the hard parts of a second engine, and they are done well here - several of the tests this PR adds are the strongest in the repo. The wiring defects are all one family (four call sites that reach a monitored server and never learned about engines), they were exactly where the PR body predicted the risk lived, and one of them is a mechanical fix that turns this from non-functional to collecting on five of seven within a day. The live-run transcript and rig recipe are available if useful for the fix round - the same two-cluster setup reproduces everything above in under ten minutes. |
erikdarlingdata
added a commit
that referenced
this pull request
Aug 12, 2026
CI caught two things and the second one is mine to own. The mechanical failure: I added a rung and never bumped StorageVersion.SchemaVersion, so ten invariants asserting the version tracks the newest script went red (ScaffoldTests.StorageSchemaVersion_TracksLatestMigrationScript and friends, plus the live-Postgres tests that migrate a store). The real error: I numbered it 69 to sit above #2213's 61-68 and argued at length that 'the gap costs nothing'. It costs everything in one order. The runner skips any rung at or below the store's stamped version, so if THIS branch merges first and a store stamps 69, every one of #2213's 61-68 is skipped SILENTLY on every upgraded store. I wrote three paragraphs warning about that exact trap and then walked into it from the other side. Gapping is only safe when the gap-filler lands first, which one branch cannot guarantee about another. So this is now 61 = max(dev) + 1 with no gap: whichever PR merges second renumbers to sit immediately above the other, and the ordering is settled by a rebase conflict on the migration list rather than by assumption. A collision is loud. A gap is silent, and a map table that was never created reads as 'plan not yet collected' on every lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
To the author, a coordination question plus a concrete offer - answer here and I will see it within minutes. QUESTION: are you currently mid-fix on B1 (the runner else-branch SqlConnection defect)? One word is enough - 'mine' or 'take it'. IF MINE: the fix shape from the review stands - route DarlingCollectorRunner.RunAsync's else branch (line 478) through TargetProviders.For(server.Target) exactly as the per-database branch at line 269 already does, using provider.CreateConnection plus the provider command path, and add one runner-level test asserting a PostgreSQL target yields NpgsqlConnection (that missing assertion is what hid this). My reproduction rig is two local clusters from the repo's own pg-runtime.zip - extract, two initdbs (store: superuser darling, timescaledb preload, workers 51/62; target: stock, darling_monitor with pg_monitor), darling.json with managed:false store + one servers entry with engine postgres and encryptMode optional (REQUIRED for a non-SSL lab target - fail-closed TLS refuses stock ssl=off otherwise), run the service exe bare (no verb - bare IS console mode). Under ten minutes total; every finding in the review reproduces. IF TAKE IT: I build the B1 fix plus the on-load engine gate and the XE gating (the three findings that share one family) on a branch tonight, run it green against the live rig, and hand you the branch to cherry-pick into this PR - integration and the rest of the gate stay yours. Retrieval note so nothing else gets lost: my review and this comment are PR CONVERSATION comments - fetch via the REST endpoint repos//issues/2213/comments (the review is comment id 5266321251). They do NOT appear under pulls/2213/reviews, and the newest comment on this thread is often a bot summary - filter user.login != claude[bot] when polling. |
… grade each freeze counter properly Review findings B1, the on-load/XE/failed-jobs engine gates, H1 and H2. The live run in the review is what found the first four; every one is a call site that reaches a monitored server and never learned engines exist. B1 (BLOCKING). DarlingCollectorRunner had two paths. The per-database branch resolved TargetProviders.For(target) correctly; the branch serving everything else did `new SqlConnection(server.ConnectionString)` literally. Six of the seven PostgreSQL collectors take that path -- only pg_autovacuum_stats fans out -- so they were handed a SQL Server connection. SqlClient rejects Npgsql's keywords while PARSING the string, before any query runs, and the resulting ArgumentException is neither SqlException nor PostgresException, so it missed both classification arms in DarlingWorker and recorded a raw ERROR every sweep forever. All three Tier 0 outage predictors collected nothing. The provider is now resolved once for both branches, the connection comes from it, and all five command sites in that branch use the provider-aware overload. Connection creation is extracted to CreateTargetConnection so it can be PINNED, because the absence of that assertion is what hid this: both providers were correct and both were separately tested, and nothing checked that the RUNNER asked them. New CollectorRunnerConnectionEngineTests asserts a PostgreSQL target yields an NpgsqlConnection, a SQL Server target still yields a SqlConnection, and -- so the reason is recorded rather than just described -- that a SqlConnection cannot even parse a PostgreSQL connection string and that the throw is neither exception type the classifier handles. Plus a source guard. My first version banned constructing a connection at all, which flagged the Azure SQL master hop -- legitimately opening a SqlConnection from a plan SqlServerTargetProvider built, because per-database enumeration on Azure SQL DB is a SQL Server feature. A rule that flags correct code teaches people to suppress the test, so it is narrowed to the actual hazard: never build a connection from server.ConnectionString, whose engine is whatever the target is. Three more engine-blind call sites, all confirmed in the review's live transcript: - The ON-LOAD dispatch loop had no engine gate, though the scheduled sweep has one with a comment explaining exactly why. A PostgreSQL target ran server_config, database_config, database_scoped_config, trace_flags and server_properties as T-SQL and logged five fake SUCCESS rows. Fake successes are worse than errors here: the health bands and analysis key on status. - XE session provisioning ran SqlClient on every connect, logging "Failed to ensure XE sessions ... deadlock/blocked-process collection will read zero rows" -- alarming and meaningless on an engine with no Extended Events, whose deadlock collectors are engine-gated off anyway. - The failed-jobs feed opened a SqlConnection per alert cycle. msdb and SQL Agent are SQL Server concepts; it gated Azure SQL DB and never considered engine. H1. The wraparound alert graded MultiXact age against autovacuum_freeze_max_age. The governing setting is autovacuum_multixact_freeze_max_age -- 400M by default against the XID's 200M -- so MultiXact warnings fired 2.2x premature while the body printed "autovacuum_freeze_max_age N" beside a counter named MultiXact, contradicting itself. The collector has stored both settings since V63; the adapter simply never selected the second one. Each counter is now graded against its own setting and the worse RELATIVE breach wins, severity first, which is the only comparison that means anything when the denominators differ. One unusable setting no longer silences the other counter. H2. criticalAt was 2x the setting with the wall at 2^31, so above a setting of ~1.07B (tunable to 2B) Critical sat beyond the wall and could never fire -- on exactly the clusters closest to a write outage. The absolute arm is OR-ed in at vacuum_failsafe_age, ~74.5% of the space, matching the ladder DarlingMcpPgWraparoundTools already classifies against, and the message says which arm fired. Verified: builds 0 errors. probecheck pins the connection types and the source guard; alertcheck is 43 assertions green including the premature-MultiXact case, the tuned-cluster reachability case, and both partial-setting cases. Two harness call sites turned out to be wrong rather than the code -- a parameter rename made `fm:` mean the multixact setting where it used to mean the XID one, which silently repointed three existing assertions. Corrected, and both settings now have names that cannot be confused.
…PG alerts mutes, cooldown and per-subject dedup
Review findings H3, H4, H5, the xmin self-attribution/persistence pair, the three
::text renders, and the wrapped-timeout classification.
H3 + the xmin pair, fixed together because fixing one alone makes the others worse.
The session branch filtered WHERE backend_xmin IS NOT NULL. Under READ COMMITTED an
idle-in-transaction writer has RELEASED its snapshot -- backend_xmin is NULL -- while
still holding backend_xid, which pins the horizon just as hard. So the collector
was blind to idle-in-transaction: its single most-cited cause, and the one the read
surface leads with. Now GREATEST of both ages, with either column qualifying.
The collector also attributed the horizon to ITS OWN snapshot. Darling's read sits
in pg_stat_activity with a backend_xmin like any other session, so it was a
permanent 'session' holder -- zero-rows-when-healthy was unreachable, and it padded
the persistence denominator so a real transient holder read as chronic. Excluded
via pg_backend_pid().
And persistence was counted per SOURCE, not per holder: sixty different sessions
each winning once rendered as "pid X held it 60/60", the exact shape of a chronic
holder and the opposite of the truth. Now counted per (source, holder). This only
became reachable once the self-attribution went, because while our own backend was
always a holder every collection had one and the distinction was invisible.
H4. PostgreSQL alerts bypassed mute rules entirely -- Muted was hardcoded false and
the mute check lives in AlertEngine, which this path deliberately rides alongside.
"Alongside" got read as "without". A muted PostgreSQL Wraparound Risk still emailed.
The same MuteRuleService.IsAlertMuted delegate the engine binds is now consulted, so
a mute-rule reload takes effect on the next sweep exactly as for every SQL Server
family.
Also missing on that path, both of which every AlertEngine site has:
- A cooldown. The sweep is 30 seconds, so a database parked over a threshold wrote
~2,880 history rows a day. Now gated per (server, metric, subject) on the
configured cooldown, stamped even when muted so unmuting does not produce a
backlog.
- A real dedup fingerprint. Finding.Subject was computed and then discarded
(Context: null), so the send-side IncidentCooldown fell back to its metric-level
key and two breaching databases -- or two bad slots -- collapsed into one incident
with the second silently suppressed. The subject now travels as a #1140
AlertIncident DedupKey, identity only, so a recurrence of the same subject
collapses and a different subject does not.
H5. The reconnect arm required `ex is PostgresException`, and a dead socket surfaces
as a plain NpgsqlException with no SQLSTATE. The provider ALREADY classifies that as
ConnectionFatal; the call site simply could not reach it, so the runtime stayed
"connected", Server Unreachable never fired, and every collector errored forever.
Any exception on a PostgreSQL target now goes through the classifier.
While in that classifier: a command timeout arrives from Npgsql as an
NpgsqlException WRAPPING a TimeoutException. Only the outer type was checked, so
every real timeout fell through to the NpgsqlException arm and was classified
ConnectionFatal -- a slow query forcing a reconnect, the exact storm the SQLSTATE arm
is careful to avoid. The bare TimeoutException the covering test constructs is a
shape the driver never throws.
The three ::text renders on timestamptz (xact_start, query_start, prepared) now use
AT TIME ZONE 'UTC' like the rest of the branch.
Verified: builds 0 errors / 0 warnings. avcheck pins all five xmin properties;
alertcheck 43 green. The fixed xmin SQL executes on live Aurora 17.7 and returns
ZERO rows, which is the self-attribution fix showing its work -- before it, our own
connection was always a holder.
And a new probe finally PROVES the timestamptz class instead of asserting it:
probe_timestamptz_render_nonutc.py sets the session to UTC+14, where the bare cast
renders 2026-08-13 02:28:27+14 and the UTC render 2026-08-12 12:28:27 -- fourteen
hours and a different calendar day apart. Every previous probe ran at TimeZone=UTC,
where the two are byte-identical, which is precisely why this class kept surviving
review.
…rmalize three files Two remaining pre-merge items from the review. Aurora-only collectors logged SUCCESS on stock PostgreSQL. The pre-dispatch gate covered the wrong DIALECT (EngineMatches) but said nothing about a collector that is right-dialect and simply inapplicable to this target: pg_wait_stats and pg_statement_stats read Aurora-only functions, so on stock PostgreSQL they dispatched, returned 0 rows, and RunOneAsync recorded SUCCESS. At a 1-minute cadence that is ~2,880 fake successes a day per server, feeding the health bands and analysis which key on status -- and the PR body promised a graceful skip with an explanation. Confirmed on the review's live stock-PostgreSQL run. Both dispatch loops now also consult the composed gate. I took pre-dispatch filtering over adding a SKIPPED status because a sixth collection_log status widens a contract shared with Lite, and the outcome is not silent either way: --test-connection already names exactly which collectors do not apply to a target, before the service runs. Scoped to PostgreSQL targets deliberately. On SQL Server the same zero-row-SUCCESS path covers a long-established handful of Azure-gated collectors, and silencing those changes a shipping SKU's log semantics -- that deserves its own decision rather than riding along in a PostgreSQL PR. Line endings. build.yml, nightly.yml and CHANGELOG.md were whole-file renormalizations: 1754, 976 and 5384 changed lines carrying 4, 4 and 8 lines of real content. That poisons every sibling PR with whole-file conflicts -- #2211 is in flight against the same CHANGELOG -- and destroys blame. Restored dev's exact blobs and re-applied the real edits at the byte level. Now 4/4, 4/4 and 2/0, and the CHANGELOG's 6 bare CR characters survive (verified by count before and after, not by eye). Worth recording how this happened, because the trap is easy to re-enter: the working tree holds LF and git stores CRLF through a filter, so a tool that reads a file, edits text and writes it back can flip the whole file without touching a line of content. Byte-level replacement of exact substrings is the only safe method here. My first attempt at measuring it was also wrong -- double-escaped \r\n in a shell heredoc made every CRLF count read zero, which briefly looked like an empty file. Verified: builds 0 errors / 0 warnings; six harnesses green.
… automatic DocCommentHygieneTests was the only CI failure -- 4771 of 4772 passed. Inserting CreateTargetConnection put its doc block directly beneath OpenDatabaseConnectionAsync's, so that member lost its documentation and mine carried two summaries. Per the test's own instruction the displaced block is MOVED back to the member it documents, not deleted. This is the second time today. The first time I wrote down 'check the stacked-summary scan' as a habit; a habit is not a check. It now runs inside pincheck, which I already run every round, so forgetting is no longer possible. Verified: builds 0 errors / 0 warnings; scan reports 0 offenders; pincheck green including the new check.
Both were MEDIUM findings in the review, both were my own doing while fixing CI, and both are cheap enough to repair rather than defer. CollectorGateSurfacePinTests compared AppliesTo(definition, t) against AppliesTo(name, t). The by-name overload just looks the name up and calls the by-definition one, so only a corrupt name->definition map could fail it and a bug in the composed rule itself would sail through. The expected value is now STATED independently -- TargetEngine == target.Engine && definition.AppliesTo(target) -- and both overloads are checked against it, so the lookup AND the composition are pinned. DuckDbSchemaGeneratorTests counted the filtered sequence against itself, which cannot fail. It now compares the emitted table NAMES against the stored collectors' names as a set, and asserts no PostgreSQL table leaks into Lite's DuckDB -- which is the actual invariant that file guards after the engine filtering. Also moved the local verification harnesses to Darling/tools/pg-harnesses and gitignored them. They previously lived on one SQLServerSetup branch, so every branch switch there deleted them mid-session; they now sit in the repo whose branch I control. Ignored rather than committed because whether pincheck belongs in the repo is a decision, not a default -- it is the only check for the CI workflow cluster numbers, the C#-vs-SQL viewer ACL, the schema-probe arity, and now the stacked-summary scan, none of which any test covers. Verified: builds 0 errors / 0 warnings; pincheck green, incl. the rebuilt tree scans (99 MCP tools / 80 endpoints / 19 exclusions, 0 stacked summaries).
The last 'same PR strongly recommended' item from the review. The comment claimed 'NULL survives the arithmetic on purpose'. It does not, in two places: GREATEST(NULL, 0) returns 0 because GREATEST ignores NULLs, and the outer coalesce(SUM(...), 0) would flatten it regardless. Outputs are unchanged -- write_counters_tracked is computed on the RAW column and already carries the information -- so this is a comment fix, not a behaviour fix. But the comment was worse than useless: it would have licensed someone to drop the tracked flag believing the NULLs were carrying the distinction. The comment now says plainly that the numbers DO come back as 0 and that the flag is the only discriminator between 'no writes happened' and 'writes are not measured here'. Verified on live Aurora 17.7 rather than reasoned about: GREATEST(NULL,0) = 0, GREATEST(NULL-NULL,0) = 0, and the CASE-guarded form preserves NULL. I also overstated this in the pganalyze gap analysis, claiming Darling was more correct because it preserves NULL where they coalesce. Corrected there and in memory: neither product preserves the NULL through the read, and Darling's genuine advantage is narrower -- it returns a trackedness flag and pganalyze has no equivalent.
…s, and correct the runbook Three more review items, two MEDIUM and one from executing the runbook. The MCP wraparound severity graded the XID percentage only, so a database at 80% toward MultiXact wraparound and 3% on XIDs was labelled 'ok' -- directly contradicting the tool's own description, which promises 'a server can look fine on transaction IDs and be in trouble on MultiXacts'. Both counters are now graded on the same ladder, the worse label wins, and the label says '_multixact' when that is the reason, since the remedy differs and a reader would otherwise go looking at transaction IDs. The autovacuum reader ranked on dead tuples over threshold, which buries append-only tables at ratio 0 -- below the LIMIT -- even though never-vacuumed is a classic wraparound route and the collector gathers inserts_since_vacuum precisely for it. Now ranks on the WORSE of the two ratios. The -1 not-applicable sentinel is kept out of the arithmetic by a CASE rather than producing a negative ratio. Validated on live Aurora 17.7 with a fixture built for the failure: an insert-only table at 10x its insert threshold with zero dead tuples now ranks above a table at 0.5x its dead threshold, a disabled table still sorts first, and the -1 sentinel yields no ratio. A ranking bug is invisible without a row that exercises it. Runbook, from the reviewer actually running it: - A target with TLS off entirely needs encryptMode 'optional'. trustServerCertificate relaxes VERIFICATION, not the requirement, so stock self-hosted PostgreSQL with ssl=off -- the normal lab shape -- is unreachable until the other setting changes. The failure gives no hint of that. - There is no --console verb; the bare exe IS console mode. Worth a sentence because hunting for the flag reads like a missing feature. Verified: 0 errors / 0 warnings; four harnesses green.
Housekeeping from the review's MINOR list, all verified rather than reasoned about. The wait reader's LIMIT was hardcoded at 50 while the tool advertised a caller-supplied limit and applied it with Take(limit). A caller asking for more than 50 silently got 50, and every smaller request fetched rows to discard them. Now $4, matching every other read in this store, and the redundant Take is gone. The aurora_stat_wait_event join keyed on event_id alone. That is correct TODAY only because Aurora packs the type into the event_id's high byte (event_id >> 24 = type_id) -- an undocumented encoding, while the function hands us type_id in the same row. A future Aurora reusing an event_id under a different type would have produced a row-multiplying join that doubled every affected wait figure. Now keyed on both. Verified on live Aurora 17.7 before and after, because a join-key change can multiply rows: 32 source rows, 32 with the old join, 32 with the new one, and all 340 rows of the wait_event catalog satisfy the high-byte invariant -- which is precisely why the old form worked and why relying on it was still wrong. Both workflow files' comments still described 'the 41-collector catalog + collection_log = 42' two lines above the corrected 51/62 numbers. Now 48 + 1 = 49. Patched at the byte level, so the files stay 4/4 against dev. Lite's gate comment still said Darling's runner 'calls definition.AppliesTo(target) directly'. It calls the COMPOSED CollectorCatalog.AppliesTo(definition, target), which also requires the engine to match -- the distinction the whole two-engine gate rests on. Verified: 0 errors / 0 warnings; four harnesses green.
… templates The last two wraparound items from the review's MINOR list, both verified on live Aurora 17.7 rather than reasoned about. xids_remaining counted to the raw 2^31. PostgreSQL refuses new write transactions with roughly 3,000,000 ids still on the clock, so the column overstated the runway by that margin -- and disagreed with the MCP tool's own 99.86%-of-space figure, which already accounts for it. Now counts to the stop point, clamped at 0 so a database past it reports none left rather than a negative. The read excluded templates. The cluster-wide stop limit derives from the oldest datfrozenxid anywhere in pg_database, so excluding template0/template1 could understate cluster risk by exactly the amount that matters, and template0 aging without ever being vacuumed is a documented route there -- usually after a major upgrade. The reviewer flagged that the shared-catalog behaviour was worth confirming rather than assuming, so I confirmed it: datfrozenxid comes from pg_database, which is SHARED, and template0's row reads fine despite datallowconn = false. No connection is involved, so the setting that blocks the per-database fan-out is irrelevant here. That makes two queries with opposite and both-correct answers, which is a trap for the next reader: the per-database enumeration MUST keep excluding templates because it opens a connection per database and template0 refuses them. The Lite test now asserts the inclusion and names the Darling test that pins the exclusion, so nobody aligns them in the name of consistency. Worth noting the harness earned itself: my new doc block landed above an existing one-liner -- the same stacked-summary trap, third time -- and pincheck caught it in seconds instead of eight minutes of CI. That is the whole reason it went in there. Verified: 0 errors; four harnesses green; ladder still 67 rungs / top V68 (the column set is unchanged, only the computed values and the row filter).
Owner
Author
|
Migration-number update for your fix round: #2211 has correctly claimed V61 (its CI is green on it after a ladder-pin fix - dev's tests enforce a CONTIGUOUS ascending ladder, so the earlier V69-with-a-gap approach was structurally impossible, which is worth knowing for any future claim). Your V61-V68 rungs must shift to V62-V69 when you rebase for the fix round, and StorageVersion/probe arms move with them. If #2211 has not merged by the time you push, re-verify dev's actual MAX at that moment - whoever lands second renumbers. |
QueryStoreBackfill.RunServerSliceAsync gated on QueryStoreCollector.Instance .AppliesTo(target) — the definition's own override, which never checks the engine. It reads SqlMajorVersion, and CollectorTargetInfo treats 0 as 'assume newest', so a PostgreSQL target (which has no SqlMajorVersion at all) sailed straight through into a method that opens SqlConnections. Latent today only because the caller is reached from a SQL-Server-shaped path; one new call site and it is B1 again. Now the composed CollectorCatalog.AppliesTo, which requires the engine to match. PgTableTuning omitted pg_statement_stats from the per-table autovacuum_vacuum_insert_scale_factor overrides. It is query_stats' per-minute PostgreSQL twin — same shape, same cadence, same pure-insert hypertable chunks — so the file's own EXPLAIN-backed rationale applies unchanged: the stock 0.2 leaves the day's hot chunk stale before the TimescaleDB rollover and the Index Only Scan degrades to heap fetches. Missed because the list is hand-maintained rather than derived from the catalog. Which is also why the test pinned Statements.Count at a literal, and adding a statement broke it — the same count-pin class that has now bitten several times. Pin updated to 11 with the arithmetic spelled out, and pincheck pins both the count and the number of tables carrying the override, so the next addition is caught locally. Verified: 0 errors; four harnesses green.
…L substring The only CI failure on the last push -- 4765 of 4772 passed. The test matched "l.dead_tuples::numeric / NULLIF(l.vacuum_threshold, 0) DESC" verbatim, and adding the insert-only ratio reformatted that clause onto separate lines. The behaviour it exists to protect was intact and strictly improved; the test failed on whitespace. Now asserts the ORDER BY's structure: both ratios present, each dividing by its own threshold, the denominators guarded, both combined with GREATEST so the WORSE one decides rather than one being a tie-break behind the other, and the three tiers in order -- disabled, then ratio, then raw counts. Verified the assertions against the real generated SQL before committing, since I wrote both sides of this one and a structural assertion that quietly matches nothing is worse than the brittle version it replaced. The behaviour itself is separately proven on live Aurora, where an insert-only table at 10x its threshold with zero dead tuples ranks above a table at 0.5x its dead threshold. Verified: 0 errors; four harnesses green.
Owner
Author
|
Migration-number traffic update for your fix round: I am claiming V62 for #2171's plan_xml_compression store setting (building now, targeted at 3.5.0). With #2211 holding V61, your rungs shift to V63-V70 when you rebase. Same rule as before: whoever pushes re-verifies dev's actual MAX at that moment. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Teaches Darling to monitor PostgreSQL and Amazon Aurora PostgreSQL alongside SQL Server. A monitored
server declares
"engine": "postgres"and is collected by seven PostgreSQL collectors instead of the T-SQLones, into the same store, on the same naive-UTC contract and the same
server_ididentity. A mixed fleetis one store, one viewer, one MCP endpoint — nothing is partitioned by engine.
Every collector definition declares the engine it targets and is never dispatched against the other one, so
a PostgreSQL target is never sent T-SQL and a SQL Server target never sees
pg_stat_statements. A storemonitoring only SQL Server gains seven empty tables and nothing else changes.
What gets collected, and why each earns its place:
pg_wait_statsaurora_stat_system_waits()pg_stat_activity.wait_eventis an instantaneous sample, so there is nodm_os_wait_statsequivalent to read elsewherepg_statement_statsaurora_stat_statements()pg_wraparound_statspg_database,pg_classpg_xmin_horizonpg_stat_activity,pg_replication_slots,pg_stat_replication,pg_prepared_xactspg_replication_slotspg_replication_slotspg_autovacuum_statspg_stat_user_tables,pg_classreloptions— without the threshold the count is not actionablepg_io_statspg_stat_io(backend_type, object, context)rather than by file. PostgreSQL 16+, valid on a standbyThree of the seven are outage predictors rather than performance metrics, and they alert — graded against
the target's own settings (wraparound against that cluster's
autovacuum_freeze_max_age, not a constant),delivered through the same deliverer, history and mute rules as every SQL Server alert. That is deliberate:
PostgreSQL's most damaging failures are quiet, slow, fully predictable days ahead, and nothing in the engine
raises its hand about them.
Also here: the engine-execution seam (
ITargetProvider), per-database fan-out for PostgreSQL, faultclassification so an operator-actionable condition records a non-fatal skip with an explanation instead of
logging
ERRORevery cycle, and a read surface + MCP tool per collector.Permissions are one
GRANT pg_monitor. Nothing is created on the monitored instance — no Extended Eventssessions to provision, no server setting to bootstrap.
Two defects found while writing the runbook, worth calling out for review
config.config_monitored_servershad noengineorportcolumn. That registry is authoritative forthe server list once seeded, so a PostgreSQL entry round-tripped as
"sqlserver"(the property default)and the service opened a
SqlConnectionto port 5432 — on the FIRST start, since the seed is immediatelyfollowed by the load that replaces the file's list. Fixed by V68 plus the seed/read paths and
add_servers. A new property-driven test fails when any round-trip-criticalMonitoredServerpropertylacks a column.
MapProbedSchemaVersion's newest arm was V60.RequiredStoreSchemaVersionisStorageVersion.SchemaVersionand the connect-time gate refuses a store below it, so a fully-migratedstore would have been rejected as "older than this viewer". V68 gets an arm.
Also fixed seven stale literal pins in test files that the PostgreSQL rungs had been silently breaking since
V60, four of which are now asserted as the invariant the test's own name states rather than a literal.
Which component(s) does this affect?
PerformanceMonitor.Collectorsis shared with Lite, so Lite rebuilds — but no Lite behaviour changes. Litehas no PostgreSQL target, and the alerting deliberately rides a separate
IPostgresAlertReadAdapterconsulted only for PostgreSQL targets rather than extending the shared
IAlertReadAdapter, precisely so Liteis not left implementing three methods that can only return empty.
AlertEngineis untouched.How was this tested?
Please read this section rather than the checklist — the honest answer is more specific than the boxes.
Verified against live Amazon Aurora PostgreSQL 16.11 and 17.7 (stage):
(
pg_stat_io: 37 rows on 16.11, 25 on 17.7, carrying Aurora-specific enum values the community docs do notlist, e.g.
aurora cache receiver process).pg_replication_slots.inactive_sinceand the four autovacuum timestamps aretimestamptz, and Npgsql 10refuses to write a
DateTime(Kind=Utc)intotimestamp without time zone, so it would have failed atCOPY time in production. Every probed instance is
TimeZone=UTC, which is exactly what hides the wrongcast form.
pg_stat_iowrite side really is NULL, not zero — backends there do not write data files. Everycounter column is nullable on purpose and the read reports whether writes are TRACKED, so absent writes
cannot be misread as no writes.
VALUEStables substituted for the storetables, asserting the arithmetic: counter-reset clamping, NULL handling, single-sample → 0, and the ratio
ranking (a 10k-row table at 50× its threshold outranks a 50M-row table at 2.5×).
pg_autovacuum_statsis gated off standbys becausepg_stat_user_tablesreads fine on an Aurora reader andreports all zeros — measured on 17.7, same cluster and tables: writer 13,654,458 dead tuples, reader 0.
Ungated, a replica target would have produced a confident report of perfect autovacuum health.
Every migration rung was diffed against the ladder generator, since V1 is generated from the collector
catalog and a hand-written rung must match it column-for-column.
Not tested, and I would rather say so than have it found in review:
Darling.TestsandLite.Testshave never executed for this branch. They targetnet10.0-windowsandreference the WPF Viewer, so they compile on macOS and cannot run there. ~200 new xUnit assertions are
unexecuted. Local substitutes were used instead — the ladder-generator diff, standalone console harnesses
referencing the real projects, live SQL probes, and source-scanning reproductions of the reflection-based
MCP parity test and the
DocCommentHygieneTestsstacked-summary scan (the latter caught a real violationin this branch). CI is the first real run.
dispatch → binary COPY into a store → MCP read back out. The queries are proven; the wiring around them is
not.
docs/postgres-first-target-runbook.mdis the procedure for closing that gap, with a proof point atevery step, and it says plainly that it has not been executed.
dotnet build -c Debug→ 0 errors, 16 warnings, all pre-existing on dev (the one warning this branchintroduced, a CA1859 on a new field, is fixed).
Checklist
dotnet build -c Debug) — 0 errors and zero NEW warnings; 16pre-existing warnings remain, unchanged from dev. Not ticking a box I cannot honestly tick.
Aurora 16.11 and 17.7. No SQL Server behaviour changes; the shared-catalog changes are engine-gated.