De-skew query_stats.creation_time before comparing it to the analysis window (Part of #2991)

[erikdarlingdata]

Part of #2991.

query_stats.creation_time is the monitored server's local wall clock — QueryStatsCollector ships the sys.dm_exec_query_stats value verbatim, and says so about this very column. The bound it was compared against is naive UTC off DateTime.UtcNow. Six analysis reads across the two stores compared them untranslated, so the PARAMETER_SENSITIVITY detector's compiled-before-the-window guard asked a different question on every server that is not UTC.

The direction, re-derived

utc_offset_minutes is DATEDIFF(MINUTE, GETUTCDATE(), GETDATE()), i.e. local - utc, so a plan compiled at UTC instant T is stored as creation_time = T + offset. The old predicate creation_time <= W therefore admitted T <= W - offset, where the correct bound is T <= W.

Measured against real PostgreSQL 17 and real DuckDB 1.5.5, on a five-plan fixture straddling the window bound, default four-hour window:

server offset	old predicate selects	correct
UTC-12	all 5	3
UTC-4 (the production fleet)	all 5	3
UTC (a dev box)	3	3
UTC+10	1	3
UTC+14	1	3

So a negative offset is the false-positive direction: it admits every plan compiled inside the window, which is exactly the population the predicate exists to exclude, and a young plan's partial-life min/max worker-time spread is then scored as full-life variance. A positive offset is the suppression direction. One correction to the issue's wording: at a positive offset the finding class is sharply thinned, not silenced — a plan cached longer than the offset still qualifies, which is why old_3d survives at UTC+10 above. Zero was the only offset that was ever right, which is why every store anyone develops or tests against agreed with the bug.

The fix

Each site resolves the collected server_properties.utc_offset_minutes in a single-row COALESCE CTE and compares a de-skewed creation_time_utc. Subtracting the offset is the direction already established at the three existing de-skew sites (ViewerDataService.SystemEvents, ViewerDataService.JobHistory, and PgFindingStore's - context.ServerUtcOffset).

No parameter was added: server_id is already $1, so the ordinals and the C# bind order are unchanged.

The two dialects diverge, and have to. PostgreSQL gets make_interval(mins => svr.offset_minutes); DuckDB has no make_interval, and AT TIME ZONE would pull in ICU, so Lite multiplies INTERVAL '1' MINUTE — the form it already uses against last_execution_time in this same table. Each expression was verified independently against its own engine rather than assumed portable.

server_properties is an on-load collector, so "no offset yet" is the state every server passes through on its first cycle. Both dialects fall back to 0 and the read proceeds, rather than refusing and pre-empting the answers that outrank any window. A pre-migration snapshot whose utc_offset_minutes is NULL behaves identically to an absent row, and a NULL creation_time is still excluded exactly as before — NULL - interval is still NULL.

compile_age_seconds would have been a frame-free alternative, and QueryStatsCollector computes it for exactly this reason, but it is deliberately not stored (PayloadColumns is unchanged; it exists only to inform the delta), so it is not reachable from either store.

AnalysisContext.ServerUtcOffset is deliberately untouched. It is TimeSpan.Zero in both live SKUs, the only non-zero assignments are in the retired Dashboard, and its documented meaning is the inverse — a server-local window converted back to UTC for persistence. Setting it here would silently move the window PgFindingStore persists, which is a different change.

What pins it now

Two behavioural twins, one per store, plus a source-level guard.

ParameterSensitivityClockFrameLiveTests (live Postgres) and ParameterSensitivityClockFrameTests (real DuckDB) assert an invariance rather than a timestamp: the same five compile instants, re-expressed in each server's local clock the way the collector would really have stored them, must yield the identical offender set. Membership is asserted alongside invariance on purpose — a constant empty result is also invariant, so invariance alone is not a test. Three offsets plus the no-offset fallback: UTC-4, UTC, UTC+10. Each covers the detector and the drill-down, which carries its own copy of the same predicate.

CreationTimeClockFrameDisciplineTests scans both analysis trees for a bare creation_time compared against a window bound and censuses the de-skew sites per file. It exists because the inventory found nothing in either suite that compares Darling's analysis SQL against Lite's — no parity guard, no shared constant, not even a count — so the realistic regression is a read ported to one store and not the other. It also reaches the two psp_signature sites, which need a whole Query Store fixture before they return a row. Its discriminator is pinned in both directions.

No existing test asserted the buggy behaviour. Every seeded creation_time in the suite is TestPeriodStart.AddDays(-3) with no collected offset, so those fixtures are offset-tolerant with a three-day margin and pass unchanged.

Verified

Eight one-at-a-time mutations, each anchor-count-asserted and hashed before and after, each watched red and restored:

mutation	assertion that moved
drop the de-skew, Darling detector	detector invariance + site census 0/1
flip the sign, Darling detector	detector invariance + site census
drop the de-skew, Darling drill-down	drill-down invariance + site census 1/2
drop the de-skew, Darling psp_signature	site census 1/2 only
drop the de-skew, Lite detector	Lite detector invariance + site census
flip the sign, Lite detector	Lite detector invariance + site census
drop the de-skew, Lite drill-down	Lite drill-down invariance + site census
drop the de-skew, Lite psp_signature	site census 1/2 only

The two psp_signature rows are the honest gap: only the source guard sees them, which is why it is here.

The three shipped Darling SQL constants were also executed against a real migrated store schema rather than a retyped copy, and the DuckDB expression against DuckDB 1.5.5 in-process.

Not measured: whether stored analysis_findings counts actually look inflated on the fleet. That needs a store query, and it is the comparison that would confirm the over-reporting empirically.

[claude]

Review

Verified the fix against the codebase's existing de-skew precedent (ViewerDataService.SystemEvents, JobHistory, PgFindingStore) and it matches exactly:

• Direction is correct: utc_offset_minutes = local - utc, so creation_time - offset = creation_time_utc is the right sign. Confirmed against the same subtraction already used in ViewerDataService.SystemEvents.DefaultTraceEventsByWindowSql.
• No row multiplication: the svr CTE is a COALESCE(scalar-subquery, 0) with no FROM, so it always returns exactly one row — the FROM v_query_stats, svr cross join can't fan out rows. Confirmed the CTE is referenced only once per query, so no duplicate-materialization risk either.
• No new bind parameter: server_id reuses $1 in the new svr CTE at all 4 sites; checked the C# AddWithValue call sites for ParameterSensitivitySql, ParameterSensitiveSql, and RegressedQueriesSql and the ordinals/bind order are unchanged, matching the PR's claim.
• Lite/Darling parity: all four production sites (PgFactCollector.QueryPerf.cs, PgDrillDownCollector.Queries.cs ×2, DuckDbFactCollector.QueryPerf.cs, DrillDownCollector.Queries.cs ×2) are structurally identical modulo the unavoidable dialect split (make_interval(mins => …) vs. … * INTERVAL '1' MINUTE, since DuckDB has neither make_interval nor AT TIME ZONE without ICU). server_properties vs. v_server_properties naming difference is pre-existing (Darling has no view over that table) and not a parity drift.
• New CreationTimeClockFrameDisciplineTests: regex-verified both ways (hazards it must flag, benign forms it must not, including creation_time_utc, plan_creation_time suffix collision, and plain projections). The scanned-file floor (>= 20) and per-file site census both hold against the current tree (21 + 22 files, exactly 1/2/1/2 de-skew sites as declared). This is the first cross-SKU SQL-parity guard in either test suite — a real gap it closes, not just for this bug.
• NULL/first-cycle handling: COALESCE(..., 0) correctly falls back when server_properties hasn't been collected yet, and a NULL creation_time still short-circuits the comparison to NULL/false as before (no behavior change there).
• Style: matches T-SQL-adjacent conventions used elsewhere for Postgres/DuckDB code in this repo (AS on the table alias, -- line comments, 4-space indent, AND alignment). OPTION(RECOMPILE) doesn't apply — these are Postgres/DuckDB queries, not T-SQL collectors.

Also checked the live-Postgres and real-DuckDB test twins (ParameterSensitivityClockFrameLiveTests / ParameterSensitivityClockFrameTests) — same fixture, same three offsets (-240/0/+600) plus the no-offset fallback, asserting invariance and membership rather than a single pinned count, which is the right test shape for a frame bug (a constant-empty result would otherwise pass the invariance check for the wrong reason).

No correctness, security, or parity issues found. Nothing to flag inline.