Bound the per-database watermark read to the clamp horizon (#2344)

[erikdarlingdata]

Closes #2344. The first of Erik's "further juice to squeeze before releasing" — and it is the phase that survives catch-up, so it is the one that follows an operator home.

What the split showed

After #2333 removed the plan-fetch catalog walk, the per-database log line reads:

query_store [AYR]  => sql:15594ms = wm:238ms + open:1270ms + drain:31ms + plan_fetch:13444ms + text_fetch:611ms
query_store [nectar] => sql:7431ms = wm:3134ms + open:566ms + drain:23ms + plan_fetch:2672ms + text_fetch:1036ms

plan_fetch is catch-up work and drains to near-nothing by design. wm does not. And wm is not a query against the monitored SQL Server — it is a read against our own store.

Measured, live use1 store (106 GB, 5 chunks, 42 primaries)

Same server/database pair, EXPLAIN (ANALYZE, BUFFERS):

	unbounded (today)	bounded to 3h
cold (first touch)	25,766 buffer reads + 195 written (temp spill)	5 chunks excluded, 29 ms
warm (cache primed)	228 ms	29.5 ms

Warm is 7.7x. The cold row is the one that matters in the field: the cost is a function of store size and cache residency, not of the monitored workload — so it is worst on a long-lived store, a busier Query Store, slower disks, or a box whose cache is not dominated by this one query. Our dogfood store is five chunks; a 90-day-retention store is many more.

Why the bound is free rather than a tradeoff

Every consumer of this value ends at max(stored, now - MaxCatchup):

• MaxCatchup is 1 hour; ClampCatchup floors anything older to now - 1h.
• A null result falls back to query_store's documented 60-minute first-run window — the same instant.

So a row older than the horizon cannot change the outcome whether it is found or not, and the unbounded MAX was paying to confirm a value the clamp would have produced anyway. WatermarkPolicyTests pins both halves: the floor sits strictly older than the horizon (the ordering is the safety argument), and a buried watermark and a not-found one reach the identical instant.

Two mechanical details that are easy to get wrong:

• The predicate goes on collection_time (the partitioning column). A predicate on the watermark column alone prunes no chunks. It is safe because a row's watermark value can never exceed its own collection_time — an execution cannot be collected before it happens — so nothing qualifying hides behind the bound.
• The bound is opt-in per caller and name-guarded to query_store, on both hosts. A ring-buffer collector whose legitimate catch-up spans days (default_trace_events) must keep reading its whole history; handing it this floor would silently truncate that. The clamp and the bound travel together — a future definition wanting one has to adopt the other, which the policy remarks say explicitly.
• The server-scoped GetLastCollectedTimeAsync is deliberately untouched: that path does not clamp, so the equivalence does not hold there.

Parity and verification

Both SKUs, same shape (Postgres gets chunk exclusion; DuckDB gets min-max pruning and a smaller scan — different mechanism, same predicate and same argument). All four affected projects build.

The policy arithmetic was executed against the built assembly in a throwaway net10.0 harness — which caught a bug in my own test fixture (an InlineData(2) case asserting a 2-hour-old watermark sits below a 3-hour floor; it does not). That would have been a red CI round. Fixed to 4h before pushing. The Windows test projects build here but cannot run, so the harness is the only pre-CI execution available.

Not yet measured end-to-end in production: the before/after above is the query in isolation via EXPLAIN, not the wm phase in the running service. That lands when a nightly carrying this installs, and the log split will show it directly.

🤖 Generated with Claude Code

[claude]

Nothing here exercises this new conditional SQL text against a live DuckDB connection with collectedSince non-null — WatermarkPolicyTests only covers the pure ReadFloor/ClampCatchup arithmetic, and there's no test in Lite.Tests (e.g. via SharedDuckDbFixture) that actually calls GetLastCollectedTimeForDatabaseAsync with a floor and checks the $3-param query executes and returns the right row. The Darling twin (DarlingCollectorRunner.GetLastCollectedTimeForDatabaseAsync) has the same gap on the Postgres side. The reasoning behind why the bound is safe is thoroughly tested, but the SQL wiring itself (conditional predicate + conditional positional parameter, on two different drivers) is only validated by the PR author's manual EXPLAIN run against production — worth a small integration test on each host so a future refactor of this method doesn't silently break the $3 binding.

[claude]

Reviewed the diff against dev. This is a tightly-scoped, well-reasoned change and I couldn't find a correctness or parity bug in it.

What it does: bounds GetLastCollectedTimeForDatabaseAsync's MAX(watermark) read on collection_time (the partitioning column) for query_store only, via WatermarkPolicy.ReadFloor. The safety argument — every consumer of a clamped watermark ends at max(stored, now - MaxCatchup), and a row below now - MaxCatchup - ReadFloorMargin can't change that outcome whether found or not — checks out, and watermark <= collection_time holds for every table this touches (PrefixTimeColumnName defaults to collection_time and QueryStoreCollector doesn't override it).

Verified:

• Both hosts are byte-for-byte symmetric: the same azureReadFloor/readFloor name-guard (definition.Name == QueryStoreCollector.Instance.Name), same call-site shape, same doc-comment reasoning, in both DarlingCollectorRunner.cs and RemoteCollectorService.{cs,DefinitionRunner.cs}.
• The name-guard correctly excludes the other three PerDatabaseWatermarkColumn collectors (BlockedProcessReportCollector, DeadlocksCollector, LongQueryCompletionsCollector) — the ring-buffer sources the doc comments say must keep reading unbounded history.
• Positional-parameter wiring ($1/$2/$3) is consistent between the SQL text and the parameters added in both the Postgres (Npgsql) and DuckDB paths; DateTime.SpecifyKind(..., Unspecified) on the Postgres side matches this store's existing naive-timestamp convention (default_trace_events: read the current trace file, fall back on rollover (#1962) #1969).
• WatermarkPolicyTests correctly pins ReadFloor strictly older than the clamp horizon and the found-vs-not-found equivalence the whole argument rests on.
• The bundled StoreSelfMetricsTests scale-test rewrite (replacing the flaky d10 > d1 duration assertion with exact chunk/row-count assertions) is unrelated to the watermark bound but is a solid fix in its own right, with a negative control proving the new assertion catches what the old one couldn't.

One gap worth a look (left as an inline comment): there's no integration test that exercises the new collection_time > $3 SQL path against a live DuckDB or Postgres connection — only the pure WatermarkPolicy arithmetic is unit-tested. The PR body notes this was validated manually via EXPLAIN against production, but an automated test (e.g. via the existing SharedDuckDbFixture) would guard the parameter wiring itself against regression.

No T-SQL in this diff (it's the C# collector-runner layer), no missing-index suggestions, no security concerns — table/column names interpolated into the SQL text are internal constants (TargetTable/WatermarkColumn), not user input, consistent with the pre-existing pattern.

[erikdarlingdata]

Force-pushed a rebase onto clean origin/dev. The previous push carried an unrelated commit: three agents were working the same checkout concurrently and one of them briefly landed #2266's StoreSelfMetricsTests.cs change on local dev, which this branch was then cut from — so the diff showed a test file this PR has nothing to do with, plus that PR's CHANGELOG entry. Both are gone; the diff is now the six files this change actually touches. origin/dev was never affected, and #2266's own work is intact on its branch (#2345).

[claude]

This removes the [#2266]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2266 link-reference definition, but [#2266] is still referenced three times elsewhere in this file (lines ~30, ~38, ~46 in the new version, in the entries for the scale-test reporting fix, the skipped-maintenance-cycle log line, and the heal-test flakiness fix). With the definition gone, those three occurrences will render as literal [#2266] text instead of links to the issue. Looks like an unintended deletion — please restore the line (or drop it back in alongside the [#2344] addition) rather than removing it.

[claude]

Reviewed the watermark-read bounding change (Darling/Postgres and Lite/DuckDB GetLastCollectedTimeForDatabaseAsync, WatermarkPolicy.ReadFloor, and both call sites in each host's enumerated + Azure per-database arms).

Correctness of the bound itself: sound. The read floor (now - MaxCatchup - ReadFloorMargin, 3h total) sits strictly below the clamp horizon (1h), and the invariant it leans on — a row's watermark (last_execution_time) can never exceed its own collection_time — holds because collection_time is stamped by the collector after the row is observed. The 2h ReadFloorMargin is a reasonable cushion for clock disagreement between the monitored server and the store on top of that. The predicate lands on collection_time, the actual partitioning column, not the watermark column, which is the detail that makes it prune anything. Confirmed query_store_stats.collection_time exists and is already filtered elsewhere in the Lite store (RemoteCollectorService.QueryStoreBackfill.cs), so the new predicate isn't referencing a phantom column.

Lite/Darling parity: clean. Both hosts add the identical azureReadFloor/readFloor name-guard (scoped to QueryStoreCollector.Instance.Name only) at both call sites (the Azure per-database branch and the enumerated perItemWatermark delegate), both thread collectionTime/context.CollectionTime consistently into ReadFloor and the later ClampCatchup call, and both GetLastCollectedTimeForDatabaseAsync overloads add the optional collectedSince parameter the same way, including the pre-existing Postgres naive-timestamp convention (DateTime.SpecifyKind(..., Unspecified), #1969) vs. DuckDB not needing it.

One concrete bug — CHANGELOG.md: this PR deletes the [#2266]: https://... link-reference definition while [#2266] is still used three times elsewhere in the file. Those three entries will now render as literal [#2266] text instead of links. Left an inline comment on the removed line.

No T-SQL collector code is touched here (this is a Postgres/DuckDB store-read change, not a monitored-server query), so the T-SQL style rules don't really apply, and I didn't see any missing-index-DMV suggestions to flag either way.

[claude]

Review

Solid change overall - the bound is on the correct (partitioning) column, the Postgres-vs-DuckDB Kind handling for the collectedSince param follows the established #1969 convention on the Darling side, and Lite/Darling stay in lockstep across all four touched call sites. WatermarkPolicyTests pins the boundary math well.

One correctness issue worth fixing before merge, left as inline comments on the four call sites (Darling's Azure + enumerated arms, Lite's Azure + enumerated arms):

The ReadFloor safety argument does not extend to the backfill-hole bookkeeping. ReadFloor's doc (and the PR description) proves the bound is free for the final clamped instant - max(stored, now - MaxCatchup) is the same whether the stored row is found or not. That is true for the value that ends up in the live query's cutoff. But the adaptive-shrink code immediately below each read site uses the raw watermark for a second purpose: deciding whether to call RecordQueryStoreBackfillHoleAsync(..., rawWatermark, ...), which persists a hole record for the trickle-in backfill worker. When the true stored watermark is older than the read floor (now - 3h, i.e. a database that has been failing for a while, or - the original #1556 field scenario - a service that just came back from being down for days), the bounded read now returns null instead of the true stale timestamp. That routes into the 'never-succeeded' branch, which explicitly skips hole recording ('pre-watermark history is the tail's job') and also skips the operator-visible clamp WARNING. So for exactly the outage-recovery case this whole watermark/backfill system exists to handle, the real gap silently stops being recorded or logged after this change - where before the PR, the unbounded read would have found the true raw value and recorded/logged it correctly.

The live query's own cutoff is unaffected (it still resolves to the same now - 1h either way), so this does not reproduce as an incorrect query - it reproduces as a silently dropped backfill hole and missing WARNING on multi-hour+ outages, which is harder to notice.

Also flagged a minor DuckDB/Postgres asymmetry on the DateTimeKind handling of the new parameter - low confidence, may not matter functionally for DuckDB, but worth a look since the codebase has an explicit convention for it elsewhere.

[claude]

Bounding the read here can silently drop a real backfill hole. ReadFloor's "found vs. not-found reach the same clamped instant" argument only covers the value that flows into BuildCutoffParameters/ClampCatchup. But the adaptive-shrink block right below this (lines ~449-475) also consumes the raw watermark for a different purpose: if context.Watermark is DateTime azureRaw is true, a stale azureRaw gets recorded via RecordQueryStoreBackfillHoleAsync(..., azureRaw, tighterFloor, ...) so the backfill worker knows to trickle in the real gap.

If the true stored watermark is older than ReadFloor (now - 3h) — e.g. this database has been failing for a while, or the service just came back from being down for days (the original #1556 scenario) — the bounded read now returns null where it used to return the true stale timestamp. That flips execution into the else branch at line ~466, which explicitly records no hole ("pre-watermark history is the tail's job") and skips the clamp WARNING too. So for the exact outage-recovery case this whole system was built for, the real historical gap now goes unrecorded and unlogged, whereas before this PR it would have been correctly captured.

Worth considering: either read the raw watermark unbounded specifically for this hole-recording path, or explicitly detect "found nothing because it's outside the read floor" vs. "genuinely never collected" so the hole can still be recorded (even if only approximately, e.g. from the read floor itself).

[claude]

Same issue as the Azure arm above, on this enumerated path: with the bound in place, raw comes back null instead of a true-but-stale watermark once it's older than now - 3h. That skips the raw.HasValue && clamped != raw block entirely (line ~767) — no WARNING, no RecordQueryStoreBackfillHoleAsync call for the real gap — and then the adaptive-shrink block below (line ~793) falls into the "never-succeeded" branch, which also records no hole by design. A multi-hour+ outage's history gap now goes unrecorded/unlogged where the unbounded read used to catch it correctly.

[claude]

Lite's twin of the same issue flagged on Darling's Azure arm: ReadFloor's safety argument covers the clamped instant, not the raw watermark that the adaptive-shrink block below (line ~264 onward) uses to decide whether/how to call RecordQueryStoreBackfillHoleAsync. Once the true stored watermark predates the read floor (now - 3h), the bounded read returns null, the code takes the "never-succeeded" branch at line ~281, and the real gap goes unrecorded with no WARNING — for exactly the extended-outage scenario this backfill machinery exists to handle.

[claude]

Same issue on the enumerated arm: once the true stored watermark is older than the read floor, raw comes back null, skipping both the WARNING/hole-record at line ~578 and falling into the no-hole "never-succeeded" branch of the adaptive-shrink block at line ~614 — silently dropping the real gap for a database that's been failing/down for more than ~3 hours.

[claude]

Minor asymmetry vs. the Darling/Postgres twin: there, floor is explicitly re-marked DateTime.SpecifyKind(floor, DateTimeKind.Unspecified) before binding, citing the #1969 trap (Npgsql infers timestamptz from Kind.Utc and shifts the value). Here floor (Kind=Utc, from collectionTime = DateTime.UtcNow) is bound as-is. There's existing precedent in this codebase for stripping Kind before a DuckDB parameter used in a collection_time comparison (LocalDataService.FinOps.Pvs.cs:111), so this may just be an inconsistency in following that convention rather than a functional bug (DuckDB.NET's binding may be Kind-agnostic, unlike Npgsql) — worth a quick check either way for consistency with the rest of the codebase.

[erikdarlingdata]

Reviewed the diff and merging. This is the one of the three I care most about, because it is the only Query Store cost that does not subside after catch-up — it is our own store's size, not the monitored server's workload.

Fresh numbers from use1 that make the case, 183 runs over the last 30 minutes post-catch-up:

	before the activity-driven fetch	now	after this
avg	46.2s	7.7s	—
median	40.8s	6.6s	—
p95	104s	17.5s	—
max	313s	32.5s	—

The worst live sample splits as sql:9346ms = wm:3894ms + open:1230ms + drain:124ms + plan_fetch:1952ms + text_fetch:2146ms. wm is 42% of that server's total, and the other phases are still shrinking as the backlog drains while wm is not — it scales with store size and cache residency. A 106 GB store on modest disk is exactly where an operator is least able to absorb it, which is the release-sensitivity concern.

What I verified rather than took on faith:

• The floor sits strictly below the clamp horizon. now - MaxCatchup - ReadFloorMargin with a 2h margin against a 1h horizon, so a watermark that could still move the answer can never fall below the bound. The margin exists only for clock disagreement between a monitored server and the store; the correctness argument needs none of it, which is why it is generous rather than tuned.
• The equivalence is real, not approximate. Every consumer lands on max(stored, now - MaxCatchup): ClampCatchup floors anything older, and NULL falls back to the documented 60-minute first-run window — the same instant. So a row below the floor cannot change the result whether found or not. The InlineData(4/6/48/2160) test asserts exactly that: a watermark below the floor clamps to the same instant as finding nothing at all.
• It bounds the PARTITIONING column. The predicate is on collection_time, not the watermark column, or TimescaleDB prunes nothing. Safe because a row's watermark can never exceed its own collection_time — an execution cannot be collected before it happens.
• It is scoped to exactly one collector. Guarded on QueryStoreCollector.Instance.Name, and the server-scoped GetLastCollectedTimeAsync is deliberately untouched. Handing this floor to a ring-buffer collector whose legitimate catch-up spans days would silently truncate it. The bound and the clamp travel together, and a future adopter has to take both.
• Both stores got it. Postgres and DuckDB, same shape, or Lite reads a permanently different value.

Prediction I will check on the box after the nightly: wm on the worst use1 server drops from 1–3s to ~30ms, and that server's total falls from ~9.3s to ~5.5s.

[erikdarlingdata]

Rebased onto dev — #2343 landed a ### Fixed entry in the same spot, so this went CONFLICTING the moment that merged. Resolution keeps both entries in issue order; the code diff is untouched (6 files, same 156/11).

While resolving I also took the cosmetic nit the review bot raised on #2343: the double blank line before the ## [3.5.0] heading. It was created by that merge and every other version boundary in the file uses a single blank, so it belonged with the conflict resolution rather than in a PR of its own.

Worth flagging for anyone watching CI here: a CONFLICTING PR silently stops GitHub running pull_request-triggered workflows. The checks do not fail, they simply never queue, and the last green run stays on the page looking authoritative. That trap already cost a round earlier today across three PRs. If checks look stale, compare the run SHA against the head SHA before believing them.

[claude]

Review

Reviewed the diff (CHANGELOG, WatermarkPolicy.cs, both hosts' DefinitionRunner/CollectorRunner, and GetLastCollectedTimeForDatabaseAsync in both the Postgres/Darling and DuckDB/Lite stores).

Correctness — checked out. The core claim ("bounding the read changes no caller's outcome") rests on the invariant that a row's watermark column can never exceed its own collection_time; given that, excluding rows with collection_time <= floor can only exclude rows whose watermark was already going to be clamped to the horizon, so the bound is a no-op on the answer. The one place this invariant could theoretically slip — clock skew between the monitored server (source of last_execution_time) and the collector host (source of collection_time) — is already covered by the 2h ReadFloorMargin on top of the 1h MaxCatchup, so the "provably free" framing holds with the stated margin.

Verified the more subtle claim in the Azure-arm comments — that the per-database branch doesn't clamp itself but is safe because QueryStoreCollector.BuildCutoffParameters (PerformanceMonitor.Collectors/QueryStoreCollector.cs:1072) clamps context.Watermark before it's used — and that's accurate; BuildCutoffParameters does call WatermarkPolicy.ClampCatchup unconditionally.

Lite/Darling parity — good. Both GetLastCollectedTimeForDatabaseAsync overloads (Postgres in DarlingCollectorRunner.cs, DuckDB in RemoteCollectorService.cs) got the same collectedSince treatment with matching predicate shape, and both hosts' two call sites (enumerated arm + Azure per-database arm) apply the same QueryStoreCollector.Instance.Name guard. The Postgres path does DateTime.SpecifyKind(floor, DateTimeKind.Unspecified) before binding while the DuckDB path doesn't — that's not a drift bug, it matches this file's existing convention (other DateTime params here, e.g. collectionTime at RemoteCollectorService.DefinitionRunner.cs:886, are also passed to DuckDB without a Kind conversion; only the Postgres/Npgsql path needs it per #1969). WatermarkPolicy.cs itself is shared code in PerformanceMonitor.Collectors, so testing it once in Lite.Tests (no Darling.Tests mirror) isn't a gap — that's the pre-existing pattern for shared-project logic.

Tests — the new WatermarkPolicyTests cases correctly encode the "found late == not found" equivalence and pin the floor arithmetic (Now - MaxCatchup - ReadFloorMargin). Nice catch mentioned in the PR description (the InlineData(2) fixture bug) — the shipped InlineData values (4, 6, 48, 2160) are all safely past the 3h floor.

Security/perf — no new user-controlled input reaches the interpolated columnName/tableName/databaseColumnName (those come from static collector definitions, unchanged from before); the new bound is parameterized correctly and only adds a third positional parameter when present. No missing-index recommendations here, nothing to flag on that front.

No blocking issues found.