Skip to content

Query Store regressions read the per-interval table instead of re-sorting raw rows (#5809) - #5826

Merged
erikdarlingdata merged 10 commits into
devfrom
fix/5809-query-store-regressions-interval-read
Oct 15, 2026
Merged

erikdarlingdata merged 10 commits into
devfrom
fix/5809-query-store-regressions-interval-read

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Fixes #5809.

Status: ready for review. The table read works on both paths, the seven tests that pinned the 9-day horizon are on 10 days, one census pin is satisfied, and every class the brief named passes on a UTC rig (results below). CI runs the full suite.

What was wrong

get_query_store_regressions and the viewer's Regressions grid deduped raw query_store_stats twice per call. Each arm ran ROW_NUMBER() OVER (PARTITION BY seven keys), the baseline arm over 8 days of raw rows. The issue gives the field figures for a 24 h call, rounded: mean 17 s, max 37 s.

What changed

  1. Service read (DarlingQueryStoreRegressionReader): new QueryStoreRegressionsTableSql. It replaces only the two deduping arms with reads of query_store_interval_wide. The aggregation, the CPU > 25% gate and the ranking are a shared suffix, so the two forms cannot drift. Each arm keeps raw's collection_time window and adds the Query Store wide-table reads bound first_execution_time at the window end too (#5523) #5533 first_execution_time range at both ends. The tier choice is get_query_store_top's own: QueryStoreTopMayReadTable, then QueryStoreIntervalWide.ResolveReadAsync over [baseline start, window end] (the Query Store reads on the interval tables can miss an interval that started just below the table's floor #5541 floor rule). The decision and the read share one read-only REPEATABLE READ snapshot. A gate refusal or a fault falls back to the shipped raw SQL.
  2. Why the first lane's live test failed (fixed in d4f1f4a): static field initializers run in textual order. QueryStoreRegressionsTableSql was declared above the QueryStoreRegressionsTablePrefix it concatenates, so it read a null prefix. The table SQL then began at baseline_performance, and every call failed with 42601: syntax error at or near "baseline_performance" and fell back to raw. The viewer copy had the same bug. In both files the prefix now comes first, with a comment saying why.
  3. Fault logging: a table-read fault logs ONE Warning per process that names the exception message (LogTableFault). Later faults log at Debug. With no logger, it writes the Trace line and leaves the once flag for the first fault that has a logger. A gate refusal logs at Debug. A gate decision failure stays a note (the gate logs its own fault).
  4. Why wide, not interval_latest (coordinator ruling): interval_latest holds Regular executions only. The raw read counts Aborted and Exception too. The wide table has raw's exact PARTITION BY key, execution_type_desc included.
  5. Wide horizon 9 -> 10 days (QueryStoreIntervalPartitions.WideHorizonDays, coordinator ruling; a constant, no migration). The Query Store reads on the interval tables can miss an interval that started just below the table's floor #5541 gate rule is unchanged. A 24 h call spans 8 days, and 8 d + 26 h never fits in a 9-day table. With 10 days, windows up to about 46 h take the table; longer ones read raw. Storage cost: one more day of wide rows.
  6. Viewer (ViewerDataService.QueryStoreRegressions.cs): the viewer cannot call the service reader, because it does not reference the service assembly (Darling viewer: built-in tabs silently lose history past 4 days - CAGG read-routing only covers Custom Views #1661, PostgreSQL targets have no UI on either SKU, and both UIs render SQL Server tabs at them #2530). Its copy takes the same shape: the same table SQL with its own parameter numbering, the grid's schema-version gate, ResolveReadAsync with GridWideMinWindow, and a raw fallback. TryGetQueryStoreRegressionsFromTableAsync is now internal, so the live test can check the viewer's table path directly. GetQueryStoreRegressionsAsync gains literalEndUtc.
  7. Known edge (documented in the SQL summary): raw counts an interval collected on both sides of the window start in both arms, and the table counts it once, in the window.

Measurement (rig, UTC, numbers rounded)

The store was seeded through the write path with the planted regression, then amplified to 2,000 query copies: 52k raw rows and 26k wide rows. Two snapshots per interval, so raw is only 2x the table here. The field has more snapshots per interval. Each read was run four times; the first run is cold.

read exec time (warm runs) shared buffers hit
raw (QueryStoreRegressionsSql) 380-640 ms (cold 240 ms) about 10.2k
table (QueryStoreRegressionsTableSql) 65-85 ms about 10.5k

Both return the same 50 rows. On the unamplified planted shape (26 raw rows, 13 wide), both run under 1 ms. Buffers are about the same: the wide rows are wider, and the saving is the sort, not the pages read. Gate decision for a 24 h call (a 192 h span from baseline start to window end) with the 10-day horizon: UseTable=True, DecisionFailed=False, QueryStoreTopMayReadTable=True. The service table path returned the rows (not null).

Test plan

All runs on a UTC rig (own cluster, timescaledb,pg_stat_statements preloaded, CI's worker settings), on head a7d4a7e unless noted. Build: Darling.Tests 0 warnings, 0 errors. No full suite on the laptop: CI decides.

  • QueryStoreRegressionsIntervalTableLiveTests on a UTC rig (own scratch store), plus the new QueryStoreRegressionsTableReadTests: Total: 5, Errors: 0, Failed: 0. The live test checks that the service and viewer table paths each return non-null and identical to raw, with the planted regression (query 10, baseline 90 executions, window 42, Aborted counted). It checks that the executed plan's scan nodes (EXPLAIN ANALYZE VERBOSE JSON, each partition or chunk resolved to its root through pg_inherits) read query_store_interval_wide rows and no query_store_stats node, for the service and viewer SQL. The raw read's plan does read query_store_stats rows, so the check can fail. When the floor moves past the baseline start, both table paths return null and the raw fallback returns the same rows.
  • QueryStoreRegressionsTableReadTests (unit): both table SQL texts start with WITH deduped_baseline and name no query_store_stats. A fault warns once per process with its message, then logs at Debug. The old declaration order produced the 42601 above.
  • Horizon pins moved to 10 days (expected values and day offsets only; no product code, and no rule changed): QueryStoreIntervalPartitionsTests.ALegacyMax_MoreThanOneHorizonPastTheNormalS_IsNotArmedOver_ButOneWithinItIs (horizon 10, last armed S 10-20), QueryStoreIntervalPartitionsLiveTests.DropExpired_TakesWholeExpiredDays_KeepsAPartialOne_AndDropsLegacyOnlyOnceBounded (edge 10-10), QueryStoreIntervalRetentionLiveTests.ADropThatHitsALockTimeout_GivesUp_AndSucceedsOnTheNextPass (promoted 13 days back, whole day 9-27). Those three classes: Total: 45, Errors: 0, Failed: 0, Skipped: 0.
  • Three more horizon pins, found by the *QueryStore* run and moved the same way: in QueryStoreIntervalConvergenceLiveTests, ALegacyRowPastTheNormalS_MakesTheArmPickALaterS_TheValidatePasses_AndThePromotionSucceeds (legacy dropped ten days after S: 10-24, kept at 10-23 12:00), ALegacyRowDatedMoreThanOneHorizonAhead_IsNotArmedOver_WarnsOncePerMaximum_AndTheArmGoesAheadOnceItIsGone and AFarFutureLegacyRow_AnHourOfBothLoops_RequestsNoLock_WhileAnAutovacuumLikeLockIsHeld (the far row moves from 10-19 to 10-20), and ALegacyRowOneMicrosecondInsideTheHorizon_IsArmedOver_AndTheValidatePasses (the exact-limit row moves to 10-19 23:59:59.999999, S 10-20; it still passed on 9 days but no longer pinned the edge). Two stale "9 days" comments now say 10 (QueryStoreIntervalWideWriterTests, QueryStoreIntervalWideBrinIndex).
  • Every *QueryStore* class (127 classes, unit and live), before those three moves: Total: 1119, Errors: 0, Failed: 3, Skipped: 2. The 3 failures were the convergence pins above. The 2 skips were QueryStoreIntervalFloorWarmUpTests.TheRetentionPass_WarmsEachServersFloorRead... (both tables), on its own documented guard (another class's open transaction predates the drain, After a large retention drain, each server's first Query Store read is slow until vacuum runs #5581). After the moves, QueryStoreIntervalConvergenceLiveTests + QueryStoreIntervalFloorWarmUpTests + QueryStoreIntervalWideWriterTests: Total: 39, Failed: 1. The one failure was the warm-up theory's wide case (40112 buffers, beside the convergence class). Alone, QueryStoreIntervalFloorWarmUpTests: Total: 17, Errors: 0, Failed: 0, Skipped: 0. Its expired rows are seeded relative to HorizonDays, so the horizon change does not touch it; the failure is the same open-snapshot limit as the skips.
  • One census pin moved: LiveClockCensusTests.EveryLiveTestThatReadsAPlanOrTheChunkCatalog_SeedsThroughTheLiveClock_OrIsAllowedWithAReason failed on the new QueryStoreRegressionsIntervalTableLiveTests (it reads executed plans and used DateTime.UtcNow). The test now seeds and reads through LiveClock.Now(). Its 24 h window holds in any chunk layout, so AnchoredEnd() does not apply.
  • Every *Census* and *Ratchet* class (includes GuardStageCensusTests, SlowClassCensusTests, LiveClassStoreSkipCensusTests, ScratchDatabaseDisposalCensusTests, McpPayloadContractCensusTests, LiveCleanupConversionRatchetTests), plus QueryStoreRegressionsIntervalTableLiveTests and QueryStoreRegressionsTableReadTests: Total: 596, Errors: 0, Failed: 0, Skipped: 0.
  • DarlingComposeTests, RawWindowFloorViewerPortTests, PlanForceNoWritePathTests, DocCommentHygieneTests: Total: 424, Errors: 0, Failed: 0, Skipped: 0.
  • Every class in a test file that names a changed identifier (WideHorizonDays, QueryStoreIntervalWideRetentionDays, the regressions SQL and reader, ClampedStart, HorizonDays): ViewerQueriesSqlTests, ViewerQueriesDisplayTests, ViewerQueryStoreRegressionsTests, ViewerQueriesLivePostgresTests, ViewerQueriesRestLivePostgresTests, ViewerQueryStoreRegressionsDataStartTests, ViewerQueryStoreRegressionsDataStartLiveTests, DarlingQueryStoreRegressionsSurfaceAndSqlTests, DarlingQueryStoreRegressionsLiveTests, DarlingMcpQueryStoreRegressionsBudgetLiveTests, ViewerCalendarRetentionPortTests, ViewerCalendarRetentionLivePostgresTests, ViewerShortWindowDataStartTests, ViewerQueryGridsDataStartLiveTests classes, DataWindowFloorSnapshotAndRunLogLiveTests, McpZeroIsAMeasurementTests, McpZeroIsAMeasurementLivePostgresTests, StatementFilterQueryReadsLiveTests, AlertNotebookTemplateQueryPlansTests: Total: 220, Errors: 0, Failed: 0, Skipped: 0.
  • Full suite: not run on the laptop (standing rule). CI decides.
  • Lite: this PR changes no Lite file. Lite has no query_store_interval_wide table, so there is nothing to mirror.

Left for the coordinator

  • "Done when": the field recheck (a 24 h call under 5 s at p95) stays open until a store runs this build.
  • QueryStoreIntervalFloorWarmUpTests' wide case can fail, not skip, when another live class holds a snapshot that its holder probe no longer sees. That is a test-guard race, not this PR's change. If CI hits it, it belongs in a flake census.

Fix lane (CI + bot review)

Head 0011938a9. CI run 38664769073 (head a7d4a7e) had one red test; review 5526961589 had five findings.

  1. Red census test (f293cd338): McpReadCommandTimeoutTests.EveryCommandInScope_IsBuiltAgainstTheStore_NotAMonitoredTarget flagged DarlingQueryStoreRegressionReader.cs:386. The table read's SET TRANSACTION READ ONLY command passed a string literal, and the census only recognises new NpgsqlCommand(<name>, connection) on a store-pool connection. It now uses a named SetTransactionReadOnlySql constant, as DarlingMcpQueryStoreHistoryTools does, with the same store deadline. No exclusion was added. The viewer copy has the same shape as its neighbours (ViewerDataService.QueryStore.cs:505, QueryTrends.cs:639) and its census was green, so it is unchanged.
  2. Stale 9-day statements (bc413deea): QueryStoreIntervalWideBrinIndex.cs:50 now says "a full 10 days", and :23 marks its 9-day measurement as the horizon at that time. QueryStoreBackgroundIndexes.cs:59 now says 10 days, noting it was 9 before get_query_store_regressions sorts eight days of raw query_store_stats rows that the interval tables already dedupe #5809. PgMigrations.cs:180 now says "15 and 10 days". The V145 rung (PgMigrations.cs:3397) keeps "Kept 9 days at V145" as its history and adds one sentence: the horizon is 10 days since get_query_store_regressions sorts eight days of raw query_store_stats rows that the interval tables already dedupe #5809, because the 7-day baseline plus a 24-hour window, with the 26-hour purge-edge margin, needs 9 days 2 hours held. The grep also found MigrationDataMovingRungCensusPins.cs:213 (V154's reason text), which now says "was kept to 9 days then (10 since get_query_store_regressions sorts eight days of raw query_store_stats rows that the interval tables already dedupe #5809)". The other hits for 9 days, nine days and AddDays(-9) are test fixtures, other tables, or QueryStoreIntervalPartitionsTests calls that pass 9 as an explicit argument. None of them is a statement about the wide horizon.
  3. Load-test purge edge (4b014a80b): measure.ps1 no longer has its own day count. It reads public const int WideHorizonDays = N; from QueryStoreIntervalPartitions.cs (relative to $PSScriptRoot). If the file is not there, it prints a warning and skips the check, so it never falls back to a stale constant. In README.md, 7-day reads now stay complete for 3 days, 1-day reads for 9 days, and the sequence should finish within 3 days. New pin QueryStoreLoadTestHarnessTests.MeasureScript_PurgeEdge_ReadsTheWideHorizonConstant runs the script's own path and pattern against the source and asserts the value is WideHorizonDays, and that no $purgeEdge = ...AddDays(-<digit> remains. The script parses cleanly, and the pattern returns 10 when run by hand.
  4. Self-contradicting test comment (0011938a9): QueryStoreIntervalPartitionsLiveTests.cs:478 now describes what the test plants. The drops run at edge, not at now: one microsecond before 10-10 00:00, the 10-day cutoff is one microsecond before 9-30 00:00. So legacy and 9-28 are whole expired days, and 9-29 is the partial day that stays. The plant already exercised that case, so no dates moved. The comment on the second store (:510) said "cutoff 9-28" (9-29 before this PR), but that store is the latest table, so the cutoff is 9-23 14:55 under its 15-day horizon. That is fixed too.
  5. history_source (f293cd338): the new ReadQueryStoreRegressionsAsync returns the rows and the tier that answered (interval_table / raw, the ReadScope words). GetQueryStoreRegressionsAsync stays as a wrapper around it. get_query_store_regressions now puts history_source on its rows payload, after truncation_note. The empty and status answers are status envelopes and carry no tier, the same as get_query_store_top. The tool description ends with the half-sentence the top tool has: "Reads the per-interval table when it holds the whole baseline and window, and the raw tier otherwise; history_source says which served." Pins: QueryStoreRegressionsIntervalTableLiveTests asserts interval_table on the table path and raw after the floor rule forces raw. DarlingQueryStoreRegressionsLiveTests asserts raw in the tool payload. The viewer copy feeds a WPF grid and has no payload, so it has no place for the field. It is unchanged.

Runs (own rig: port 5673, UTC, timescaledb,pg_stat_statements, CI's worker settings; stopped at the end):

  • Guard, Darling: Darling.Tests Total: 4578, Errors: 0, Failed: 0, Skipped: 0 (run after the last edit)
  • Guard, Lite: Lite.Tests Total: 1281, Errors: 0, Failed: 0, Skipped: 0
  • Targeted, live (McpReadCommandTimeoutTests, every *QueryStoreInterval* and *QueryStoreRegressions* class, DarlingCompose* and the Compose* live classes, QueryStoreLoadTestHarnessTests, MigrationDataMovingRungCensusPins, McpPayloadContractCensusTests, McpToolGuideHeads*, McpToolsListBudgetTests, McpServedSchema, DarlingMcpToolClassification, McpZeroIsAMeasurement*, DatabaseNameParamTwinTests, DarlingMcpDataTools*, McpReadToolBudgetLiveTests, DarlingMcpStoreHostBudgetLiveTests, StatementFilterQueryReadsLiveTests, QueryStoreReadsDatabaseFilterLiveTests, AlertNotebookTemplateQueryPlansTests, QueryStoreComposeStampTests, ViewerQueries*, ViewerQueryStoreRegressions*, DarlingQueryStoreRegressions*): Total: 1180, Errors: 0, Failed: 0, Skipped: 1. The skip is QueryStoreIntervalFloorWarmUpTests...(table: query_store_interval_latest), which skips for its own reason.
  • Rerun after the history_source pins and description edit (the touched classes, plus census and budget pins): Total: 370, Errors: 0, Failed: 0, Skipped: 0. QueryStoreRegressionsIntervalTableLiveTests alone: Total: 1, Failed: 0, Skipped: 0.
  • Full suite (laptop rule; CI decides).

SECTION: Fixed
ENTRY: - Query Store regressions read the per-interval table instead of re-sorting eight days of raw rows ([#PR]) - get_query_store_regressions and the viewer's Regressions grid deduped raw Query Store snapshots on every call, which took 17 s on average and up to 37 s for a 24-hour window on a large store. Both now read query_store_interval_wide for windows up to about 46 hours, with every execution outcome counted as before, and fall back to raw otherwise. The wide interval table now keeps 10 days instead of 9, and the regressions payload names the tier that answered in history_source.

… gate allows (#5809)

Both arms read the interval table's latest snapshot per interval instead of deduping raw query_store_stats with a window sort, bounded on first_execution_time both sides (#5533). The wide gate decides over baseline start to window end, so a store whose table does not cover that span reads raw (#5541).
…izon 9 -> 10 days, live test (#5809)

WIP: the new live test fails on the interval path; see the PR body's handoff.
…joins them; fault warns once per process (#5809)

Static field initializers run in textual order, so QueryStoreRegressionsTableSql read a still-null prefix in both the
service reader and the viewer copy; the table SQL began at baseline_performance and every call failed with 42601 and
fell back to raw. A table-read fault now logs one Warning per process naming its message, later ones at Debug; a gate
refusal logs at Debug. The live test asserts from the executed plan's scan nodes (resolved to their root table) and
checks the viewer's table path directly.
The legacy-max arm bound, the drop-expired edge and the lock-timeout whole day each shift by one day; the rules they pin are unchanged.
… wide horizon (#5809)

The far-future row, the exact-limit row and the legacy drop day each shift by one day; two stale nine-day comments now say ten.
It reads executed plans, so LiveClockCensusTests requires its clock to come from LiveClock.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 15, 2026 01:16
Comment thread Darling/loadtest/measure.ps1 Outdated
# The service purges wide rows older than 9 days (QueryStoreIntervalPartitions.WideHorizonDays), and the copy
# The service purges wide rows older than 10 days (QueryStoreIntervalPartitions.WideHorizonDays), and the copy
# gets no new ones, so a window that starts before that edge has lost rows to the purge.
$purgeEdge = (Get-Date).ToUniversalTime().AddDays(-9)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment two lines up now says 10 days and names QueryStoreIntervalPartitions.WideHorizonDays as its source, but the edge is still computed at -9. The derived constant did not move with the horizon.

The warning now fires for windows that are complete: real loss starts below now - 10 days, so a 168 h read whose anchor sits 9.5 days back prints

WARNING: the 168h window starts before the purge edge ... Restore a newer copy.

and the README (Darling/loadtest/README.md:96) tells the operator that means restore a newer copy and start the sequence of N again — a re-run bought by a stale literal.

Suggested change
$purgeEdge = (Get-Date).ToUniversalTime().AddDays(-9)
$purgeEdge = (Get-Date).ToUniversalTime().AddDays(-10)

Comment thread Darling/loadtest/README.md Outdated
The copy gets no new Query Store rows, because its production servers cannot be reached. The reads of the test end at the newest Query Store row of the server (see "Measure at each N"), so their windows stay full. But the service still purges rows by their age each day. You cannot change the purge age: it is fixed in the code, at 9 days for `collect.query_store_interval_wide` and 15 days for `collect.query_store_interval_latest`. No setting raises it. So:
The copy gets no new Query Store rows, because its production servers cannot be reached. The reads of the test end at the newest Query Store row of the server (see "Measure at each N"), so their windows stay full. But the service still purges rows by their age each day. You cannot change the purge age: it is fixed in the code, at 10 days for `collect.query_store_interval_wide` and 15 days for `collect.query_store_interval_latest`. No setting raises it. So:

- The 7-day reads stay complete for 2 days after the newest row of the copy. The 1-day reads stay complete for 8 days.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence above this bullet moved to 10 days, but the three numbers derived from it did not. They are the 9-day arithmetic:

  • "The 7-day reads stay complete for 2 days" → 3 days (10 − 7).
  • "The 1-day reads stay complete for 8 days" → 9 days (10 − 1).
  • line 97, "finish within 2 days of the backup" → 3 days.

Same shape as the measure.ps1 edge: the prose that states the horizon was swept, the values computed from it were not. An operator reading this plans a shorter run than the copy actually supports.

/// the index, on the same store at <c>random_page_cost</c> 1.1, a warm Parallel Bitmap Heap Scan read 24 h in
/// 4.3 s (1.14M blocks), 12 h in 1.6 s, 6 h in 0.8 s and 48 h in 9.2 s. Those figures are from a store still
/// inside its first retention cycle. Once the 9-day purge runs, the freed early pages are reused by new
/// inside its first retention cycle. Once the 10-day purge runs (9 days before #5809), the freed early pages are reused by new

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR body says "Two stale 9 days comments now say 10 (QueryStoreIntervalWideWriterTests, QueryStoreIntervalWideBrinIndex)". This file has two, and only this one moved — line 50 of the same file still reads

a lock that blocks the writer for the whole heap read: 108 s at 25 GB, minutes at a full 9 days.

Three more statements of the current horizon in product source are also still at 9:

  • QueryStoreBackgroundIndexes.cs:59 — "several minutes at a full 9 days' retention"
  • PgMigrations.cs:180 — "two tables are kept to 15 and 9 days respectively by the same purge this index speeds up"
  • PgMigrations.cs:3397 — "Kept 9 days (F1: the 7-day preset plus a day of margin for an interval that can span a day)"

The last one is the V145 rung's own statement of what the table keeps, and the parenthetical is the superseded reasoning: DarlingRetention.cs:186 now carries the #5809 argument (8 days + 26 h = 9 d 2 h, which 9 days never is) and this one still carries the 7-days-plus-a-day one. The measurement prose on the index files can honestly stay a measurement taken at 9 days if it says so; the two PgMigrations lines read as present tense.

await QueryStoreIntervalPartitions.CreateAheadAsync(connection, Wide, now, logger, ct);

/* Rows: legacy (before S), day 9-28 (whole day expired by 10-08), day 9-29 (ends 9-30 00:00; the cutoff is 9-29 14:55 on 10-08, so it is a partial day). */
/* Rows: legacy (before S), day 9-28 (whole day expired by 10-08), day 9-29 (ends 9-30 00:00; the cutoff is 9-28 14:55 on 10-08 under the 10-day horizon, #5809, so it is a partial day). */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the number moved mechanically and the comment now contradicts itself. With now = 2026-10-08 14:55:30 (line 470) the 10-day cutoff is 9-28 14:55, so:

  • "day 9-28 (whole day expired by 10-08)" is false — 9-28's upper bound is 9-29 00:00, above the cutoff, so it is the partial day at 10-08.
  • "day 9-29 ... so it is a partial day" is false for the same reason in the other direction — 9-30 00:00 is a day and a half above the cutoff, so 9-29 is not expired at all at 10-08.

The whole/partial split the test actually asserts is the one at edge (line 491, 10-10 00:00), and the comment on line 490 states it correctly. The assertions are unaffected, so this is comment-only — but as written it describes a cutoff the test never drops against.

claude[bot]
claude Bot previously requested changes Oct 15, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — table SQL replaces only the two deduping arms; aggregation, the CPU > 25% gate and the ranking are a shared suffix — DarlingQueryStoreRegressionReader.cs:95, :298, suffix :152-:220.
  • VERIFIED — each arm keeps raw's collection_time window and adds the #5533 first_execution_time range both ends — :241-:242, :262-:263; safe because PurgeEdgeMargin (26 h) exceeds the bound every row satisfies (QueryStoreIntervalWide.cs:535, :548-:558).
  • VERIFIED — tier choice is get_query_store_top's own pair over [baseline start, window end], decision and read in one read-only REPEATABLE READ snapshot — :376, :385-:392, :410; viewer ViewerDataService.QueryStoreRegressions.cs:403-:418.
  • VERIFIED — gate refusal or fault falls back to raw; an empty list from the table is a real answer — :340-:348, :393-:406, :419-:424.
  • VERIFIED — the 42601 cause and fix: prefix declared above the field that concatenates it, reason inline, in both files — :222-:230 before :298, viewer :252-:260 before :328 (each pair in one file, so the order holds for the partial class too).
  • VERIFIED — one Warning per process naming the message, later faults Debug, null logger writes Trace and leaves the flag — :430-:440, pinned by both QueryStoreRegressionsTableReadTests fault tests; McpToolLatencyFilter.cs:110 supplies the real logger.
  • VERIFIED — wide, not interval_latest: wide identity is raw's exact PARTITION BY key incl. execution_type_desc — QueryStoreIntervalWide.cs:78 vs QueryStoreIntervalLatest.cs:232.
  • VERIFIED — one constant, no migration; 8 d + 26 h never fits in 9 days; ~46 h cut-over; UseTable unchanged — QueryStoreIntervalPartitions.cs:188, DarlingRetention.cs:198, clause 3 QueryStoreIntervalWide.cs:723-:724.
  • VERIFIED — viewer copy's own numbering, schema gate, GridWideMinWindow, raw fallback, literalEndUtc as the grid passes it; read never goes below raw's span (:411 binds ClampedStart, not ReadStart) — viewer :371-:380, ViewerServerTab.Queries.cs:467 matching :288.
  • VERIFIED — one more day of wide rows (docs/darling-store-sizing.md:28); no Lite file changes and nothing to mirror (Lite/Services/LocalDataService.QueryStoreRegressions.cs:84 has no second tier, no payload key moves).
  • UNVERIFIED — field figures (mean 17 s, max 37 s) — a production measurement.
  • UNVERIFIED — rig timings, "both return the same 50 rows", gate readout — needs the seeded rig and a live Postgres.
  • UNVERIFIED — every test tally and "0 warnings, 0 errors" — no build or test run here.
  • REFUTED — "Two stale 9 days comments now say 10". QueryStoreIntervalWideBrinIndex.cs has two; only :26 moved, :50 still reads "minutes at a full 9 days". Three more present-tense statements are still 9: QueryStoreBackgroundIndexes.cs:59, PgMigrations.cs:180, PgMigrations.cs:3397 ("Kept 9 days (F1: the 7-day preset plus a day of margin ...)") — the V145 rung's own statement, still carrying the reasoning DarlingRetention.cs:186 replaced.

Riskiest lines

  1. DarlingQueryStoreRegressionReader.cs:236 — 1 AS rn stands in for the #1841 ROW_NUMBER() dedup the suffix's WHERE rn = 1 was written for; holds only while the wide upsert key stays exactly raw's PARTITION BY (QueryStoreIntervalWide.cs:78, guard :385), so a key added to raw alone under-counts silently — pinned by QueryStoreRegressionsIntervalTableLiveTests.ARegressionBetweenTheBaselineAndTheWindow_IsTheSameFromTheIntervalTableAndFromRaw (live only)
  2. QueryStoreIntervalPartitions.cs:188 — WideHorizonDays = 10 moves purge, create-ahead, read floor and compose rollup floor at once — pinned by QueryStoreComposeStampTests.TheTenDayHorizon_IsOneConstant_SharedByThePartitionsThePurgeTheWideReadAndTheRollupFloor
  3. Darling/loadtest/measure.ps1:205 — purge edge still AddDays(-9) under a comment this diff moved to 10 — UNPINNED

Traps near this diff

  • "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — the interval tier is PostgreSQL-only; Lite/Services/LocalDataService.QueryStoreRegressions.cs:84 has no tier to route to and no payload key changes.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — the viewer cannot reference the service assembly (#1661/#2530), and QueryStoreRegressionsTableReadTests.cs:36-:41 holds each copy to a fixed shape, not to the other.
  • "A retention-tiered store answers a 7-day question from a 4-day raw tier and labels the missing days quiet unless the read routes by tier and says which tier served it (#3590)" — does not apply — :411 binds ClampedStart, so the table never answers below raw's floor; the notice stays raw's probe (DarlingMcpQueryStoreRegressionTools.cs:109-:114).
  • "Truncation inferred from a full page ... Fetch limit + 1" — does not apply — the tool still over-fetches (:101), the table form binds the same $5, :118 unchanged; its sibling "refused, never clamped" holds too (:84).
  • "CASE WHEN interval > 0 THEN delta / interval ELSE 0 END spells 'unknowable' as 0", "Integer division in a rate column", "ORDER BY 3 + 4 DESC" — do not apply — no rate or delta column; NULLIF after CAST(... AS double precision) (:155-:157, :184-:190); ranking on a named column (:218).
  • "A roster-based census does not see the file it does not roster ... rosters are for the positive contract only" — does not apply — ViewerQueriesTests.cs:237-:241 is a positive-contract roster, and the new SQL meets the dedup contract by construction, as the new pins state.
  • ".gitattributes says text=auto eol=crlf ... a diff that touches every line is a line-ending conversion" — does not apply — targeted hunks throughout; only the two new test files are whole-file.

Divergence (blind read vs PR body)

The read matches the body on every mechanism checkable from the tree. The divergence is the horizon bump's blast radius, which the body treats as finished.

  1. measure.ps1:205 is code and did not move: the comment above it now names the 10-day horizon while the edge is still AddDays(-9), so the harness warns on windows that are complete — and loadtest/README.md:96 tells the operator that warning means restore a newer copy and restart the sequence of N.
  2. loadtest/README.md:95/:97: the horizon sentence moved in the same hunk, the values derived from it ("2 days", "8 days", "within 2 days") did not.
  3. The stale-comment sweep is incomplete in four more places (REFUTED above), one in a file this diff did partly fix.
  4. Not in the body: the comment at Darling.Tests/QueryStoreIntervalPartitionsLiveTests.cs:478 was renumbered mechanically and now contradicts itself — at now = 10-08 14:55 the cutoff is 9-28 14:55, making 9-28 the partial day and 9-29 not expired at all. The drops assert against edge (10-10), so nothing fails; the comment no longer describes the test, though the body calls these edits "expected values and day offsets only".
  5. Observation only: the payload names no tier, unlike get_query_store_top's history_source, though the path calls ReadScope.NoteSource (:356, :414). Fine because the read is clamped to raw's span, but get_read_latency and the once-per-process Warning are then the only signals it serves.

Reviewer checklist

  • Run QueryStoreRegressionsIntervalTableLiveTests with DARLING_TEST_PG set — the only pin on table/raw equality, the ClampedStart binding and the EXPLAIN-ANALYZE claim; it skips silently without the variable.
  • On a store past ten days whose purge has run, confirm a default 24 h call really takes the table: the 46 h cut-over needs tableFloor settled at now - 10 d.
  • Re-capture the rig timings with cold and warm separated; "raw: 380-640 ms (cold 240 ms)" cannot be read as written.
  • Confirm the grid still paints inside its interactive deadline on a large store: every call now pays the schema probe plus the gate's round trips, including PlainTableFloorSql (19.6 s / 854,535 heap fetches right after a large purge delete, QueryStoreIntervalWide.cs:465-:474).
  • After fixing measure.ps1, run the Reads step on a copy whose anchor is 9-10 days old and confirm no WARNING prints.

@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 15, 2026 03:06

Claude posting for Erik Darling. Stale: all five findings were fixed by the fix lane at 0011938 (the store-command census red, the four stale 9-day statements plus a fifth the grep found, measure.ps1 reading WideHorizonDays from source with the README figures, the test comment, history_source on the payload); see the PR body section Fix lane (CI + bot review).

@erikdarlingdata
erikdarlingdata merged commit 8f42150 into dev Oct 15, 2026
27 of 29 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5809-query-store-regressions-interval-read branch October 15, 2026 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant