Repository navigation
Route the Performance Trends duration chart through the interval table at 48 h and wider (#4310) - #4382
Merged
Conversation
…ough the V145 wide table (#4310 site 3) Mirrors #4341's grid top-read gate: at or above QueryStoreIntervalWide.GridWideMinWindow (12h), on a schema-145+ store, ViewerDataService.GetQueryStoreDurationTrendAsync's raw (non-rollup) arm now tries QueryStoreDurationTrendTableSql over query_store_interval_wide before falling back to raw, on one connection in one read-only REPEATABLE READ transaction via QueryStoreIntervalWide.ReadsTableAsync. Any fault or a coverage miss returns null and the caller reads QueryStoreDurationTrendSql unchanged. The rollup route (#2736) is untouched. Adds QueryStoreDurationTrendTableLiveTests.cs: raw vs. table point equality on a covered window, below-minimum-window routing, and coverage-miss fallback, reusing the grid's own seed. Performance measurement (before/after at 24h/7d on the #3953 seed) not yet run in this lane; a successor must complete it before the PR opens.
…4310 site 3) Site 3 (QueryStoreDurationTrendSql / GetQueryStoreDurationTrendAsync) measured slower on the table at 24h (279ms raw vs 370ms table, lane 4310s3f) but faster at 48h and 72h (rig port 55485, NULL-free 15-day seed, gate-routed, 1 cold + 5 warm): 48h raw 423.0ms vs table 361.4ms, 72h raw 515.0ms vs table 380.5ms. Raw vs table equality exact at 48h (46/46 points). Adds QueryStoreDurationTrendMinWindow = 48h, this site's own constant (does not touch QueryStoreIntervalWide.GridWideMinWindow, the grid's 12h). Adds a 24h below-threshold pin (RED against the pre-fix code, which would have routed a 24h window to the table via the shared 12h GridWideMinWindow).
…tore row (#4310) The upsert's identity carries no collection_time, so a legacy row (interval_start_time_utc IS NULL) re-fetched across several collections keeps only its latest snapshot in query_store_interval_wide. Both duration- trend arm 2's and the grid/MCP top read's arm 2 place these rows at their own collection_time and need every snapshot, not the running maximum, so the table silently understated a window containing one. QueryStoreIntervalWide.ReadsTableAsync gains a sixth clause: once clauses 1-5 already say table, probe collect.query_store_stats (indexed on server_id, collection_time) for any legacy row in the clamped window and refuse if one is found. The probe only runs on the path already headed for the table, so a store with none pays nothing extra; a store with any (field observation: only briefly after an upgrade, until raw retention ages them out) falls back to raw, matching every other refusal clause. Doc comments corrected to state the current truth: the table's own arm 2 is not a safe un-deduped read on its own; it depends on clause 6 having already refused the cases where it would be wrong. Also fixes two CI failures: DocCommentHygieneTests (a stray extra </summary> closing tag) and CS8600 (a nullable local replacing the points = null! pattern in GetQueryStoreDurationTrendAsync).
erikdarlingdata
changed the title
erikdarlingdata
marked this pull request as ready for review
September 26, 2026 04:33
This was referenced Sep 26, 2026
Closed
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.
Refs #4310.
Why
Measured #4310 site 3 (
QueryStoreDurationTrendSql/GetQueryStoreDurationTrendAsync,ViewerDataService.QueryTrends.cs) gate-routed against the grid's sharedGridWideMinWindow(12h) andfound the table LOSES at 24h (279 ms raw vs 370 ms table) even though it wins at 7 days (1,045 ms raw vs
374 ms table). This site therefore gets its own threshold: 48h, or 72h if 48h isn't a clean win.
What changes
QueryStoreDurationTrendMinWindow = TimeSpan.FromHours(48)inViewerDataService.QueryTrends.cs, next toGetQueryStoreDurationTrendAsync. Site 3's own gate checknow uses it instead of the grid's
QueryStoreIntervalWide.GridWideMinWindow;GridWideMinWindowitselfis untouched (the grid keeps its own 12h).
QueryStoreDurationTrendTableLiveTests.cs.Test plan
Build:
dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=true— 0Warning(s), 0 Error(s). Same for
Lite.Tests.csproj— 0/0 (Darling.Tests and Lite.Tests targetnet10.0-windows: they build on macOS but cannot run here; CI runs them).
Rig: docker
timescale/timescaledb:2.30.1-pg18, port 55485, databasepm_test. Harness at/tmp/qs4310s3treferenced the builtPerformanceMonitor.Darling.Storage/.Viewer/.ServiceDLLs via<Reference HintPath>.PgMigrations.MigrateAsync, loadedscratch-3953/seed15.sql, deleted 4,005rows with
first_execution_time IS NULL(a NULL-free seed), then filledquery_store_interval_widevia the product's ownPrepareServerAsync/ApplyBatchAsync(one batch perdistinct
collection_time, 6,624 batches, 0 pending) → 1,715,265 wide rows. Backdatedfilled_sincetothe seed's own first
collection_time(the fill really did cover every raw row from there forward).Measured through the gate (
GetQueryStoreDurationTrendAsyncwith a literal end, mirroring the realrouting), 1 cold + 5 warm, warm median, pooled connections:
48h is the chosen threshold — the lower of the two measured wins, rather than chasing the exact crossover
beyond 48h/72h.
Equality (gate-routed, open end, NULL-free seed) at 48h: EXACT, 46 of 46 points, row for row
(
collection_time,duration_ms_per_second,executions_per_second), matching the earlier exact resultat 24h/7d.
Pins (
QueryStoreDurationTrendTableLiveTests.TableRead_EqualsRaw_OnACoveredWindow_AndRoutesBelowTheMinimumWindow):used the shared
GridWideMinWindow(12h), so a 24h window would have routed to the table there; thispin fails on
17116062f(pre-fix) and passes once the site-specific 48h constant gates it.both, now validating against the correct (higher) threshold.
UnforcedCoverage_FallsBackToRaw_AndStillMatchesRawExactly: unchanged, still exercises thecoverage-miss fallback at the 3-day window.
These pins are listed as build-verified but not run here (
Darling.Testsis net10.0-windows-only); CIruns them live.
CHANGELOG entry
SECTION: Changed
ENTRY:
REF:
[Route the Performance Trends duration chart through the interval table at 48 h and wider (#4310) #4382]: Route the Performance Trends duration chart through the interval table at 48 h and wider (#4310) #4382