Fixes #2800: document Lite's server-scoped watermark read as deliberately unbounded

[erikdarlingdata]

Fixes #2800.

Verdict: Lite does not have this defect, and the proposed bound would make the common Lite deployment slower. No behaviour change — the decision is recorded where the next person will look.

#2800 was a reasonable read of the code: GetLastCollectedTimeForDatabaseAsync takes a collectedSince bound and GetLastCollectedTimeAsync does not, which is exactly the asymmetry #2344 left on the Postgres side and #2795 later fixed there. The issue explicitly asked for Lite's own measurement first, citing #2762's revert as the precedent for not porting an optimization on another store's numbers. That measurement is below, and it says don't.

The #2795 failure mechanism cannot occur here

#2795 was not fundamentally about a slow query. It was about Npgsql's undocumented 30 s default CommandTimeout cancelling a 40,743–50,560 ms read, the cancellation being swallowed, and the resulting null being indistinguishable from a first run — so the collector silently fell back to its 60-minute window and re-collected what it already held.

DuckDBCommand.CommandTimeout defaults to 0, meaning no limit (verified against DuckDB.NET.Data 1.5.5). There is no ceiling to exceed and nothing to cancel. The silent-degradation path has no way to open.

And the bound does not pay

Measured on DuckDB 1.5.5 — the version Lite ships — against this table's shipped generated DDL (DuckDbSchemaGenerator.CreateTable + CreateIndex on the real QueryStoreCollector, not a retyped copy), using the shipped query strings. Connection already open, so the figure is query cost rather than connect cost (a SELECT 1 control put connection-open at ~0.1 ms warm, so it was not dominating).

At 50M rows / 4.7 GB, far past any realistic Lite store:

shape	server-scoped unbounded	server-scoped bounded	verdict
1 monitored server	0.34 ms	0.56 ms	bound is a 65% LOSS
5 monitored servers	8.26 ms	0.84 ms	9.8x, saving ~7 ms per 5-minute cycle

The single-server case is the common Lite deployment — it is a desktop tool. There, server_id selects everything, and DuckDB answers the unfiltered MAX straight from column zonemap metadata; adding a collection_time predicate forces real evaluation instead. The inversion is stable across repeated runs and holds under adversarial data.

For scale context, the whole curve (per-call, including DuckDB re-opening the file, as Lite does):

rows	file	unbounded cold	unbounded warm
1M	78 MB	3.3 ms	0.8 ms
5M	434 MB	5.1 ms	2.2 ms
10M	744 MB	7.0 ms	3.9 ms
50M	4.7 GB	39.5 ms	14.2 ms

Against Darling's 40,743–50,560 ms, that is roughly three orders of magnitude apart in absolute terms.

The per-database twin's bound IS earned, so the asymmetry is correct

The same harness, same store, same run:

shape	per-database unbounded	per-database bounded	ratio
1 server	8.40 ms	0.79 ms	10.6x
5 servers	10.42 ms	0.93 ms	11.2x

That bound pays in both shapes, because database_name is not in idx_query_store_time(server_id, collection_time), so that MAX genuinely has to scan and the collection_time predicate genuinely prunes.

So the two reads have different cost structures in a columnar engine. In Postgres both scanned every chunk, which is why the shapes match there and diverge here. #2344's per-database bound was right; extending it to the server-scoped read would not be.

What was varied, so the conclusion is not scale- or shape-dependent

• Scale — 1M, 5M, 10M, 50M rows. Roughly linear; no cliff.
• Payload width — narrow vs realistic query_text / query_plan_text. Identical (2.2 ms both at 5M), as expected when the query reads three narrow columns out of 60 in a columnar store.
• last_execution_time correlation — correlated with collection_time vs fully scattered. Scatter is the adversarial case for the unbounded form because its zonemaps stop being selective; it moved the multi-server number to 14.3 ms warm and left the single-server inversion intact.
• Server count — 1 vs 5.

One useful side-observation: in the scattered runs the bounded form returns a different, earlier watermark than the unbounded one. That is expected and is precisely why WatermarkPolicy.ReadFloor's bound is only sound behind the caller's clamp and the QueryStoreCollector name guard — the harness surfacing it independently is a small confirmation that the guard is load-bearing rather than decorative.

What lands

A doc comment on GetLastCollectedTimeAsync carrying the measurement and the reasoning, so this reads as a decision rather than an oversight. It names DuckDBCommand.CommandTimeout's default as the load-bearing assumption to re-check, since that is the single fact the whole argument rests on and a DuckDB.NET change could invalidate it.

No pin added, deliberately. A test asserting the method does not take a bound would forbid a future change that might become correct, and would be an assertion about today's cost curve wearing the costume of an invariant. The comment is the right vehicle; #2800 exists because the code did not explain itself.

Not addressed here

#2797 — skipping the server-scoped read entirely for collectors that discard its answer — is a genuine win in both engines and is unaffected by any of this. It remains the better fix for query_store specifically, as #2800 itself noted.

Verification: dotnet build Lite/PerformanceMonitorLite.csproj -p:EnableWindowsTargeting=true succeeded (the one warning is pre-existing, in an unrelated PerformanceMonitor.Ui file); global.json restored. The Windows-only suites cannot run on macOS, and this change adds no executable code for them to cover. CHANGELOG staged as 2 insertions / 0 deletions with a plain git add, so #2858's renormalization is holding.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

Reviewed. This is a documentation-only change (a doc comment on Lite/Services/RemoteCollectorService.cs:GetLastCollectedTimeAsync plus the matching CHANGELOG entry) — no SQL, no executable-code diff, so the T-SQL style rules and parity checks don't have anything to bite on here.

Spot-checked the factual claims against the current code rather than taking them on faith:

• Darling's GetLastCollectedTimeAsync (DarlingCollectorRunner.cs:1765) does take an optional collectedSince bound, while Lite's does not — confirms the asymmetry the comment describes.
• Lite's GetLastCollectedTimeForDatabaseAsync does accept collectedSince and is documented as earning its bound — consistent with "the per-database twin's bound IS earned and stays."
• The XML doc block is well-formed (<para>/<list> tags all balanced) and attaches to the right method.
• The new [#2800] reference link is inserted in the correct numeric position in the link list.

No parity drift: Darling's own doc comment on its bounded server-scoped read is untouched and still correct, so both sides remain internally consistent — Lite explains why it stays unbounded, Darling explains why its bound is earned.

Nothing to flag on correctness, security, or performance — there's no behavior change, and the perf numbers/reasoning in the comment are plausible and internally consistent with the DuckDB zonemap-pruning explanation given. No missing-index-DMV concerns apply since no queries changed.