Release 3.5.0 - #2335
Merged
Merged
Conversation
…-profile Refuse an install location the service account can never read (#2187)
The database-state alert baselines each database's first observed state as its accepted normal. A database swept into monitoring mid-restore therefore learned RESTORING as expected and deviated forever by being ONLINE: 636 alerts in 24 hours from 5 databases on the 52-server production fleet, with no escape but an operator re-baselining by hand. Two halves, both stores. The seed's refusal list widens from the integrity states to include the transient ones (RESTORING, RECOVERING) via a shared DatabaseStateTokens constant, so a database mid-operation stays pending and silent until it settles into a state worth learning. That governs rows that do not exist yet, so the second half applies the same rule after the fact: an AUTO-seeded baseline recording a state the seed would refuse is not a baseline anyone chose, and once the database's effective state reaches ONLINE the steady state is learned instead. Already-poisoned rows repair themselves on the next sweep with no migration, and "reset to current" pressed mid-restore -- which records whatever it sees with no filter, and always will -- now un-writes itself. What the heal does NOT touch is as load-bearing as what it does, since being wrong here means silence: - A user override. #2166's composition contract depends on it: a database parked at expected OFFLINE stays quiet while parked and still alerts when it comes back ONLINE. - An OFFLINE or STANDBY baseline, even though both are inferred. They are steady states, and leaving one is real news -- a STANDBY secondary that turns up truly ONLINE has stopped being a secondary, so log shipping is broken, and healing it would swap that alert for silence and then fire when the operator FIXED it. An auto-OFFLINE database brought up for an hour and re-parked would come back deviating forever. - The raw state_desc. A standby secondary reports state_desc = ONLINE with is_in_standby set, so matching that column would re-baseline every log-shipping secondary and alert it forever for being STANDBY. A NORECOVERY secondary, permanently RESTORING, stays silent as before; it now gets there by never being baselined rather than by baselining RESTORING. Tested watched-red across the transition matrix: onboard-during-restore then ONLINE, permanent RESTORING secondary, operator override wins, ONLINE to OFFLINE still fires, standby not healed, re-park stays quiet. The seed and heal are pinned live against real Postgres, and the two Darling copies of the SQL are pinned to share the one state list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
…eline # Conflicts: # CHANGELOG.md
…or-surfacing # Conflicts: # CHANGELOG.md
Two review findings, both real. Lite has the same orphan. The first cut declared this Darling-only on the grounds that Lite never sets CapturePlanXml and so writes no planwm: row. True, and irrelevant to two thirds of it: Lite's own backfill worker writes done: and hole: per database and only ever deletes a hole it SERVICES or expires, which a dropped database can never do. So its markers were kept forever in Lite's DuckDB too. Ported, with the key set moved into the shared QueryStorePerDatabaseState so a future prefix cannot be pruned on one SKU and orphan on the other. Lite runs the planwm: statement too, against rows it never writes: one no-op delete is what makes enabling plan capture there later a non-event rather than a thing to remember. The snapshot must be NEWER than the row it judges. Existing is not current: if database_states stops collecting, its newest snapshot freezes, and every database created after that instant is missing from it while being perfectly alive -- presence alone would prune those live rows on every cycle forever, paying a full plan-XML refetch each time and logging that a live database was dropped. A snapshot cannot judge a row written after it was taken. Both stamps are the service clock's naive UTC, so this holds whatever the two collectors' relative cadences are, and a genuinely dropped database is still pruned because its last write necessarily precedes any snapshot taken after the drop. Also from review: gate both call sites on the same AppliesTo that decides whether database_states is collected, so #2191's Azure boundary is stated rather than emergent; RETURNING the keys instead of a bare count, since a wrong delete's only other symptom is a silent refetch; a prefix-overlap case (App live, AppArchive dropped) that a starts_with anti-join would fail; a hole: survivor, not just a casualty; the call-site gate pinned in the window around the call rather than anywhere in the file; and a drift-guard message that names the trap -- adding a server-scoped key to PrunableKeys to silence it would have that key deleted every cycle. Live tests split into QueryStoreStatePruneLivePostgresTests per the shape LivePostgresCollectionHygieneTests asks for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… pass (#2186) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
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.
…line Never learn a transient state as a database's expected state (#2189)
…or-surfacing # Conflicts: # CHANGELOG.md
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).
# Conflicts: # CHANGELOG.md
…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.
Discover the query_store state classes instead of listing two of them. A hand-written pair made a THIRD state class invisible to the guard, which is the same silent-omission shape the guard exists to catch; a new *KeyPrefix const on any of them now fails until somebody decides which list it belongs in. Watched red by adding one. And assert the prune's log names the keys it retired. That line comes from the statement's RETURNING clause, so if RETURNING ever stopped yielding rows the deletes would still happen and the log would just go quiet -- invisible to every assertion about what the store contains, which is exactly the case the diagnostic exists for, since a wrong delete's only other symptom is a silent refetch. Watched red by logging the count without the keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r-surfacing Decode the loader status instead of printing a raw Win32 number (#2186)
# Conflicts: # CHANGELOG.md
…alf) The memory an edge trigger needs is persisted per database, Lite's config_database_state_expected had no such columns, and LiteAlertStateStore's two methods were documented no-ops. So alreadyAnnounced was always false in Lite and a database parked OFFLINE for a month still alerted every cooldown forever - the complaint #2166 was filed about, still live in the SKU most likely to hit it. Four pieces: - Schema v53: last_alerted_state + last_alerted_at, nullable, plus an ADD COLUMN IF NOT EXISTS migration so existing stores get them. NULL means never announced, which is what a first observation, a fresh store and a recovered database all look like - all three must be free to alert. - The two store methods write for real. Save is an UPDATE and never an upsert, the constraint #2166 established: an INSERT would have to invent expected_state (NOT NULL) from the state being alerted ON, so a database first observed SUSPECT would get SUSPECT as its accepted baseline, stop deviating, read as recovered while still corrupt, and never alert again. The seed refuses to baseline the integrity states for exactly that reason and this must not do it behind the seed's back. - The deviation read returns the value, so the shared engine can use it here. - A STORE-DERIVED recovered-clear running before the seed, which is the part worth reviewing. The engine also clears on the falling edge it witnesses, but that path is reachable only through an in-memory active set that empties on restart - so a restart landing between an alert and the recovery left the memory sticky forever and silently swallowed the next parking. Darling needed the same fix during #2182's review; porting the no-op without porting this would have shipped the bug I already fixed once. One sample at expected is enough where deviation needs two: clearing can only cause an extra alert, never a missed one, and a flap cannot exploit it because a flap never survives the two-sample test to alert at all. Five tests against a real in-process DuckDB, no live gate: the value round-trips to the deviation read; a recovered database has the memory cleared BY THE STORE with nothing held in memory (the restart-gap invariant); (ignore) clears it too, since a silenced database should not keep a memory outliving the silence; and the engine's immediate clear forgets the announcement while leaving the baseline intact - clearing the expected state instead would re-baseline and silence a real deviation. Deliberately unchanged: the integrity states keep repeating on the cooldown. One self-inflicted note: the schema comment initially used double quotes inside a C# verbatim string and broke the literal. Caught by the compiler, and I then scanned every file I touched for the same mistake. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lite used NOT IN where Darling used NOT EXISTS. They are not equivalent: one NULL anywhere in a NOT IN list makes the whole predicate NULL, so the prune would silently stop retiring anything. Fail-safe, and therefore exactly the kind of divergence that sits undetected for a release -- the two statements answer the same question, so they should answer it the same way. Pinned, in both directions, beside the two guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prune query_store per-database state for dropped databases (#2188)
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 live compression self-heal test failed one nightly out of 4,507 tests with Assert.DoesNotContain at line 1559, on a dev where nothing touching Timescale or compression had landed since the previous green nightly. WaitUntilDetectorReportsHealthyAsync returns the instant one poll finds the job unflagged. Leg (1) then issued two further queries and asserted health from a fresh read, so the assertion was not the observation the wait had validated -- and the test itself documents a state that lands in that gap: from scheduler pickup to completion, job_stats reads next_start = -infinity with status Running, which the detector flags and is right to flag. The helper now returns the flagged list from the poll that satisfied it, and the assertion reads that snapshot. Settled-according-to-the-wait and settled-according-to-the-assertion become the same observation by construction, which is what the helper's contract already claimed. Third appearance of this class: #1760 polled one detector arm (and the very value the caller had just written, so it never waited at all), the follow-up made the first observation deterministic and left the second racing. Both times the guarantee stopped where the helper returned and the caller reached past it. The observability assertion keeps its own read, which cannot race: StuckCompressionJobsSql filters on proc_name alone and returns every compression job whatever state it is in. Flagging is the C# predicate on top. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Review note: the DoesNotContain in leg (1) can no longer fail, since it reads the snapshot the wait already found clean. Kept for what it documents and as a guard if the helper stops returning the satisfying poll's result; the load-bearing check is the wait's bounded loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
…led-snapshot-2206 Assert compression-job health from the settled snapshot (#2206)
… files (#2197) Six independent copies of one sentence told every operator with a missing managed-store credential to "start the service once so its first run initializes the store". That is right for a genuine first run and a dead end for the case that actually produces it in the field: in #2185 the service HAD been started, its initdb had died in the Windows loader, and this message is what sent the reporter to darling.json. The five CLI verbs and the Viewer's managed-mode parse now share one message that decides between the two from evidence under or beside postgres.dataDirectory - an initialized cluster, a pg.log, or a credential file, in particular the store's own credential, which the service writes immediately before it runs initdb and which therefore survives the exact failure #2186 decodes. With evidence the message says this is not a first run, quotes what it found, and names the service log; without it the first-run advice is unchanged and gains the one sentence it was missing for an operator who has already started the service. Evidence is deliberately never a machine-global signal such as the service log directory existing: that is true of any box that has ever run the service, and would tell somebody standing up a second store that their bootstrap had failed - this same defect pointed somewhere new. An empty data directory an operator pre-created is not evidence either. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
--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.
…ge (#2197) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m 1) (#2287) The scale test fails intermittently on diffs that cannot reach it. The reading that reframed it: d1=689ms, d10=689ms -- BYTE-IDENTICAL. Two independent sub-second timings of different workloads do not land on the same millisecond by chance, so "runner jitter owns the constant factor" cannot be right, and that reading is exactly why the assertion was left as bare monotonicity. The obvious alternative is already ruled out in the helper: RunJobViaSchedulerAsync waits for last_successful_finish to ADVANCE, so each measurement is of a genuinely new completed run, not a stale row. What is left is that both runs did the same work -- plausibly near none, duration dominated by fixed overhead -- which a duration alone cannot show. So the failure message now carries chunk totals, compressed-chunk counts, total_runs, last_run_status and last_successful_finish for BOTH passes, and says outright that equal compressed-chunk counts mean the assertion was never measuring the #2136 capacity model: a fixture defect, not a tolerance one. No threshold changed. Guessing a tolerance is how an intermittent test stops looking broken without becoming correct; the next recurrence now diagnoses itself instead of costing a re-run. The describe helper is best-effort and cannot throw -- an explanation that fails would replace the failure it exists to explain, the #1902 mistake in miniature. Two dialect bugs caught before pushing: I wrote the T-SQL 'alias = expr' form (Postgres needs 'expr AS alias', and it would have failed only against a live server), and read total_runs as Int32 when the view is bigint -- which the defensive catch would have swallowed, taking the whole explanation with it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shipping statement carried TOP (50000) WITH TIES ... ORDER BY last_execution_time directly over the plan/query/text joins. A Top-N Sort carries every output column through the sort and reads all of its input before emitting a row, so choosing 50,000 rows materialized query_sql_text (nvarchar(max)) for the whole qualifying set. Neither knob that looks like it bounds this can, both measured on a 1,608-plan Azure SQL DB store: TOP (500) and TOP (50000) both cost 15.89s, and the client byte budget is flat across 4-256 MB because the server finishes before the client sees a byte. Time-to-first-row was 15.94s as shipped against 0.84s without the sort. The cap and the tie group now run against #pm_qs_slice, where every column is an int or a datetime, and the wide join runs from the chosen keys: 20.82s -> 4.81s on the same rows and the same 169.9 MB. The shipped row set is unchanged. The ORDER BY key comes off the slice in both forms and every join below the cap is one-row-per-slice-row, so nothing can multiply or reorder rows. WITH TIES still completes the boundary tie group, so #1960's derived-watermark invariant holds. One behavioural difference, stated rather than left to be found: the three inner joins can still drop a row whose plan Query Store evicted mid-batch, and that now happens after the cap instead of before it, so such a cycle ships slightly fewer rows than the cap. It cannot open a watermark hole, because the watermark is derived from rows actually stored. Verified by generating both the live and backfill SQL and asserting the row-choosing statement names no wide column and no TVF, that the cap, tie group, ship order and self-query marker are on it, and that the batch still returns exactly one rowset. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… (#2290) query_stats keys its deltas on the full dm_exec_query_stats row identity, which includes plan_handle -- and plan_handle changes on every recompile. So a statement whose dynamic SQL mints a new plan constantly presents a new key on nearly every sighting, and the first sighting of a key reports 0. Field measurement on a plan-churning readable secondary: a query Datadog measured at 43.1% / 43.9% / 42.5% of an 8-vCPU box across three windows read through these collectors as 18 executions and 2,824 ms over 168 hours. Instance-wide, the top-25 procedures accounted for ~49M ms against roughly 498M core-ms available. The worse half is that it was invisible. The counter-reset branch reports interval = 0 precisely so a reader can tell a fabricated zero from an idle one (#2234), but it needs the SAME key to reappear lower, and a recompile never does -- it arrives under a new key and takes the baseline path. Same class of harm as the 300-second gap policy #2233 replaced: it did not merely lose data, it invented quiet. The discriminator was already being collected: creation_time has been in this collector's SELECT all along. The row now also carries how long ago its plan was compiled, and when a series demonstrably began since the previous pass its whole counter accrued inside that window, so the delta is the full value rather than 0. No new collection, no schema change -- PayloadColumns is unchanged and the age is not stored. Sent as an AGE rather than creation_time itself because a DMV creation_time is in the monitored server's local time while collection times are UTC; comparing them client-side is a timezone bug on every server that is not UTC, so DATEDIFF is evaluated where both clocks are the same one. Stated rather than left to be discovered: a plan compiled AND evicted between two passes never appears in the DMV at all, so no keying scheme can recover it. plan_handle was deliberately NOT dropped from the key. Parameter-sensitive variants of one statement coexist in cache, so a statement-only key would collide across several live rows in a single pass and each delta would be computed against whichever row was processed last -- the multi-statement cross-contamination #2012 fixed, reintroduced from the other direction. The new entry point is default-implemented on the interface, so the other forty-odd delta call sites and every existing implementer are byte-identical. All eight of the row's counters take the rule together; crediting only some would make one row's metrics disagree about how much work it did. Verified by executing the new xunit facts in a plain net10.0 harness (Darling.Tests is net10.0-windows and cannot run on this machine): 11/11, including the gap-policy bound, the cold start, per-pass stability across every row of one sweep, ClearServer, per-server/per-collector isolation, and a pre-#2235 implementer still compiling. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add a watermarked query-text fetch seam, default off (#2150) The query_store payload selects query_sql_text (nvarchar(max)) inside a TOP ... WITH TIES ... ORDER BY last_execution_time. A Top-N Sort carries every output column through the sort and reads all of its input before emitting row one, so choosing the rows to ship materialized the text for the entire qualifying set. Measured with #2210's plan XML already gone and that one column as the only difference, on a purpose-built Azure SQL DB store: 1,505 rows / 12.8 MB text: ttfr 4.67s -> 0.45s drain 8.06s -> 0.50s 4,037 rows / 34 MB text: ttfr 5.02s -> 0.57s drain 16.95s -> 1.45s So #2210 did not finish this. It removed the larger column (195 KB average plan against 8.5 KB of text on that store) and left the one that still dominates. Neither knob bounds it, both measured: TOP (500) cost the same as TOP (50000), and wall time was flat from a 4 MB to a 256 MB client budget because the server finishes before the client sees a byte. This commit adds only the seam and changes no behaviour. The new FetchQueryTextSeparately flag defaults false and no host sets it yet, so the emitted SQL is byte-identical to before -- verified against the SQL captured from dev prior to the change, and pinned by a test that normalizes the one column out of both forms and requires the remainder to match exactly. It is a flag rather than a deletion because Lite stores that text inline in DuckDB and its grid reads it from there, so nulling the column unconditionally would blind Lite. Gated, the placeholder keeps the column's ORDINAL, which matters because the readers index the row by number. The fetch is watermarked rather than deduped per pass, a decision this collector already made once: #1556 shipped each plan once per PASS via ROW_NUMBER and #2164 replaced it because that form re-ships every pass forever, and drain is 94-97% of a pass. query_id is an identity, monotonic within a database, so a statement's text is fetched once ever -- and keying on query_id rather than query_text_id needs no new fact-table column and no migration, because query_id is already a stored payload column. Deliberately simpler than the plan fetch: no candidate-window estimator, because SUM(DATALENGTH(query_plan)) forces decompression of every plan in the window while query_sql_text has no such cost; and no content hash, because plan XML can be rewritten in place whereas a statement's text is fixed for the life of its id. The refresh horizon is kept for the hazard that does apply -- query_id is monotonic in first-seen order, not in "we have stored it", so a Query Store reset renumbers from the start. Verified by executing the new watermark facts in a plain net10.0 harness (Darling.Tests is net10.0-windows): 10 facts / 17 cases, plus direct assertions on the emitted SQL for flag-off byte-identity, ordinal stability, the fetch's shape, and all four stall-guard inputs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Give the new text watermark a prune verdict (#2150) CI caught this, and the guard that caught it is the point: adding a per-database state key prefix without classifying it is a new orphan class, and nothing else about adding one would fail. textwm: is keyed prefix + databaseName exactly like planwm:, so it goes in QueryStorePerDatabaseState.PrunableKeys and both hosts drop it when the database is dropped. Paired with its OWN collector name rather than the plan fetch's -- the two watermarks are stored separately on purpose, and a prefix pruned under the wrong owner silently deletes nothing, which looks exactly like having nothing to prune. Also updated the literal count pin in AzureForeignStatePruneTests from 3 to 4 and added the matching Contains, and named QueryStoreTextState in the drift guard's discovered-class assertions. The reflection already found it without being told; naming it means a rename that drops it out of the pattern fails instead of silently shrinking the set under test. Verified by replicating the guard's reflection locally against the built assembly: three state classes discovered, four prefixes declared (done:, hole:, planwm:, textwm:), every one with a verdict, count 4, NotKeyedByDatabase still empty, and KeyFor confirmed to be exactly prefix + databaseName -- which is what justifies PrunableKeys over the other list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2292) The storage half of the seam merged in #2291. collect.query_store_text is keyed (server_id, database_name, query_id), and query_id was chosen because it is ALREADY a stored fact column -- so this rung adds a table and alters nothing, readers get the join key for free, and no migration touches query_store_stats. Text is stored inline rather than as a digest into a content-addressed dimension. QueryStorePlanMap earns that machinery because plan XML is enormous and duplicated; Query Store has already de-duplicated text one row per statement per database, so there is nothing to squeeze -- and inline removes the dimension GC liveness interlock whose failure mode is silently missing text. The upsert overwrites the TEXT, not just the stamp. query_id is unique within a database only until Query Store is RESET, which renumbers from the start, so id 5 afterwards is a different statement than id 5 before. The refresh horizon brings us back to re-read it and this is where the corrected text lands; touching only last_seen would leave the old statement's text on the new id forever, which reads as a plausible wrong answer rather than as missing data. Pruned on last_seen rather than by drop_chunks (a keyed store, not a time series), bounded to one chunk-width of the oldest rows per call, with the retention margin ADDED to the fact horizon so text outlives the rows that reference it. SHIPPED INERT. FetchQueryTextSeparately is still false, because flipping it nulls the payload column while six reader surfaces still project query_text straight off query_store_stats -- the flip and the reader conversion have to land together or those surfaces silently lose text for new rows. They get their own reviewable change. What ships live: the fetch pass, its per-database watermark under its OWN state owner, and the Viewer probe. The state owner matters -- the load merges both owners, so writing the text watermark under the plan fetch's owner would read back fine and then never be pruned, because the shared prune set pairs textwm: with query_store_text and a prefix pruned under the wrong owner deletes nothing. The Viewer probe is the three-place edit: a probe column, a reader argument, and a map parameter. Verified in lockstep at 50/50/50 with contiguous ordinals 0..49 -- note the probe's raw "EXISTS (" count is 51 because one column is a compound EXISTS(...) OR EXISTS(...), so counting occurrences is a false failure. A probe that cannot SEE the newest object maps every fully-migrated store below the required version and the Viewer refuses to open. Darling.Tests BUILDS on macOS (it cannot run -- needs the Windows desktop runtime), so the whole suite is compile-verified here; the new facts run in CI. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…2293) Caught on the dogfood box: startup logged 42501: permission denied for table config_notification, then "MCP could not read the monitored-server registry -- live plan fetch will use darling.json". Two defects met. The least-privilege carve is correct and unchanged: DarlingManagedRoles deliberately REVOKEs table-wide SELECT on config_notification from BOTH viewer and mcp and re-grants only the non-secret columns, so the SMTP password and username and the Teams/Slack/generic/PagerDuty bearer URLs stay unreadable. The MCP host was asking for the whole row, and a column-level denial answers for the TABLE. The second defect is why one password cost the registry: every section of LoadViewAsync shares ONE try/catch, so the failed notification read discarded the four reads that had already succeeded. MCP therefore fell back to darling.json for live plan fetches -- a silent capability loss whose logged cause named a table MCP does not use. Fixed by making the reader agree with the boundary rather than by widening the grant: a caller that does not DELIVER alerts skips the notification row, and the MCP surface references neither Smtp nor Webhooks anywhere. Pinned three ways so they cannot drift apart again: the host passes includeNotification: false, the MCP surface is asserted to use neither type, and the notification SELECT is asserted to still name carved secret columns -- so narrowing that SELECT fails the guard and tells whoever did it that the skip became unnecessary. Not fixed here, and worth its own change: the all-or-nothing try/catch still means any one failing section discards the rest. The skip removes today's trigger, not the cascade. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…2294) Found on the dogfood box. The service logged: Failed to compute baselines for io_latency: Exception while reading from stream which points an investigation at the network. The store's own log, in a different file, showed 267 ms earlier: ERROR: canceling statement due to user request Npgsql enforces its command timeout by CANCELLING the statement, so the server reports the cancellation and the client is left holding a torn stream. The real cause was a query outgrowing its deadline on a store grown to 184 GB, and establishing that took correlating two logs by timestamp -- which is not a diagnosis the next person should have to repeat. Classified structurally (57014 query_canceled, or a TimeoutException anywhere in the chain) rather than by message text, since the message is the very thing that was ambiguous. The timeout arm also names the consequence the old line left implicit: the metric has NO baseline that pass, so its anomaly detection goes silent while the collected data looks perfectly healthy. Both paths now report elapsed seconds, so "it nearly made it" and "it never had a chance" are distinguishable. The other direction is pinned too: a genuine connection fault must keep saying so. Labelling one a timeout is the identical defect aimed the other way, and would send the next investigation at the query instead of the network. Extracted as an internal predicate so the classification is testable rather than buried in a catch. Verified by executing all seven cases -- including the negatives -- against the built assembly. Not changed: the timeout value itself. The right number wants measurement of what that query actually costs on a large store, not a guess; this makes the next occurrence say what it needs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eparate fetch on (#2150) (#2297) * Resolve Query Store text from collect.query_store_text and turn the separate fetch on (#2150) Flips FetchQueryTextSeparately for the sweep and converts every reader that projects query_text, in one change, because they cannot land apart: the flip nulls the payload's inline query_sql_text, so an unconverted reader would show blank text for newly collected rows while looking perfectly healthy. Six blocks across five files resolve collect.query_store_text first and fall back to the fact row's inline column, which is permanent rather than a migration step -- rows collected before the flip carry their text inline and nothing backfills them. Three of the queries also filter on the text (#1565's WAITFOR self-exclusion, the resolver's IS NOT NULL), so each resolves once inside its existing lateral or a derived table and the filter tests the resolved value under its original name; testing the raw column would have excluded every post-cutover row. The comparison read needed query_id projected through its dedup CTEs (it groups by query_hash, but text is keyed by query_id) and both arms converted, since its projection coalesces current over baseline. FetchRowsAsync stays off: it returns rows to its caller and writes nothing, so nulling the inline column there would lose the text rather than relocate it. Lite is untouched -- its DuckDB store has no side table. Verified against the live 52-server store, the only instrument that can see any of this (no CI job executes these Postgres strings). All six shipped bodies were extracted from source, planned with EXPLAIN (GENERIC_PLAN), then executed twice: with the side table empty each returned a byte-identical md5 to its pre-change form over 330 rows (1/50/50/179/50/1), proving the fallback arm and no fan-out; with the post-cutover condition induced by shadowing the table, all rows resolved from the side table at identical counts. * Teach the drill-down join guard about keyed side tables (#2150) DrillDown_AllSql_EveryFromJoinTarget_ResolvesToAV4ViewOrACollectorTable allowed three categories -- V4 passthrough views, collector TargetTables, and the query's own CTEs. collect.query_store_text is a fourth: a keyed side table written by a bespoke upsert path and pruned on last_seen rather than by drop_chunks, so it is in neither catalog, and the drill-down is the first analysis read to reference one now that statement text has to be resolved back. Sourced from QueryStoreTextStore.TableName rather than spelled in the test, so a rename cannot leave the guard asserting against a table that no longer exists. Verified the conversion introduces exactly one new FROM/JOIN target across all of PgDrillDownCollector.AllSql, which is this one.
…2298) (#2304) * Source the MCP host's server map from the worker's privileged load (#2298) #2293 fixed the first denied column and the failure moved to the next one: ReadMonitoredServersAsync selects encrypted_password, which the section-6 secret ACL deliberately SELECT-carves from mcp, so the MCP host's config view read still failed whole with 42501 and live plan fetch fell back to darling.json - on a seeded box, exactly the servers the file doesn't know. Skipping columns one 42501 at a time was chasing the carve. The durable agreement with the boundary: the MCP host performs NO config read of its own. The worker already loads the same rows privileged (it must, or it could not collect) and now publishes the effective server set through MonitoredServerRegistryState - the same publish/observe seam as the #1560 control-plane knobs. The plan-fetch resolver reads it PER FETCH: before the first publish it falls back to darling.json (the documented store-down posture) and heals on the next resolve; a server added via add_servers or the Viewer now reaches the resolver on the worker's next reload instead of never (the old map was built once at host start). Security property preserved, not weakened: the mcp DATABASE role's grants are untouched, no MCP tool exposes the state, and a token-holder still cannot obtain a stored credential. The carve was never about keeping credentials out of this process (the worker holds them); it keeps them off the MCP wire, which they remain. The #2293 pin moves with the fix: it now asserts the host contains no LoadViewAsync call at all and reads _registryState instead. New unit tests pin the seam's contract (null-before-publish -> file fallback, snapshot swap -> restart-free heal, first-wins duplicate ids); all four verified against the real build with a net10.0 harness on this machine since the Windows suite cannot run here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Leave a Debug breadcrumb for the pre-publish fallback window (review note) The permanent-failure WARN went away with the failing read; the transient pre-publish window gets one Debug line per host start instead - it is self-healing by design, so per-fetch logging would only be noise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Drop the includeNotification parameter with its last caller (review follow-up) The MCP host's #2293 skip was the only false ever passed; with its config read removed entirely, every remaining caller is privileged and the row is read unconditionally. The comment keeps the carve's history - it is the reason no restricted-role caller reads this view at all now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Say (re)start, not start, on the breadcrumb comment (review nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Bring the class doc forward to the #2298 fix it now pins (review nit) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ns grid (#2300) (#2303) * Cap the Index Analysis banners so they cannot evict the Recommendations grid (#2300) The reported empty grid held 2,078 rows the whole time. Six analyzer Note banners in an uncapped Auto row, plus the rollup grid, consumed the tab's full height at 1908x985 - the detail grid's star row collapsed to its header and horizontal scrollbar, so the rows had no viewport while the count indicator (computed from the same list) said 2078 recommendation(s). Two changes, one cause each way: - The banner strip moves into a ScrollViewer capped at 150px. Banners are caveats; they must never evict the content they qualify. Visibility now toggles on the scroller - the element occupying the layout row. - The detail grid gets MinHeight=120 as a backstop, so future growth in the rows above degrades to a shorter-but-alive grid, never a vanished one. Layout-only; no data path touched. Verified by building the Viewer - WPF layout is not unit-assertable here, and the repro arithmetic is in the issue thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Indent the #2300 comments to their nesting level (review nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…l dedup key (#2302) (#2310) * Give the generic webhook raw-JSON context tokens and the cross-channel dedup key (#2302) * Redact remediation T-SQL from context_json like every other channel (review find) * Validate the raw tokens against a quote-bearing stand-in context (review find) * Nudge CI after the outage dropped this branch's workflow events
… 15-minute stamp (#2312) (#2314) * Ship closed Query Store intervals every cycle, the open interval on a 15-minute stamp (#2312) * Bump the Azure foreign-prune count pin for the fifth per-database prefix (CI find) * Land the open-interval stamp only after the item's read and flush succeed (review find) * Treat an out-of-range numeric stamp as one more corrupt-row include (review find)
) * Wire the plan fetch's adaptive candidate sizing (#2312 Finding 1) * Average over only the plans that carried XML (review find)
…ck across gaps (#2324) (#2325) The #1944 NaN gap markers shipped first in 3.4.0 and collide with the gradient area fill. Reproduced headlessly against ScottPlot 5.1.59: one NaN in a FillY + ColorPositions series renders the ribbon as opaque black polygons with straight chord edges crossing the gap - the fill path closes its contours through the break, and its fill paint under ColorPositions is hardcoded Colors.Black with the gradient shader expected to paint over it, which a NaN-bearing series defeats. On a dark theme that black buried every other series on every gapped chart; the reporter's one healthy tab was the one whose data had no gaps, and 3.3.0 predates the markers - which is why 'previous versions' looked right. StyleScatter now withholds the fill from any series carrying gap markers (realYs.Count != pointCount): line-only, the break stays visible, nothing is buried. Continuous series keep the full gradient ribbon. Both directions pinned in ChartStyleGapFillTests so the fix cannot quietly repeal the fill feature. StyleScatter is the only fill producer (ChartHoverHelper only toggles what it captured), so no other path needs the guard. Verified with a three-way headless render harness against the shipped ScottPlot 5.1.59 on this machine: gap+fill reproduces the reporter's black polygons exactly (chord edges included), gapless+fill renders the intended ribbon, gap+line-only renders a clean broken line. The Windows suite runs the new pins in CI. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* V75: plan content gets its own retention horizon (#2316) The dimension GC's fact-coupled horizon guarantees no live fact ever references deleted content - and cannot bound a store younger than the fact retention. Measured on the dogfood fleet: query_plan_dim hit 127 GB (63% of the store) in its first 22 days at ~6 GB/day of param-sniffing recompile churn (344k distinct XMLs/day from 5,327 shapes; the worst shape produced 57,402 in one day), with the first coupled-GC delete mathematically impossible before ~Oct 27 - a month after projected disk-full. Orphan pruning existed and was healthy; compression was spent (every row already app-gzipped); lifetime was the remaining lever. config_service.plan_content_retention_days (V75, default 21, clamps [7,365], 0 = disabled = old behavior byte-for-byte): the dimension cutoff becomes the NEWER of the fact-coupled cutoff and now - (knob + 1), the same one-day margin as the measured floor for the same hourly last_seen refresh guard. Facts keep their full retention; a plan older than the window renders as the missing plan every reader already handles. A knob wider than the fact horizon is deliberately a no-op - it must not become a way to keep XML nothing can reference. Full rung recipe: Scripts entry + SchemaVersion 75, viewer probe sentinel/ordinal/newest-first arm, the V74 pin file demoted to its keeps-true-forever form (including its InvokeMap arity), and the knob plumbed file -> seed -> store view -> ApplyToConfig -> both PurgeAsync call sites (purge_now now receives the config it previously didn't need). Cutoff arithmetic and clamps verified against the real build with a net10.0 harness on this machine (10/10, including the field prediction: on the use2 box's shape the first post-upgrade sweep can delete immediately). The Windows suite and the live-PG job run the new pins, migration, and probe in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move the V75 doc block above V74's, not between V74's doc and its const The doc-hygiene guard caught the exact displaced-block trap it exists for: inserting the V75 summary+const anchored on V74Sql landed it BETWEEN V74's doc comment and V74's const - two summaries stacked on V75Sql and V74Sql left undocumented. Both jobs failed on this one test and nothing else. Verified locally with a reimplementation of the detector's rule: no stacked openings remain, and both consts have their own doc attached. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Scope the knob to the plan dimension, and teach the map's prune about it (review catches) Two review findings, both real: - The dedicated horizon applied to EVERY payload dimension, so the shipped default would have shortened query_text_dim too - breaking 'text stays analyzable for the facts' full retention', half the knob's own justification, to reclaim ~40 MB. A pure router (ComputeDimTableCutoff) now sends only query_plan_dim to the dedicated cutoff; every other dimension keeps the fact-coupled one. - The Query Store plan map's prune cutoff never learned the knob, so the dedicated dim cutoff overtook it: on the shipped default a 9-day window (69 days at 90d fact retention) existed where a dim row was prunable while the map row pointing at it survived - a live fact resolving to absent content, the exact silent-missing-plans failure the margin ordering exists to prevent, and the existing ordering pin never passed the knob so it kept passing. ComputeMapCutoff now folds the knob in with a one-day gap (map at knob, dim at knob+1 - the same stamp-skew margin as everywhere else, since TouchSql refreshes the map's stamp eagerly but the dim's hourly guard lets its stamp trail). Both cutoff components are strictly ordered, so the max-of-newer composition preserves the invariant under every knob value. New pins: the router's per-table scoping, map-disabled-equals-old-behavior, and the both-orders age sweep across (retention x knob) including the shipped default, clamp edges, disabled, and wider-than-facts. All verified against the real build via the local harness (16 pairs x full age sweeps, zero violations) plus the dogfood scenario end to end: plan dim reclaims at now-22, text dim untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clamp the knob at the destructive sink, and say what disabled actually does (review catches) The knob arrives pre-clamped only when a store read succeeded and ApplyToConfig ran. On a store-unreachable boot the worker passes darling.json's RAW value, and a file value of 1-6 would prune plan content below the [7,365] contract - the failure direction is data loss, so PurgeAsync now clamps first thing, exactly like retentionDaysFor's documented belt-and-suspenders clamp beneath it (the guarantee RunPurgeNowAsync's doc already relies on). Also tightened the ComputeDimensionCutoff comment: disabled takes an early return before any dedicated value exists - the old text described a MinValue-loses-the-comparison mechanism the code doesn't have. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Actually clamp at the destructive sink this time, and pin it (review catch, round two) The previous commit's message described this clamp and shipped only the comment beside it: the batch edit script applied its substitutions in-memory and wrote the file only after ALL anchors matched, so when the comment anchor failed its assert, the already-applied clamp edit was silently discarded with it - and the comment half was then re-applied by hand without noticing the loss. The reviewer caught the diff not matching the message. PurgeAsync now clamps planContentRetentionDays first thing, before either cutoff computation. And because the miss survived a green build (the clamp is unreachable by any test that can execute here), it now has a source pin - PurgeAsyncClampsTheKnobAtTheDestructiveSink asserts the clamp exists AND precedes both uses, and was proven to fail against the unclamped code before this landed. Also: [JsonPropertyName("planContentRetentionDays")] for serialization consistency with every sibling knob (review nit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…#2317) (#2327) * Give the self-metrics sweep a real timeout, and name a timeout as one (#2317) The sweep's ~5-a-day 'Exception while reading from stream' ERRORs were command timeouts in a network-fault costume - the #2294 misdirection one layer over. The managed server's own log confirms it at the exact failure timestamps: 'canceling statement due to user request' with CONTEXT: SQL function "hypertable_local_size", and pg_database_size cancelled alongside. All five sweep statements ran on Npgsql's default 30 seconds, which sizing a 141-object store with a 100+ GB plan dimension outgrows under load. - Every statement now carries SweepTimeoutSeconds = 300 (DarlingRetention's destructive-statement budget: an hourly sweep on its own connection can afford patience, and one that cannot finish in five minutes should skip the tick - a self-healing one-hour series gap - rather than retry into the same load, which is why the issue's retry-once suggestion is deliberately not taken). - The worker's catch classifies through PgBaselineProvider.IsCommandTimeout (one definition, InternalsVisibleTo extended to the Service, not a copy that drifts): a timeout logs as a timeout with the budget named; a real fault keeps the old message. - StoreSelfMetricsTimeoutTests pins statement-count == timeout-count so a sixth statement cannot ride the default back in, and pins the budget. The issue's 19:51 burst is separate (something SIGTERMed the TimescaleDB scheduler/launcher and two backends once - operational, evidence on the issue) and its FilterMutedFindingsAsync single line is on the post-V75 dogfood watch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cap the WHOLE sweep at one budget, and name the right sizing function (review catches) The sweep is awaited on the main loop, unlike the fire-and-track per-server sweeps - so five sequential 300s statement timeouts could stall per-server dispatch and the disk-pressure/compression checks for 25 minutes against a genuinely wedged store, hourly. The worker now caps the whole sweep at one SweepTimeoutSeconds through a linked CTS (worst-case loop block ~5 minutes, comparable to the old default's 5x30s), and the budget's own OperationCanceledException takes the timeout-named log arm when the service token is untripped. Per-statement timeouts stay as the belt for callers that pass no token. Also the naming nit, in all four places it appeared: the sweep calls hypertable_detailed_size - hypertable_local_size is its inner function and the frame the server log names when it cancels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… series (#2319) (#2328) * Collect per-database Query Store health (#2319): V76, both SKUs, both stores database_config knows one bit (is_query_store_on) - which cannot answer what #2312's investigation needed: is Query Store actually WORKING (desired READ_WRITE with actual READ_ONLY after the cap hit is the classic silent failure - readonly_reason says why), how close to the cap, and at what interval grain. The new query_store_health collector reads sys.database_query_store_options per database. Shape decisions, each pinned: - The database_scoped_config enumeration idiom verbatim (accessible ONLINE primaries, then [db].sys.sp_executesql per database) - the issue asked for the fields on database_config itself, but that collector is a single sys.databases scan and these fields need per-database context; a sibling enumerating collector preserves its execution model and failure isolation, and mirrors the family member that already exists. - Deliberately NOT filtered to QS-on databases: the options view answers one row even when Query Store is off, so OFF is recorded as OFF and an absent row can only mean 'not collected'. - Hourly, not the config family's on-load cadence: these values change BY THEMSELVES, and the cap-hit transition is the point. - All columns exist on 2016+; no version gates. Full recipe: V76 rung (body generated from PgSchemaGenerator so fresh and upgraded stores agree byte-for-byte, asserted in PgSchemaGeneratorTests), StorageVersion 76, viewer probe sentinel/ordinal 51/newest-first arm, V75 + V74 pin files demoted (including both InvokeMap arities), catalog + schedule registration, both hosts' dispatch, Lite golden schema + NOT NULL overlay + table-count pin, CI worker sizing 52/63 -> 53/64 in both workflows (the formula comments updated with them), Query Store sub-tab on both apps' Configuration tab (readers byte-identical over v_query_store_health; row classes verified identical), and QueryStoreHealthStoreTests pinning ladder/probe/filters/quoting/honesty/ schedule/payload order. Twelve definition facts additionally executed against the real build via a net10.0 harness on this machine. A get_query_store_health MCP read follows as its own PR - same slicing as pvs_stats (#1951), which shipped collector + viewer first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Register the enumerator and its view in the three guarded lists (CI catches) Two guards fired, both doing exactly their job: - The empty-enumeration inventory pins (#1852, both suites) compare the catalog's actual database enumerators against CollectorHealthClassifier.ExpectsUserDatabases - query_store_health now enumerates user databases, so its persistently-empty note must qualify against whether the target HAS any. Added to UserDatabaseEnumeratorNames and both suites' named sets. - The passthrough-view parity pin compares every CREATE OR REPLACE VIEW any migration emits against PgSchemaGenerator.AllPassthroughViews - V76 creates v_query_store_health, so the collector joins PostV8ViewCollectors and the V14 refresh-all idiom can never silently revert the view. Also the InitializeAsync_CreatesAllTables hand list in Lite.Tests, found by sweeping every file that hand-names the sibling enumerator rather than waiting for the next CI round to name it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Bump the golden oracle's table count with its new entry (CI catch) The oracle was extended by hand (as designed) for query_store_health; its 41-literal count is the frozen-shape half of the same pin: 42. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Gate on Query Store's existence, and decode readonly_reason as the bitmask it is (review catches) Three findings, all real: - sys.database_query_store_options does not exist before SQL Server 2016, so the ungated collector would have errored once per database per hour on pre-2016 targets. AppliesTo now carries QueryStoreCollector's exact condition (v13+ / version-unknown / either Azure flavor), so Lite and Darling skip identically; pinned across 2012/2014/2016/unknown/Azure, and the class doc's 'no version gates' claim is corrected to what it meant (no per-COLUMN gates within the view). - readonly_reason is a COMBINABLE bitmask - QueryStoreCollector already bit-tests it - and the display switch matched exact values only, losing every multi-bit state. Both row copies now decode bit by bit and join, reporting undocumented leftover bits numerically rather than guessing. - Two labels were written from memory and wrong: 131072 is the statement-count internal memory limit and 262144 the persist-backlog memory limit (no 'user request' reason exists). Fixed with the documented wording in both SKUs together. Gate facts additionally executed against the real build (16/16 in the local harness). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Type the two threshold columns as the bigint the DMV declares, and order the README rung table (review nits) stale_query_threshold_days and max_plans_per_query are bigint in sys.database_query_store_options, like every other numeric on the row - they were collected as int, harmless at realistic values but inconsistent and a latent OverflowException. Safe to change end-to-end (collector types, V76 rung, Lite golden, both row classes and readers) because no store has run V76 yet; the generated-vs-rung parity assertion follows the generator automatically. Also moved the V76 row in Darling/README.md's notable-rungs table to version order (it landed between V46 and V47). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * NumericCell on Lite's six numeric Query Store columns (review parity catch) Darling's twin tab right-aligns these six; Lite's rendered them left-aligned - the identical row data displayed inconsistently between the two apps. NumericCell is the established convention (~266 columns in this same file). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review catches: the definition-test file every collector carries, and the README collector table Every enumerating collector pairs its Darling store pins with a Lite.Tests definition-test file; query_store_health shipped without one, so ReadItemAsync's 9-column ordinal mapping and WritePayload's write order had no executable pin. QueryStoreHealthCollectorDefinitionTests mirrors the database_scoped_config sibling: enumeration exclusion splice + Azure variant, bracket doubling, the 2016+ gate, ReadItemAsync accumulation across two databases (healthy row + the cap-hit READ_ONLY shape with DBNulls through every coalesce arm), and the 10-column payload order via RecordingCollectorRowWriter. Root README's Lite Collectors table also learns the new collector and its count moves 41 -> 42. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review nit: two WritePayload comments still said INTEGER after the BIGINT widening Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…#2319) (#2329) * get_query_store_health: the MCP read for the new collector, both SKUs The follow-up #2319's changelog entry promised: one browsable tool beside get_database_scoped_config (whose latest-snapshot shape it mirrors), returning per-database actual vs desired state with the mismatch pre-folded into state_matches_desired, readonly_reason raw and decoded, storage vs cap with pct_of_cap, cleanup mode/thresholds, and the runtime-stats interval length. Darling reads through DarlingConfigHistoryReader over v_query_store_health and also exposes the /api/read endpoint; Lite reuses LocalDataService's existing grid read. The JSON emission is byte-identical across SKUs. The readonly_reason bit table now lives once, in PerformanceMonitor.Common (QueryStoreReadonlyReason): both viewers' grids and both MCP tools decode through it. The labels were miswritten from memory once during #2319 review - a single source is the fix, not care. Counting tools for the instructions doc surfaced that the census sentence had silently drifted: it said ninety tools while the server exposes one hundred, and its shared-bucket arithmetic summed to 74 against a stated 73. It is rewritten with digit counts (101 / 76 shared / 25 Darling-only, now including the PostgreSQL reads in the unique enumeration), and a new pin in CrossAppMcpToolInventoryPinTests parses the sentence against the scanned inventory so a new tool on either side fails CI until the census moves. Test pins: ConfigToolSurface 4 -> 5, param contract, QueryStoreHealthSql latest-snapshot + payload-order + PG-dialect pins, advertised-schema count, and the gated live-PG round-trip plants the cap-hit shape (desired READ_WRITE, actual READ_ONLY, reason 65536) and asserts the folded and decoded fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review nit: the Globalization using outlived the decode extraction Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review catches: DBNull parity on the state strings, and the README tool lists The Darling reader mapped actual_state/desired_state (and size_based_cleanup_mode) to null on DBNull where Lite's row defaults to "" - a type-level drift that would serialize different JSON across SKUs in the never-observed null case. The read row now coalesces to "" like both viewers' QueryStoreHealthRow. Root README's Available Tools Configuration row and Darling/README's Config bullet learn get_query_store_health, and the root README's 'Lite exposes 74 tools' count (stale before this PR - actual was 76) moves to 77. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… ranking explains (#2320) (#2330) * cpu_attribution on the top-CPU rankings: what fraction of the box the ranking explains The last unshipped item from #2235's wishlist, split out as #2320. get_top_queries_by_cpu and get_top_procedures_by_cpu (both SKUs) now return the returned rows' summed CPU-seconds, the SQL process's measured CPU-seconds for the same window (avg cpu_utilization % x core count x window - both stores already collect every piece), and attributed_cpu_ratio. Twice earned per the issue: pre-#2290 the reads explained ~10% of the box and nothing said so, and the ratio catches impossible claims at a glance - the Datadog comparison died when its worker_time sum divided out to 137% of the box's available CPU-seconds. Above the process's own measured consumption the note says to distrust the numbers; below half it explains where unattributable CPU goes (evictions between snapshots, rows outside the top-N or filters, zero-cost rows, non-query CPU). The degrade rule is explicit and pinned: missing CPU series, missing core count, or coverage under 90% of the window omits the ratio rather than inventing one. One computation in PerformanceMonitor.Common (CpuAttribution), decision-table tested identically in both test projects and executed against the built assembly in a local harness (all cases pass); the denominator read windows on collection_time with the same bounds as the rankings, so numerator and denominator share collection gaps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review catches: concurrent denominator reads, one shared window on Lite, honest Result doc The CPU aggregate and server-properties reads are independent, so both SKUs' tools now run them under Task.WhenAll instead of paying two sequential round-trips on every call. Lite's GetCpuWindowAggregateAsync takes the window explicitly and the tools capture one nowUtc backing both the aggregate read and the ratio math - Darling had that by construction, Lite sampled UtcNow three times for one disclosure. The Result record's doc no longer claims SqlCpuSecondsInWindow and AttributedCpuRatio are null together - the measured-zero case deliberately reports the zero, as the tests pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review catches: hoist Lite's nowUtc above the ranking read, and the %% doc artifacts The ranking read windows on its own internal UtcNow, so capturing nowUtc after it left the numerator and denominator skewed by the ranking query's duration. Hoisting the capture above the read shrinks the skew to call-entry overhead - zeroing it entirely would mean threading an instant into the shared ranking read's signature, which sub-microsecond drift against an hours window does not buy. The %% doc-comment artifacts (a template-escaping leftover) become %. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…he other SKU (#2181) (#2332) * Darling Viewer: the Query Store grid's DarkButton crash - Lite's #2114, on the other SKU The inline View Plan button in Queries -> Query Store by Duration referenced DarkButton, a key that IS defined in the Viewer - in MainWindow.xaml's window resources, a scope a UserControl's templates cannot see, because StaticResource resolves lexically at load rather than through the runtime tree. A missing key inside a cell template stack-overflows the process the moment the grid renders a row: uncatchable, no error dialog. It survived dogfooding because an EMPTY grid never applies its cell template, and the dogfood fleet's Query Store data is empty. #2181 reported it against the Darling Viewer and was closed as a duplicate of the Lite-only fix (#2118) on a wrong premise; #2331 re-proved it on 3.4.0 stable, which the Lite fix missed by twelve hours anyway. The button uses default chrome now - Lite's exact fix. And the hygiene test that let this through gets the model it should have had: per-FILE resolution (own keys + transitively merged dictionaries + App.xaml scope), matching WPF's actual StaticResource lookup instead of the per-app existence check that declared MainWindow's key 'defined somewhere, good enough'. The widened scan was proven exact before adoption: over both apps it flagged exactly this one real crash and zero false positives. Closes #2181 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Review nit: the pack-path doc claimed a repo-layout fallback the code does not have Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…2243) (#2334) Versions move to 3.5.0 in the three live csprojs plus the deprecated Dashboard csproj the dev->main version check reads. The Unreleased section cuts to [3.5.0] - 2026-08-19. And #2243 rides the window it was parked for: the review prompt pointed every run at a CLAUDE.md that exists on no branch (it is gitignored local tooling), so its first instruction was to follow a file it cannot open - it now points at CONTRIBUTING.md and names the T-SQL style rules inline instead of dangling a reference to a guide that is not there. Closes #2243 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
The 3.5.0 release PR — dev → main.
Why now: 3.4.0 stable has carried a crash-on-click in BOTH viewers' Query Store by Duration grids for twelve days (three reporters: #2114, #2181, #2331); the Lite fix missed the 3.4.0 cut by twelve hours and the Darling copy was only diagnosed this week.
What ships (highlights; CHANGELOG [3.5.0] has the full detail):
query_store_healthcollector +get_query_store_healthMCP read, both SKUs (database_config reports query_store as a boolean — collect its health (actual_state, size vs cap, read-only reason) so a thrashing store is visible #2319) — found the fleet-wide stuck-READ_ONLY Query Store condition within its first hour on the dogfood boxcpu_attributionon the top-CPU rankings, both SKUs (Report unattributed CPU on the top-queries reads — ranked-query CPU vs the box's measured CPU-seconds #2320/cpu_attribution on the top-CPU rankings: what fraction of the box the ranking explains (#2320) #2330)Soak evidence: the exact code in this PR (minus version stamps) ran overnight on the 42-server dogfood box: 42/42 collecting, zero service-fault errors (the only ERRORs were one target-side maintenance window, fully recovered), zero self-metrics failures, query_store_health at exact hourly cadence fleet-wide, cpu_attribution verified live, clean stops verified across three restart cycles.
After the tag: #2333 (the activity-driven plan fetch) merges into the next cycle.
🤖 Generated with Claude Code