Skip to content

Plan regression reads stop re-reading three open days of Query Store intervals, and on-demand calls reuse the candidates for one analysis cadence (#5812) - #5817

Merged
erikdarlingdata merged 25 commits into
devfrom
fix/5812-plan-regression-incremental
Oct 15, 2026
Merged

erikdarlingdata merged 25 commits into
devfrom
fix/5812-plan-regression-incremental

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Fixes #5812.

Status: ready for review, merge prep done. Merged with dev after #5819, so this PR's rung is V185 (dev's V185 is #5819's investigation tables). Round 2's five Lows are in. The newest word is ## Fix lane (review round 1) at the end, after ## Merge prep (round 2 Lows, V185); where an older lane section disagrees with it or with the diff, the newer section wins. Read first: (1) the cache injection, PgFactCollector.PlanRegressionCache; (2) the work_mem bound, PlanRegressionWorkMemSql (SET LOCAL work_mem = '64MB', every PLAN_REGRESSION read); (3) the watermark read, the $6 lookup in PlanRegressionDailySql and PlanRegressionLiveFloorWithWatermarks; (4) the builder's two queues, PlanRegressionDaily.PlanSql.

Why

The scheduled analysis pass runs the PLAN_REGRESSION read for every server on every pass. Since #5448, closed days (three or more days old) come from collect.plan_regression_daily. The three open days still come from collect.query_store_interval_latest on every run. The issue measured, on its largest store, one run for the busiest server reading about a million live interval rows and sorting them to disk twice, in about five seconds. The pg_stat_statements counts in the issue (about 26,000 calls in 20 days, about 1,300 a day) fit a 30-minute cadence across a few dozen servers, plus MCP and viewer calls.

What changes

First lane (commit a87e4a9, rebased onto dev): a current pass reuses the candidate rows of a fresh read of the same shape (PgFactCollector.PlanRegressionCache.cs), and the daily builder planned the newest days first (ORDER BY d.day DESC). That order now holds only for days never built: since the merge prep (L5), queue 1 takes the longest-waiting day first.

This lane (commit b5fba44):

  1. Partial-day builds (V185). plan_regression_daily_built gains built_through and partial_seq. The builder now plans days from T-14 through T+1 (NewestBuiltDayAhead), so it builds the open days too. A build covers the day's rows (day = the date of last_execution_time, as before) whose first_execution_time is below the watermark. The watermark is the next midnight for a whole day. For an open day it is three hours before the current hour, read from the store's clock in SeqSql (PartialWatermarkLagHours). An open day is built again only when it has rows whose first execution is in [built_through, the new watermark); otherwise AdvanceIdleSql moves its watermark with no build. NULL built_through (a build from before V185) reads as the whole day, so the migration invalidates no built day.
    • Why the watermark is on first_execution_time, not last_execution_time. first_execution_time is part of an interval row's identity, so it never changes when the collector updates the row. A row stays on one side of the watermark for its whole life. A last_execution_time watermark would let a running interval cross it, so the row would be counted in both halves. The interval table is also partitioned and indexed on first_execution_time, so the live half touches only the rows after the watermark.
    • PLAN_REGRESSION does not compare plans compiled for different inputs (#5630) #5631 validity, extended to partial days. A day is read from its totals only while built_seq = late_seq (a whole build) or partial_seq = late_seq (a partial build, through its built_through). V185 recreates the late-row trigger with its upper bound moved from yesterday's midnight to two hours before the current hour, so a write that could change a partial day marks it. The builder's watermark sits one hour below the trigger's bound, so a write whose transaction began up to an hour before a build still marks the day. Steady collection does not fire the trigger. The collector fetches only intervals whose last_execution_time advanced (QueryStoreCollector.WatermarkColumn), and those intervals start at or after the trigger's bound for intervals of 60 minutes or less, with an hour to spare: the trigger compares the monitored server's first_execution_time with the store's now(), so the bound sits an hour below the start of the interval that has just ended, and a store clock up to an hour ahead of the monitored server's stays quiet (review round 1; at the old one-hour bound that margin was zero). A bigger skew, or day-long intervals, fires on every apply, so the open days read live, as today, and pay a trigger call per row.
  2. The read. PlanRegressionDailySql takes $6, each built day's watermark. The live half drops a row when its day is built and its first execution is below that day's watermark (a lookup of the row's day in $3 and its watermark in $6, in the scan's filter, replacing <> ALL($3); see the fourth lane). PlanRegressionLiveFloorWithWatermarks sets the floor to the lower of: the lowest partial watermark, and one day below the first day that is not built. On a current store every day through tomorrow is built, so the floor is today's watermark and the live half reads the last two to three hours of first executions.
    • The "off-by-one" floor is not an off-by-one. One day below the first unbuilt day is required: a row of day D can start on D-1 when its interval crosses midnight (a day-long interval, or an hourly one on a server whose intervals are not aligned to UTC hours). This change removes the field case's wasted partition read a different way: the day after the newest built day is no longer one the read has to look back from. A stale day still reads its day before. The test for this is not written yet (see Handoff).
  3. No spill (bound 2), fifth lane, commit 8de5251. Measured with the manual rig PlanRegressionDailyNoSpillRig.Measure (skipped unless DARLING_NOSPILL_RIG_PLANS is set): one server, 15 built days of totals (360,000 daily rows, today built through 10:00) and one open hour (24,000 interval rows). At the default work_mem (4MB) the read spilled: the outer plan_agg GROUP BY sorted 384,000 rows with an external merge, about 16MB on disk, about 0.7 s. The fix is SET LOCAL work_mem = '64MB' (128MB in 8de5251; the sixth lane lowered it, see below) in the snapshot transaction (PgFactCollector.PlanRegressionWorkMemSql, run right after SET TRANSACTION READ ONLY), so it ends with that transaction and touches no other statement on the pooled connection. With it the planner hashes both aggregates in memory (peak about 37MB each at 64MB), no node spills, about 0.4 to 0.5 s. A sort or hash takes memory only as it grows, so a small server's read uses what it needs. PlanRegressionTimeoutCaveatTests now counts four commands in the fact (pin moved deliberately) and PlanRegressionDailyBoundaryTests pins the SET LOCAL inside the snapshot.
  4. Candidate reuse cadence (ruling R3, commit 3ee66a6). RefreshAfter is now one analysis cadence, not six hours. The worker sets it on every scheduling tick from the configured interval, clamped to 5-360 minutes (default 30) (DarlingWorker.cs, next to analysisIntervalMinutes). The scheduled pass never reuses: it always reads and refills the entry (AnalysisContext.RefreshPlanRegressionCandidates, set where persistCollectionCaveats marks the scheduled pass). Only analyze_server and get_analysis_facts with no as_of reuse, and only a read younger than one cadence. The scheduled pass, its findings and its alerts are as fresh as before.
  5. Bound 4 (a migration that invalidates every built day): V185 invalidates nothing (NULL built_through reads as whole). A store that is behind gets its newest days first: the builder's first hourly tick after start plans newest-first, queue 1 (days with no valid build) at most MaxBuildsPerTick builds per tick, never-built days newest first, then the oldest build first; queue 2 (partial days with rows to add) adds at most the number of builder servers.
  6. Lite: Lite runs its own PLAN_REGRESSION over DuckDB with the same 14-day shape (Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:523, TimeRangeStart.AddDays(-14) over v_query_store_stats). It is a local single-user store with no scheduled-pass/MCP split over a shared store. No change in this PR; whether it needs one is Lite's PLAN_REGRESSION read aggregates 14 days of Query Store intervals on every analysis pass #5823.

Rulings (decided):

  • R1: the watermark is on first_execution_time (it never changes and is the partition key), and the live floor keeps its one-day lookback for a day that is not built (an interval can cross midnight). Both stay as built in b5fba44.
  • R2 (superseded by fix lane A; this is the final shape): a closed day is built once through its end and is due again only when its late_seq moves. Today and yesterday (until its watermark reaches its end) are partial days: queue 2 builds one only when it has rows to add, and AdvanceIdleSql advances an idle one with no build. Tomorrow is not an empty build every hour: StampTomorrowSql stamps it with no totals, and it is built (queue 1) only when a row dated tomorrow starts below the watermark. The test this line once named was deleted by fix lane A; ThePlan_PutsALateClosedDayFirst_..._At200Servers and the merge prep's ThePlan_PutsALateClosedDayInTheCap_WithinTwoTicks_... pin the plan.
  • R3: see item 4.

Third lane (commit 3ee66a6), tests:

  • V185 rung pins: every probe-ordinal and mapping pin moves up one, the V183 pins (ReadCallerRungTests, ReadLatencyCapacityRungTests) relax to "below the top", RungRewindArtifacts.DropV185SentinelAsync drops built_through, and every rewind drops it. The migration-count and version-list pins count V185.
  • PlanRegressionDailyBuilderLiveTests: the builder plans T-14 through T+1 (16 days); the two-queue plan tests named under R2.
  • PlanRegressionDailyTriggerLiveTests: the steady rows are now first executions within the last 40 minutes (always above V185's hour-aligned bound). The recent edge is two hours before the current hour (review round 1), and the test now waits out the hour boundary as well as midnight (WaitPastTheHour, unit-tested).
  • PlanRegressionDailyParityLiveTests and PlanRegressionDailyReadShapeLiveTests bind $6 (each built day's watermark) and the floor from PlanRegressionLiveFloorWithWatermarks, as the collector does. PlanRegressionDailyRungTests binds and pins $5.
  • PlanRegressionCandidateReuseTests: the reuse window is one cadence and the worker sets it; a scheduled (refresh) pass reads live and refills, and the call after it reuses that fresh read.

Fourth lane (commit 21bcdfb; dev merged at 8765368):

  • No double count (diagnosis). The parity test did not get 12000 for 9000. Its hand-made build bound four parameters, and V185's BuildDaySql takes five (08P01: bind message supplies 4 parameters, but prepared statement "" requires 5, PlanRegressionDailyParityLiveTests.cs:334). The 12000 for 9000 was in PlanRegressionDailyReadShapeLiveTests.cs:96 and :175. The rows: 3 servers, 3,000 rows per server per day, T-14 through T-3 built, so the live floor is T-3 (one day below the first unbuilt day, T-2). The live half's scan reads server A's rows from T-3 up, four days, 12,000 rows. With <> ALL($3) the T-3 rows were dropped in the scan's own filter, so the scan returned 9,000. With b5fba44's NOT EXISTS (unnest($3, $6)) the planner ran the exclusion as a join above the scan: the scan returned 12,000 and the join dropped T-3's 3,000. The read's answer was right, as the parity test shows; only the scan's shape had changed. Rows read were 36,000 both ways.
  • The read. The exclusion is a scalar lookup in the scan's filter again: l.first_execution_time >= COALESCE(($6::timestamp[])[array_position($3::date[], l.last_execution_time::date)], '-infinity') (PgFactCollector.QueryPerf.cs:544). A built day keeps only its rows at or after its watermark. A day that is not built finds no watermark and keeps every row. The per-row cost is the same as <> ALL($3) (a search of at most 17 days), and no join strategy rests on the planner's estimate of an array. After the change the scan returns 9,000 on the heap and on day partitions, reads 36,000, and scans 7 of 21 partitions. With the NOT EXISTS put back, both read-shape facts fail with 12000, so they pin it.
  • Partial-day parity. In PlanRegressionDailyParityLiveTests, the test's build now binds $5 and stamps built_through, as the builder does. A new step also builds the empty days from the window's floor, so the live floor rises to the lowest watermark, as on a current store. It then builds three partial days. Day 2 is cut at 13:00, between its interval at 12:00 and the late interval at 14:00. Day 9 is cut at 11:00, so its regressed plans' intervals are all live, and only the interval that crossed midnight into it is in its totals. Day 10 is cut at its own midnight, so its totals hold only the interval that crossed into it from 23:40. The daily read equals the table read. Read as whole days, it does not, so the step is not vacuous. Each row is counted once: below its day's watermark in the totals, at or above it in the live half.
  • PlanRegressionInputsLiveTests (intervalTable: True, daily: False). Not a code failure. The class's cleanup did not remove the days its daily case built. A rerun on the same database found them valid under the next run's coverage claim, so the interval-table case took the daily path. It passed on a fresh database and failed on the rerun. The cleanup now deletes collect.plan_regression_daily and collect.plan_regression_daily_built for its servers, and two runs in a row on one database pass. Dev has the same gap; it showed here because the lanes reran on one database.
  • Pins. QueryStoreComposeStampTests: the "behind" and "at this rung" mappings clear the V185 sentinel (ProbeOrdinal + 11). QueryStoreIntervalPartitionRungTests (the V170 upgrade): the trigger text after migrating must be the V170 text with one change, V168's upper bound replaced by V185's, both as pg_get_triggerdef renders them. QueryStoreIntervalBoundedReadPruningLiveTests was not on the previous list; it failed with 42601: wrong number of parameters for prepared statement. Its daily build case now binds $5 (D + 1 day for a whole day) and its daily read case binds $6.

Fifth lane (commits c4594df, dbeeb3a, 0169dc4, 8de5251), tests:

  • The runtime zip claim was false. The zip has pgsql/share/extension/pg_stat_statements.control. On a fresh rig from the zip (port 5571, UTC, timescaledb,pg_stat_statements preloaded, CI's worker settings), pg_config --sharedir is the extracted share, the control file is under it, and CREATE EXTENSION pg_stat_statements gives 1.12. PlanRegressionDailyBuilderLiveTests: Total: 12, Failed: 0, including TheGc_ProbesTheTotalsByIndex.... The fourth lane's failure was its rig, not the zip.
  • PlanRegressionLiveFloorWithWatermarks unit cases (PlanRegressionDailyBoundaryTests, beside the existing live-floor cases, which live there and not in PgFactCollectorTests): nothing built (the window less one day), contiguous whole days (a day below the first unbuilt day; a watermark exactly at the next midnight is whole), partial days (the lowest partial watermark), a gap (the walk stops; a partial day after the gap takes no part), and the bound (built days below the window are ignored; the floor never falls more than one day below the window). The brief said "the lowest partial watermark, less the lookback"; the code takes no lookback below a partial watermark, and that is right: a built day's live rows are exactly those at or above its watermark. RED mutations: taking a day off below a partial watermark (floor = t.AddDays(-1)) fails 3 of the 5 new cases and none of the old; treating a watermark at the next midnight as partial (<=) fails 5; dropping the unbuilt lookback fails 9.
  • Half-stale, live (PlanRegressionDailyWatermarkLiveTests): a late UPDATE to two closed days goes through the real trigger, which marks each day and the day after it, so four of twelve built days go stale. Their totals are then tampered with (plan 1 made 1000 times dearer). The read takes totals for the 8 current days only, and its rows equal the interval-table read. Handed the stale days as valid, it differs, so the check is not vacuous. From EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON): the interval scan returns exactly the stale days' 1,600 planted rows, and the daily scan exactly the current days' 3,200 totals rows. RED: with built_seq = late_seq dropped from BuiltDaysSql, it fails.
  • Planted 14 days, three partial, live (same class, about 5,000 rows): closed days whole, T-2, T-1 and today built through 20:00, 18:00 and 06:00, plus an open hour of 400 rows after today's watermark. Every interval scan's Index Cond is first_execution_time >= '<T-2 20:00>'. The scan returns exactly the rows after each day's watermark (692, counted by a separate statement with the watermarks spelled out), and reads no more than the rows at or above the lowest watermark. The daily scan returns exactly the built days' totals. Sequential scans are off for this EXPLAIN, so the CI-sized table shows the index path. RED: with the pre-V185 exclusion (<> ALL($3)), the live half returns 0 of 692.
  • get_analysis_facts at 1 h, live (PlanRegressionCandidateReuseLiveTests): DarlingAnalysisService.AnalyzeAsync(..., 4, persistCollectionCaveats: true) (the worker's call) reads and fills the entry. Then CollectAndScoreFactsAsync(hoursBack: 1) (the tool's entry point, no as_of) makes no interval read. Proved by both pg_stat_statements (the plan_agg interval-read calls in this database are unchanged) and the cache's Hits counter (a test cache set on the service's own collector). With RefreshAfter set to zero, the next call reads fresh (+1 call, +1 miss). RED: with the facts call's ReusePlanRegressionCandidates forced false, the call count is 2, not 1. The test skips when pg_stat_statements is not preloaded. From 01:00 to 04:00 UTC, the 1-hour window starts a day later than the 4-hour pass's, so the shape differs and the call reads live; the test asserts that branch by the clock.
  • Census fit: SlowClassCensusTests, LiveClassStoreSkipCensusTests, ScratchDatabaseDisposalCensusTests, LivePostgresCollectionHygieneTests, LiveCleanupConversionRatchetTests, TimingTestCensusTests with the new class: Total: 50, Failed: 0.

Sixth lane (commits 06e1991, 48312da):

  • The one-hour reuse test's failure in the combined run: diagnosis. Two candidates were named. (a) Another class touched PlanRegressionCandidateCache.Shared: ruled out by the code. In the failing commit (0169dc4, line 231) the test already set its own cache instance on the service's collector (EngineFor(null).Collector, the same _sqlServerEngine that AnalyzeAsync resolves for this unregistered server), and Hits is that instance's counter. No other class can reach it, and Shared is never read by that collector after the setter. The collector already took its cache through the internal setter PlanRegressionCache, with Shared as the default that only the worker configures, so that part of the brief needed no product change. (b) pg_stat_statements evicted the read's entry: proved live in the combined run. On a fresh rig (pg_stat_statements.max 5000), one test alone makes about 900 entries (its scratch database's migrations), and one combined *PlanRegression* run made 151 and 165 deallocations in about 70 s, holding the table near 4,900 entries. Each deallocation drops the least-used 5% (about 250 entries; among one-call entries, the oldest), so an entry with one call lives on the order of ten seconds. The old check summed calls in a fresh database: 0 before the pass and 1 after it. If the entry was evicted and the call then read live, the new entry started at 1, so afterFacts == afterPass passed while a read ran. The old text filter also missed the raw read variant entirely (it required FROM query_store_interval_latest). Why the call missed the cache in that one run was not reproduced: three combined runs and one solo run with the hardened test were all green, and the rig's server log showed only the errors other tests provoke on purpose, plus one statement timeout in the run that added the command-timeout classes. If it recurs, the failure message now names the hits, the misses and every read entry by variant, which separates a changed shape (a miss plus a daily read), a changed source (a miss plus a table or raw read) and an entry that was never stored.
  • The fix (test only). The check reads every PLAN_REGRESSION read in the database (all three variants end FROM plan_agg), classifies each as daily, table or raw, and holds the pass's entry to its query id and stats_since. The pass's read must be exactly one daily entry of one call. After the 1-hour call the entries must be unchanged, the cache must show 1 hit and 0 misses, and an entry that is missing or remade fails the test instead of passing it. The refresh step must add one call to the same entry. The cache stays the test's own instance.
  • The work_mem bound. One plant of PlanRegressionDailyNoSpillRig (360,000 daily rows plus 24,000 open-hour rows), with every bound run in the same session: 4MB spills (an external merge of about 15MB, about 0.5 to 0.8 s). 32MB, 64MB and 128MB all hash in memory with no spill, in about 0.4 to 0.5 s, with hash aggregate peaks of about 35MB, 37MB and 42MB. The lowest bound that does not spill, 32MB, is below its own peak: it fits only because a hash may use twice work_mem (hash_mem_multiplier 2). By the rule (keep the next bound up when the lowest is within 20% of its peak), the bound is 64MB. The rig now runs the weighed bounds (32MB and the 128MB it replaced) on the same plant and asserts only the collector's own value. It was planted twice, once to measure 32MB and 64MB beside 128MB, and once to assert the 64MB constant. Each plant took about 7 s.

Test plan

  • Darling.Tests builds, 0 warnings, 0 errors.
  • Local rig (PostgreSQL runtime zip, UTC, port 5547). Before this lane, one run of *RungTests*, *PlanRegression*, ViewerSchemaVersionGateTests, CaggGroupIndexDropLiveTests, ReadLatencyFlushLiveTests and QueryStoreComposeStampTests: Total: 576, Failed: 42.
  • After the pins and R2/R3, the same set: Total: 578, Failed: 17.
  • After the last pin fixes, the 12 classes they touch (AwsPerServerRoleRungTests, PlanRegressionCandidateReuse*, PlanRegressionRowcountRungTests, QueryStoreComposeStampTests, QueryStoreWatermarksRungTests, RunnableQueueSettingsRungTests, PlanRegressionDailyRungTests, PlanRegressionDailyBuilderLiveTests, PlanRegressionDailyTriggerLiveTests, ReadCallerRungTests, ReadLatencyCapacityRungTests, ExternalAlertRungTests): Total: 105, Failed: 2. One is QueryStoreComposeStampTests (see Not done). The other, TheGc_ProbesTheTotalsByIndex..., fails on the rig only: pg_stat_statements was not available there.
  • Fourth lane, fresh rig (runtime zip, UTC, port 5561, timescaledb,pg_stat_statements preloaded, CI's worker settings): PlanRegressionDailyParityLiveTests, PlanRegressionDailyReadShapeLiveTests, PlanRegressionInputsLiveTests: Total: 11, Failed: 0; the inputs class alone a second time on the same database: Total: 8, Failed: 0. QueryStoreIntervalBoundedReadPruningLiveTests and QueryStoreComposeStampTests: Total: 22, Failed: 0.
  • After merging dev (89a266b), on a freshly created suite database: every *RungTests*, ViewerSchemaVersionGateTests, every *PlanRegression* class, QueryStoreComposeStampTests, QueryStoreIntervalBoundedReadPruningLiveTests, PgFactCollectorTests, QueryStoreBackgroundIndexesTests, CaggGroupIndexDropLiveTests, ReadLatencyFlushLiveTests: Total: 612, Failed: 1, Skipped: 1. The failure was PlanRegressionDailyBuilderLiveTests.TheGc_ProbesTheTotalsByIndex... with 0A000: extension "pg_stat_statements" is not available. That rig's cause is unknown: the runtime zip does have the control file (fifth lane, below), and the test passes on a fresh rig from the zip.
  • After merging dev: every Census* and Ratchet* class, GuardStageCensusTests, PlanForceNoWritePathTests, DocCommentHygiene*, LivePostgresCollectionHygieneTests: Total: 683, Failed: 0.
  • Fifth lane, fresh rig from the zip (port 5571): PlanRegressionDailyBuilderLiveTests: Total: 12, Failed: 0 (TheGc_ProbesTheTotalsByIndex... passes).
  • PlanRegressionDailyBoundaryTests: Total: 12, Failed: 0. PlanRegressionDailyWatermarkLiveTests: Total: 2, Failed: 0. PlanRegressionCandidateReuse* alone: Total: 8, Failed: 0. Each RED mutation above was run once and reverted.
  • PlanRegressionDailyNoSpillRig.Measure, run on the rig: before, spill yes (sort, external merge, about 16MB); after, spill no (about 0.4 s). The rig was planted twice (once before the fix, once for before and after in one plant), about 8 s each.
  • The fifth lane's combined run after the SET LOCAL commit (Total: 172, Failed: 1, Skipped: 2, the failure being GetAnalysisFactsAtOneHour... at cache.Hits) is superseded by the sixth lane's runs below.
  • Sixth lane, fresh rig from the runtime zip (port 5583, UTC, timescaledb,pg_stat_statements preloaded, CI's worker settings, pg_stat_statements 1.12, pg_stat_statements.max 5000). GetAnalysisFactsAtOneHour... alone: Total: 1, Failed: 0.
  • The combined run (every *PlanRegression* class, PgFactCollectorTests, QueryStoreComposeStampTests, QueryStoreIntervalBoundedReadPruningLiveTests) twice in a row with the hardened test: Total: 172, Failed: 0, Skipped: 2 both times (the two manual rigs). pg_stat_statements deallocations during the runs: 151 and 165.
  • PlanRegressionDailyNoSpillRig.Measure (DARLING_NOSPILL_RIG_PLANS=24000): 4MB spills; 32MB, 64MB and 128MB do not; the 64MB constant passes the rig's assertion.
  • After both commits, on a freshly recreated suite database: every *PlanRegression* class, PgFactCollectorTests, QueryStoreComposeStampTests, QueryStoreIntervalBoundedReadPruningLiveTests, StorageCommandTimeoutTests, StartupCommandTimeoutTests: Total: 211, Failed: 0, Skipped: 2 (PlanRegressionDailyTriggerCostRig.Measure and PlanRegressionDailyNoSpillRig.Measure, manual rigs). PlanRegressionTimeoutCaveatTests is in the *PlanRegression* set.
  • Then every *Census* and *Ratchet* class (including SlowClassCensusTests, LiveClassStoreSkipCensusTests, ScratchDatabaseDisposalCensusTests), GuardStageCensusTests, PlanForceNoWritePathTests, DocCommentHygiene*, LivePostgresCollectionHygieneTests: Total: 683, Failed: 0.
  • CI: it does not run on a draft, so the rig runs above are the only evidence until the PR is readied.

Fix lane B (review round 1)

Commits 463fc69 and c93e69c (M2, L1, L2, L3, L6, L7, L8), rebased onto fix A (1da1a92). The rig numbers below were measured before that rebase, at 48312da plus this lane; they were not run again after it. Rig: runtime zip, UTC, port 5689, timescaledb,pg_stat_statements preloaded, CI's worker settings, server work_mem 4MB.

  • M2, the fallback reads now run under the same work memory. The table read (no day built yet, or the built-days read failed) and the raw read (a server without coverage) used to run on an ordinary statement at 4MB. They now run in a short read-only transaction of their own, and a local function in the fact method (BoundTheReadAsync) runs SET TRANSACTION READ ONLY and PlanRegressionWorkMemSql on whichever transaction holds the read. The constant is still the one place that sets the value (renamed from PlanRegressionDailyWorkMemSql in the merge prep, L4). The transaction is disposed right after the read, as the daily snapshot is, so the plan reads and the next user of the pooled connection keep the server's value. Rig check (PlanRegressionDailyNoSpillRig.MeasureFallback, DARLING_NOSPILL_RIG_PLANS=24000: the S4 size, 15 days of 24,000 plans as interval rows plus the open hour, about 384,000 rows, no built day), PlanRegressionTableSql under EXPLAIN (ANALYZE, BUFFERS): before (4MB, no transaction) a Sort Method: external merge, about 5,000 temp blocks written and 5,000 read, about 0.9 s; after (the collector's transaction and SET LOCAL) no node spills, 0 temp blocks, about 0.6 s.

  • L1 and L3, the cache. Forget and its doc claim are gone. The doc now says a failed read stores nothing, so the older entry stays and serves until RefreshAfter from its own read. Store evicts every entry read RefreshAfter or more before the new one, and every entry of a server outside the set the service last published (RetainServers, called next to both _registryState.Publish calls in DarlingWorker, at start and on every reload). An entry is removed only if it is still the one judged (TryRemove(KeyValuePair)), so a concurrent store for that server is not lost. A test's own cache never publishes a set, so it evicts by age only. Every decision is logged at Debug with its reason (ReuseDecision: hit; no entry, not current, stale, watermark moved, shape differs), and the scheduled pass logs its refresh. The daily shape puts $6 (the watermarks) last, after its own separator, so a builder tick that moves a watermark is reported as WatermarkMoved. The table and raw shapes now carry the exact rawWindowStart, so a 1-hour call never takes a 4-hour pass's exact-bound read. One consequence: rawWindowStart moves with the clock, so an exact-bound read is in practice never reused. Reuse is now a daily-read matter, which is the case the issue measured.

  • L2. The reuse stays for an on-demand analyze_server without as_of. The cache doc's "What an operator sees" and the comment above if (context.PersistFindings) in DarlingAnalysisService both say that such a pass saves findings that may come from a read up to RefreshAfter old: the same band as a scheduled pass's own findings when the next call reads them.

  • L7. PlanRegressionDailyWatermarkLiveTests no longer asserts Index Cond text. It asserts rows from the plan's JSON nodes (returned plus removed by filter or recheck, times loops, exact with parallelism off): the interval scans read no more rows than start at or above the lowest watermark, they return exactly the rows after each day's watermark, and the daily scan returns exactly the built days' totals. No other executed-plan assertion in this PR reads Index Cond.

  • L6 and L8, the body. The memory figure is restated per node under "For the coordinator to double-check". The field figures are gone from the ENTRY, and "Why" cites the issue's figures as the issue's. "For built days" replaces "after the watermark". Item 6 names Lite's PLAN_REGRESSION read aggregates 14 days of Query Store intervals on every analysis pass #5823, and the ENTRY's work memory claim now covers both reads.

  • The issue's before and after (PlanRegressionDailyNoSpillRig.CompareWithDev). This is the S4 plant: 15 days of totals for 24,000 plans, today built through 10:00, and the open hour. On top of that, T-2, T-1 and today up to 10:00 carry an interval every four hours, about 360,000 more rows, so dev's read has its three open days to read. Dev's statement is the origin/dev prefix copied verbatim into the rig, plus the shared suffix, which this PR does not change. It gets dev's inputs: the 12 closed days and dev's live floor. All runs are on one plant, three runs each, rounded:

    Read work_mem Execution Shared blocks (hit + read) Temp blocks written
    dev (three open days read whole) 4MB, as dev runs it 2.5 to 4.6 s about 20,200 about 8,000 (two external merges)
    dev 64MB 1.1 to 1.4 s about 20,100 0
    this branch 64MB, as it runs now 1.3 to 1.6 s about 8,350 0

    Client wall time was within 0.3 s of execution time on every run. The partial days cut the read's buffers by about 60%. On this plant they do not cut its time: the 15 days of totals the branch groups (360,000 rows) cost about what dev's 12 days of totals plus three days of intervals cost. Against dev as it ships, the time falls because the read no longer spills. On a laptop rig the spread between runs is as wide as the difference between the two 64MB reads.

Tests (this lane). Darling.Tests builds with 0 warnings and 0 errors. PlanRegressionCandidateCacheEvictionTests (new: eviction by age and by server set, the entry just stored survives, every decision's reason, the moved watermark, the rawWindowStart miss, the work memory pin for both reads, no Forget and the worker's two RetainServers calls), PlanRegressionCandidateReuse*, PlanRegressionDailyWatermarkLiveTests and PlanRegressionDailyBoundaryTests: Total: 30, Failed: 0. The rigs MeasureFallback and CompareWithDev pass. Gate run before the rebase, on one database, every *PlanRegression*, *QueryStore*, *Census* and *Ratchet* class plus GuardStageCensusTests, PlanForceNoWritePathTests, DocCommentHygiene*, PgFactCollectorTests, StartupCommandTimeoutTests, StorageCommandTimeoutTests and LivePostgresCollectionHygieneTests: Total: 1996, Failed: 4, Skipped: 5. (1) TimingTestCensusTests was this lane's: the rig's Stopwatch. The rig now has an allow-list entry with its reason (it prints wall time and never asserts on it), and the class passes. (2) QueryStoreWatermarkReaderLiveTests (hypertable: True) timed out under load and passes alone (Total: 22, Failed: 1 for it and the writer class together, the one failure being (3)). (3) QueryStoreIntervalLatestWriterTests.APlanRegressionLateBatch_ThroughTheRealWritePath_MarksItsDays_AndSteadyBatchesLeaveTheBuiltTableEmpty fails alone too: steady batches marked today and tomorrow ([2026-10-14] = 1, [2026-10-15] = 1), in a run just after midnight UTC. (4) QueryStoreBackgroundIndexesLiveTests.ThePerDayBuildAndTheServerFloor_UseTheLatestBtree_OnceItIsBuilt: 08P01: bind message supplies 4 parameters, but prepared statement "" requires 5, so it binds PlanRegressionDaily.BuildDaySql with dev's four parameters. Neither class (3) nor (4) is in this PR's diff, and both read code V185 changed (the late-row trigger's bound and BuildDaySql's fifth parameter), so they look like this PR's fallout, not flakes. They are left to fix A or the coordinator: both sit on the builder and trigger, which are fix A's. After the rebase onto fix A: Darling.Tests builds with 0 warnings; the touched classes plus TimingTestCensusTests and PlanRegressionTimeoutCaveatTests: Total: 40, Failed: 0, Skipped: 2 (two of fix A's new watermark tests, which skip themselves).

Leftovers. None from this lane's items. Fix A owns H1, M1, M3, L4 and L5. For the coordinator: two failures in classes this PR does not touch (see the gate run above) look like V185 fallout and need an owner before CI.

Not done

None.

For the coordinator to double-check

  • R3 uses persistCollectionCaveats as the scheduled-pass marker ([BUG] MCP tool audit on 3.9.0-nightly.20261006.515: 23 defects (blocking, spills, FinOps, store, tool payloads) #5633 made it the one caller that sets it). A new scheduled caller must set it, or it would reuse.
  • RefreshAfter is process-wide (PlanRegressionCandidateCache.Shared). The worker sets it on every server's scheduling tick, from the live config value.
  • A build stamps its watermark from the store's clock (SeqSql), while PlanSql plans from the service's clock. A small skew between them can rebuild a day one extra time; it cannot leave a day stale.
  • The read pairs $3 and $6 by position (array_position). Both arrays are built from the one BuiltDaysSql list (PgFactCollector.QueryPerf.cs, the builtDays binds), and a day appears in it once (the built table's key is server and day).
  • SET LOCAL work_mem = '64MB' (bound 2, and since fix B every PLAN_REGRESSION read) is a per-node figure, not a per-read cap. Each sort or hash node of the read may take up to 64MB, and a hash node up to 64MB x hash_mem_multiplier (2) = 128MB. The daily read has six such nodes: the inner live HashAggregate, the outer plan_agg aggregate, plan_dedup (array_agg ... ORDER BY), two window sorts and the final sort. The measured plant used about 37MB in each of its two hash aggregates. Scheduled reads run inside each server's sweep, under the fleet sweep gate (DarlingWorker.ProcessServerSweepAsync, which holds a permit through RunScheduledAnalysisAsync), so as many run at once as MaxConcurrentSweeps allows: 4 by default (DarlingConfig.MaxConcurrentSweeps), at most 16 (StoreConfigProvider.MaxConcurrentSweepsLimit). On-demand analyze_server and get_analysis_facts calls that miss the cache run outside that gate. Nothing caps the product of nodes, bound and degree.
  • The 1-hour reuse holds only when the 1-hour and 4-hour windows start on the same UTC day (outside 01:00-04:00 UTC); otherwise the call reads live. That is correct, but it saves less.
  • Rung: Alerts hand a Level 1 investigation to your own agent (#5632 Phase A) #5819 merged to dev first with its own V184 (the investigation tables), so this PR's rung is V185, after it. The renumber is listed under ## Merge prep (round 2 Lows, V185).

Fix lane A (review round 1)

Commit 1da1a92 (one commit; pushed to the issue branch). Fix B's files (PgFactCollector.QueryPerf.cs, PgFactCollector.PlanRegressionCache.cs, the reuse tests) and the ENTRY are untouched.

Why two stamps exist that are not builds (approved by the coordinator, outside the cap): the live floor (PlanRegressionLiveFloorWithWatermarks) is the lower of the lowest partial watermark in the contiguous built run and the first unbuilt day less one day. So (a) a tomorrow that is never built puts the floor at today's midnight (the live half reads all of today, not the last 2-3 hours), and (b) a partial day of a server that goes idle in the evening never gets a row past its watermark, stays partial for good, and holds the floor there for 14 days (the #5812 cost again).

  • H1 (ruling 1). PlanRegressionDaily.PlanSql now plans in two queues. Queue 1: days with no valid build (never built, or late_seq moved), any day of the series, day DESC, server_id, at most MaxBuildsPerTick (120). Tomorrow is in it only when a row dated tomorrow starts below the watermark. Queue 2: valid partial days whose watermark can advance AND that have a row of the day with first_execution_time in [built_through, new watermark), ordered built_at (oldest first), at most the number of builder servers. Both under MaxTickDuration. Before the plan, the tick runs AdvanceIdleSql (a valid partial day with nothing in that span gets the new watermark, or a whole stamp at its end) and StampTomorrowSql (tomorrow with no valid build, no totals and no row dated it below the watermark is stamped with no totals). Both read the store's clock as SeqSql does, and both write only while late_seq has not moved (the advance's partial_seq = late_seq is rechecked on the locked row; the stamp takes the snapshot's late_seq and its conflict arm writes only while the row still holds it and holds no valid build). TickResult gains Advanced and Stamped.
    • Live: PlanRegressionDailyBuilderLiveTests.ThePlan_PutsALateClosedDayFirst_ThenEveryServersTodayOldestBuildFirst_AndNoEmptyTomorrow_At200Servers (replaces the R2 test): 200 servers, one late closed day on server 150; the plan is that day, then all 200 todays in built_at order, no tomorrow; then a row dated tomorrow below the watermark on server 7 puts (7, tomorrow) first in queue 1.
    • Live (coordinator condition 1): PlanRegressionDailyWatermarkLiveTests.ATick_AdvancesAnIdleDayWithoutABuild_StampsTomorrow_AndTheFloorIsTodaysWatermark: idle server A's today carries the new watermark with no plan slot; A's T-2 (partial at 22:00) is stamped whole and its read equals PlanRegressionTableSql; tomorrow is stamped partial with no totals; A's floor equals today's watermark; the plan is exactly server B's today.
    • Count pins in the builder class: a tick builds 15 days per server (T-14 through T), tomorrow is stamped.
  • M1 (ruling 2). V185 changed in place: adds partial_seq with built_through. MarkBuiltSql: whole build stamps built_seq, clears partial_seq and built_through; partial build stamps partial_seq and built_through, built_seq NULL. BuiltDaysSql: whole when built_seq = late_seq (watermark NULL, whatever built_through holds), partial at built_through when partial_seq = late_seq. RungRewindArtifacts.DropV185SentinelAsync drops both columns; comment corrected. The probe sentinel stays built_through.
    • Pins: PlanRegressionDailyRungTests.V185_AddsBothPartialDayColumns_... and TheStampAndTheBuiltDaysRead_KeepAPartialBuildInvisibleToPreV185Code.
    • Live: PlanRegressionDailyWatermarkLiveTests.APartialDay_IsUnbuiltToTheV183Predicate_AndAV183WholeBuildAfterItReadsWhole_WithNoDoubleCount.
    • The parity, watermark and no-spill test stamps now write partial days as the builder does (PartialAwareStamp).
  • M3 (ruling 3). PlanRegressionDaily.LateTriggerLagHours = 1; V185's trigger SQL interpolates it (V185Sql is now static readonly, same rendered text). Pinned pair: PartialWatermarkLagHours >= LateTriggerLagHours + 1. Live: ABuildOnTheStoreClock_StampsTwoHoursBeforeTheHour_AndAWriteBelowAnAdvancedWatermarkMarksTheDay: a real BuildDayAsync stamps date_trunc('hour', now() AT TIME ZONE 'UTC') - 2h; set an hour back, the idle advance restamps it (coordinator condition 2); a row at W - 30 min through the real trigger moves late_seq; the daily read equals PlanRegressionTableSql. The stamp tests skip before 04:00 UTC and wait out an hour boundary under 3 minutes off.
  • L4 (ruling 4). Class doc, ClosedDayLagDays (now "a test pin only"), BuildDaySql, MaxBuildsPerTick, MarkBuiltSql, BuiltDaysSql, PlanSql, RunTickAsync, V185's doc.

Tests (rig: runtime zip, UTC, port 5647, timescaledb,pg_stat_statements preloaded, CI's worker settings):

  • Darling.Tests builds, 0 warnings.
  • Every *PlanRegressionDaily*, *PlanRegressionBuiltDays*, *PlanRegressionInputs*, *RungTests*, *Census*, *Ratchet*, PlanForceNoWritePathTests, *DocCommentHygiene*, QueryStoreComposeStampTests, ViewerSchemaVersionGateTests, CaggGroupIndexDropLiveTests: Total: 1147, Failed: 2, Skipped: 2. The two (TimingTestCensusTests: a Stopwatch in the 200-server test; DocCommentHygieneTests: a displaced summary) were fixed; rerun of those two plus the three touched PlanRegressionDaily classes: Total: 78, Failed: 0. The stamp tests ran (store clock 23:xx UTC) and passed.

Leftovers (handoff; the watchdog's 200k mark was reached):

  • M3's RED plant (PartialWatermarkLagHours = 0) was not run. Expected red line: the built_through assertion in ABuildOnTheStoreClock_... (PlanRegressionDailyWatermarkLiveTests.cs, the first Assert.Equal after BuildDayAsync).
  • The plan's wall time at 200 servers was not captured (the Stopwatch was removed for TimingTestCensusTests).
  • L5 (ruling 5, PlanRegressionCandidateReuseTests one-hour test: capture the pass's window end, compute both floors with PlanRegressionWindowFloor, skip when they cross; band 01:00-04:00 UTC) not done.
  • SlowClassCensusTests, LiveClassStoreSkipCensusTests, ScratchDatabaseDisposalCensusTests ran inside the *Census* filter above (green); no new class was added.
  • Not run: PlanRegressionCandidateReuse*, PgFactCollectorTests, QueryStoreIntervalBoundedReadPruningLiveTests (fix B's area; BuiltDaysSql changed its second column, so they read it).

Fix lane A2 (review round 1)

Commits 88baf9f, 0a726fc, 5894832, bcbea42 (pushed to the issue branch; fix B's files and the ENTRY untouched). Rig: runtime zip, UTC, port 5648, timescaledb,pg_stat_statements preloaded, CI's worker settings. The store clock read 00:09-00:40 UTC during the run.

  1. The classes fix A did not run. PlanRegressionCandidateReuse*, PgFactCollectorTests, QueryStoreIntervalBoundedReadPruningLiveTests: Total: 28, Failed: 0 (no pin moved; PgFactCollectorTests does not name BuiltDaysSql). The two the coordinator added (88baf9f):
    • QueryStoreBackgroundIndexesLiveTests.ThePerDayBuildAndTheServerFloor_... bound BuildDaySql with 4 parameters (08P01: bind message supplies 4 parameters, but prepared statement "" requires 5). It now binds V185's watermark, D + 1 day (a closed day, a whole build).
    • QueryStoreIntervalLatestWriterTests.APlanRegressionLateBatch_...: the test was wrong, not the trigger. Its "steady" row started at now - 3h, which is below V185's bound (date_trunc('hour', now()) - 1h) at every hour, not only after midnight: V185 moved the bound from yesterday's midnight to the open days on purpose, and a row that old changes a partial day's totals, so marking it is required. The steady rows now start at the current hour's start (SteadyFirst), which stays at or above the bound even if the hour rolls over once mid-test; the fault-arm test's steady rows (now - 1h, a seconds-wide race at the hour) take the same shape. New TheLateBatchTestsRows_HoldAgainstTheShippedTriggerBound_AtEveryHourOfAPinnedClock reads the trigger's WHEN back from the migrated store (pg_get_triggerdef), pins now() to every hour of a day at :00:00 and :59:59, and asserts steady rows never fire it (also one hour later), the late batch always does, and a now - 3h row always does. Both classes: Total: 17, Failed: 0 at 00:2x UTC.
  2. M3 RED plant. The seam test skipped before 04:00 UTC, so it could not run tonight; it now uses the watermark's own day (yesterday before 02:00 UTC, when the watermark is 22:00 or 23:00 of yesterday, also a partial day) and skips only from 02:00 to 04:00 UTC, when no partial day can hold rows two hours below the watermark (0a726fc). Green at 00:3x. With PartialWatermarkLagHours = 0 (reverted): PlanRegressionDailyWatermarkLiveTests.cs(370) (the built_through assertion after BuildDayAsync) Assert.Equal() Failure: Strings differ, Expected: "2026-10-14 22:00:00", Actual: "" (lag 0 at 00:xx makes the build whole, so built_through is NULL; after 04:00 the actual is the current hour).
  3. Plan time at 200 servers (the 200-server test's seed, EXPLAIN (ANALYZE, BUFFERS) in a rolled-back transaction, 3 runs, a temporary probe not committed): PlanSql about 12 ms (planning under 1 ms, 201 rows), AdvanceIdleSql about 5 ms, StampTomorrowSql about 2 ms warm (10 ms cold). Far under 2 s; no finding.
  4. The lock-free stamps (bcbea42). New PlanRegressionDailyWatermarkLiveTests.TheTicksStampsDuringAHeldPartialBuild_LeaveTheDayConsistent_AndTomorrowWithoutTotals: a partial day set an hour back; a builder holds server A's lock and has read late_seq and the watermark (BuildDayAsync's afterSeqRead hook), and a second connection runs the tick's stamps (StampWithoutBuildAsync); the idle advance restamps the day (advanced = 1); the build finishes. Asserted: built_through = the watermark, partial_seq = late_seq, built_seq NULL; the daily read (totals plus live rows after the watermark) equals PlanRegressionTableSql; no totals for any day after today. ASecondBuilder_...'s loosened assertion (built_seq IS NOT NULL AND day <= 03-20 is 0) is replaced by the exact one: the one stamped day is tomorrow (03-21) with source_rows 0 (and the totals count above is 0). Race argument: neither stamp changes late_seq or any totals, so the held build's stamp (partial_seq = the late_seq it read) stays valid and its totals cover exactly the rows below its own watermark; a stamp that lands after the build commits re-checks partial_seq = late_seq and the no-rows-in-span test against the committed row, and a row written into the span after the stamp's snapshot sits below the trigger's bound, so its late_seq bump makes the day stale rather than silently advanced. Not RED-planted (not asked); the interleaving tested is the one the hook offers (stamps between the seq read and the aggregate), not stamps blocked on the build's row lock after MarkBuiltSql.
  5. L5 (5894832). The one-hour reuse test reads the clock before and after the pass and before and after the call, computes each window's floor with PlanRegressionWindowFloor at both ends, skips when either pair crosses a midnight, and compares the two floors. The comment's band is 01:00-04:00 UTC (00:00-01:00 both windows start on the day before). Ran at 00:28 UTC (same-day branch): Total: 8, Failed: 0. The PR body's older text "outside 00:00-04:00 UTC" sits under 'For the coordinator to double-check' (line 113 of the body); the band there should read 01:00-04:00 UTC.

Tests: Darling.Tests builds, 0 warnings. *PlanRegression*, *RungTests*, *Census*, *Ratchet*, GuardStageCensusTests, PlanForceNoWritePathTests, *DocCommentHygiene*, QueryStoreIntervalLatestWriterTests, QueryStoreBackgroundIndexesLiveTests, PgFactCollectorTests, QueryStoreIntervalBoundedReadPruningLiveTests, LivePostgresCollectionHygieneTests, ViewerSchemaVersionGateTests: Total: 1270, Failed: 0, Skipped: 5 (the three no-spill rig measures and the trigger-cost rig, opt-in; ATick_AdvancesAnIdleDayWithoutABuild_..., which skips before 04:00 UTC).

  • Not run at the 02:00-04:00 or 04:00+ UTC hours on this rig: the seam and stamp-race tests ran only in the yesterday's-partial-day branch (00:xx UTC); CI runs them at its own hour.
  • No full suite (CI decides).

Merge prep (round 2 Lows, V185)

Merged origin/dev (ae0fca7, which carries #5819's V184) with a merge, not a rebase. 18 files conflicted, all at the top of the ladder; each kept both sides.

  • V185 renumber. PgMigrations: dev's V184Sql (investigations) stays V184; this PR's is V185Sql, new Migration(185, "plan-regression-partial-days", ...) after it. StorageVersion.SchemaVersion = 185. ViewerDataService: the built_through probe line follows the investigation line, GetBoolean(160) added, MapProbedSchemaVersion(..., hasInvestigations, hasPlanRegressionPartialDays), the return 185 arm above return 184. PlanRegressionDaily.cs doc mentions say V185. RungRewindArtifacts: DropV184SentinelAsync is dev's (investigations); DropV185SentinelAsync drops built_through and partial_seq, and every rewind that drops V184 drops V185 first (CaggGroupIndexDropLiveTests gains a simulatedVersion < 185 step). Rung pins moved in their house form: every ProbeOrdinal + N last-ordinal pin gains one ordinal; parameters[^N] lists gain hasPlanRegressionPartialDays at [^1]; the "absent" cascades gain a withoutV185 step; the version lists and migrate counts count V185 (QueryStoreIntervalPartitionRungTests 15 rungs, Scripts[^14]/[^15]; QueryStoreWatermarksRungTests 178-185). Dev's own InvestigationRungTests moved too: V184 is now just below the top (Scripts[^2], parameters[^2]), its live upgrade drops V185's sentinel and expects both rungs to apply. Test renames: V185_AddsBothPartialDayColumns_..., ..._InvisibleToPreV185Code.
  • L1. The daily read's SQL comment (PgFactCollector.QueryPerf.cs) now states the rule: a day not built is all live, a built day keeps only its rows at or above its watermark ($6) live. This changes the statement text, so its pg_stat_statements query id. PlanRegressionLiveFloor's doc says it is the whole-day case of PlanRegressionLiveFloorWithWatermarks and that only tests call it.
  • L2. Queue 2's cap is documented as the SQL has it, in all three places (MaxBuildsPerTick, PlanSql, PlanBuildsAsync): the queue's total is capped at the number of builder servers, not one per server. The SQL is unchanged.
  • L3. This body: the status banner, "01:00-04:00 UTC" (two places), open days no longer rebuilt every tick, validity built_seq = late_seq OR partial_seq = late_seq, the caps as they are, the R2 line and the deleted test's name, the rung line, and the ENTRY ("adds the partial-build columns"). Every V184 that named this PR's rung now says V185.
  • L4. PlanRegressionDailyWorkMemSql is PlanRegressionWorkMemSql, in the production file and its three test files.
  • L5. Queue 1 orders by built_at ASC NULLS FIRST, day DESC, server_id: a never-built day first (so a store that is behind still gets its newest days first), then the oldest build. A server whose today goes stale on every apply rebuilds it every tick, so its built_at keeps moving, and a late-marked closed day passes it. New live test ThePlan_PutsALateClosedDayInTheCap_WithinTwoTicks_When130ServersHaveTodayLateEveryTick: 130 servers with today late-marked (built 10:00-10:59), one closed day late-marked on server 131 (built 11:00); tick 1 fills its 120 slots with todays, those are built and late-marked again, and tick 2 has the closed day at slot 11. RED: with the old day DESC, server_id order, tick 2 does not contain it (Assert.Contains() Failure: Item not found). The source-text pin of the old ORDER BY in TheConstantsAndStatements_AreTheDocumentedOnes is deleted, not moved: its rule was spelling, and the live test now pins the order by behavior.

Results (this branch after the merge, local rig on PostgreSQL with TimescaleDB, UTC):

  • *RungTests, ViewerSchemaVersionGateTests, *PlanRegression*, CaggGroupIndexDropLiveTests, *Investigation*, QueryStoreBackgroundIndexesLiveTests, QueryStoreIntervalLatestWriterTests, QueryStoreComposeStampTests, *Census*, *Ratchet*, GuardStageCensusTests, PlanForceNoWritePathTests, *DocCommentHygiene*, *MigrationDataMoving*: Darling.Tests Total: 1385, Errors: 0, Failed: 0, Skipped: 4 (the four skips are the opt-in measuring rigs).
  • PlanRegressionCandidateReuseLiveTests.GetAnalysisFactsAtOneHour_... failed once in a run of the plan regression classes ("the pass's pg_stat_statements entry is gone or was remade") and passed in the run above; its own message says the check could not speak, so I read it as load, not code. CI decides.
  • Pre-push gate:
  • [guard-darling] Darling.Tests Total: 4610, Errors: 0, Failed: 0, Skipped: 0
    • [guard-lite] Lite.Tests Total: 1281, Errors: 0, Failed: 0, Skipped: 0
    • classes: darling 282, lite 31: darling Total: 1507, Failed: 0 and Total: 1825, Failed: 0; lite Total: 550, Failed: 0. GATE GREEN.
    • A first gate run was red on one guard test, OversizedPlanBacklogPins.V121_...: the merge resolution had left PgMigrations.cs with LF line endings in the working copy, so a verbatim SQL string held LF where the pin expects the checkout's CRLF. Git stores both the same (the commit carries no line-ending change); restoring CRLF in the working copy made it green.

Fix lane (review round 1)

All four findings of review 5528556062 and the title divergence, in commit 78ec763 (one push). Rig: PostgreSQL from pg-runtime.zip, UTC, CI's preload and worker settings.

  1. Spelling pins deleted (finding 4279338315). Gone: PlanRegressionCandidateCacheEvictionTests.EveryPlanRegressionRead_RunsUnderTheOneWorkMemSetting (twin: PlanRegressionDailyNoSpillRig.MeasureFallback), TheCacheHasNoForget_AndTheServicePublishesItsServerSet (twin: AStore_EvictsTheEntriesOfServersNoLongerMonitored_OnlyOnceASetIsPublished), and PlanRegressionDailyRungTests.TheStampAndTheBuiltDaysRead_KeepAPartialBuildInvisibleToPreV185Code (twin: PlanRegressionDailyWatermarkLiveTests.APartialDay_IsUnbuiltToTheV183Predicate_AndAV183WholeBuildAfterItReadsWhole_WithNoDoubleCount). For AnExactBoundRead_WithADifferentWindowStart_IsADifferentShape I deleted only its three source-text lines (the FactSource() asserts), because the twin the finding names is that member's own pure-rule assertions, which stay. The Count, FactSource and Root helpers and two usings went with them. The lag change also broke two '2 hours' SQL-text lines in PlanRegressionDailyBuilderLiveTests' statement pin; they were spelling too, so I deleted them (the watermark is pinned by behavior in ABuildOnTheStoreClock_StampsThreeHoursBeforeTheHour_...). The probe-ordinal and migration-literal rung pins stay.
  2. The trigger's skew margin (finding 4279340553). LateTriggerLagHours 1 -> 2 and PartialWatermarkLagHours 2 -> 3, changed in place in the unreleased V185. Both constants and V185's doc comment now state the bet: the trigger compares the monitored server's first_execution_time with the store's now(). A store clock up to an hour ahead stays quiet with 60-minute intervals. A bigger skew, or longer intervals, puts the open days back on the live read: slower, with the same numbers. The cost is an hour of live-half width (the last 3-4 hours of first executions instead of 2-3). New live test QueryStoreIntervalLatestWriterTests.TheJustEndedIntervalsRows_AtTheOldBoundAndASecondBelow_StayQuietUnderAStoreClockAhead_AndARowBelowTheBoundFires runs on a pinned clock at every hour against the shipped trigger. A row at the just-ended interval's start (the old bound) and one a second below it stay quiet, including with the store's clock 2 s into the next hour. A row at the new bound stays quiet, and one a second below it fires. SteadyFirst is now the just-ended interval's start (the old bound), so the existing pinned-clock test's "hour rolls over" case sits on it. Moved with the lag: PlanRegressionDailyTriggerLiveTests' recent edge (-2 h), QueryStoreIntervalPartitionRungTests' V185 bound literal ('02:00:00', a rung pin), the rung's lag pins (2, 3), PlanRegressionDailyWatermarkLiveTests (the watermark now reads the constant, its skip hours moved to 03:00-05:00 and < lag + 2), and the 200-server plan test (fixture an hour earlier). Reverted to 1/2, three tests fail: the new test, TheLateBatchTestsRows_HoldAgainstTheShippedTriggerBound_AtEveryHourOfAPinnedClock, and PlanRegressionDailyTriggerLiveTests.AFirstBeforeTheDayAlignedSeventeenDayEdge_MarksNothing_AndTheEdgesAreExact (Total: 21, Failed: 3). With the fix: Failed: 0.
  3. AnalysisContext doc (finding 4279341518). ReusePlanRegressionCandidates now names only analyze_server and get_analysis_facts with no as_of, and says the scheduled pass never sets it and sets RefreshPlanRegressionCandidates instead. Doc only; no test needed.
  4. work_mem ceiling (finding 4279342759). Added above PlanRegressionWorkMemSql: the figure is per node, so one read can take 6 x 64MB, doubled for a hash through hash_mem_multiplier (768MB). The fleet ceiling multiplies that by MaxConcurrentSweeps (4 by default, at most 16). On-demand reads that miss the cache run outside the sweep gate. Size MaxConcurrentSweeps before raising the bound. Doc only.
  5. Title and ENTRY. The title and the ENTRY's bold line now name what ships: one analysis cadence of reuse, not six hours. "What changes" and the ENTRY now give the new figures (watermark three hours back, trigger bound two hours back).

Results. Gate (handoffs pre-push.sh; the branch has no .github/scripts/pre-push.sh): GATE GREEN. [guard-darling] Total: 4610, Errors: 0, Failed: 0, Skipped: 0; [guard-lite] Total: 1281, Errors: 0, Failed: 0, Skipped: 0; classes: darling 287, lite 33; classes Total: 1495, Failed: 0, Skipped: 29, Total: 1837, Failed: 0, Skipped: 31, Lite Total: 557, Failed: 0, all with the rig. The first gate run was RED on one ScratchDatabaseSweepLiveTests.ADatabaseNobodyDisposed_IsDroppedByTheDrain_AndForgottenAfterwards (a file this PR does not touch). Run alone it passed (Total: 4, Failed: 0), and the full rerun was green. The brief's classes outside the gate's selection (AnalysisContextSecondaryScopeSourceScanTests, DarlingAnalysisServiceEngineRoutingTests, PlanRegressionDailyTriggerCostRig, PlanRegressionInputsAdviceTests, PlanRegressionIntervalTableEquivalenceTests, PlanRegressionReadShapeTests, QsCaptureModeRungTests): Darling Total: 51, Failed: 0, Skipped: 1.

  • Not run: Lite's own QsCaptureModeRungTests (not in the gate's selection; this PR changes nothing Lite reads).
  • Coordinator: the older sections above (Fix lane A2, lines on - 2h and the 02:00-04:00 skip) describe the one-hour lag; this section supersedes them.

CI red (reuse test)

Cause: the test, not the product (case a). CI run 38702741139 failed PlanRegressionCandidateReuseLiveTests.GetAnalysisFactsAtOneHour_AfterTheScheduledPass_ReusesItsCandidates_AndReadsFreshOncePastRefreshAfter on its own pg_stat_statements check. The line of evidence is in its message: hits 1, misses 0; entries after the pass [... Calls = 1 ...], after the call []. The product's cache counted a hit and no miss, so it reused. The entry list after the call is empty, not remade (a re-read after a reset would have left a fresh entry with one call). Something removed the entry, and the product made no read after that. pg_stat_statements is cluster-wide. This class is #1776 own-store and has no collection, so it runs in parallel with the rest of the shard. PgStatementsDeallocLiveTests (also parallel) runs more distinct statements than pg_stat_statements.max to force eviction, and every parallel scratch database that runs the migrations adds its own entries, because the key includes the database. Eviction drops the least-used entries first, and the pass's read has one call. (ComposeStoreRolesLiveTests resets the whole cluster too, but its collection runs alone, so it is not the cause here.)

Fix (commit d334868, test only). The test now proves the reuse the way the class's first test does, through what the product reports. After the scheduled pass, it deletes the costly plan's rows from query_store_interval_latest and plan_regression_daily. On the same-day shape, the 1-hour get_analysis_facts call must still report PLAN_REGRESSION (it can do that only from the pass's candidates), with the test's own cache at hits 1, misses 0. On the cross-day shape, it must miss, read live and find none. Past RefreshAfter (set to zero), the call reads fresh: one more miss, and no PLAN_REGRESSION. The pg_stat_statements read-back, its helper record and the shared_preload_libraries skip are gone, so the test now also runs on a server without pg_stat_statements. No product change.

Verified on a UTC rig (port 5681, CI's preload and worker settings):

  • *PlanRegressionCandidateReuse* 5 runs in a row: Total: 8, Errors: 0, Failed: 0, Skipped: 0 each time (18.7 to 21.1 s). The runs fell between 08:00 and 10:00 UTC, so they exercised the same-day reuse branch.
  • The same class, with a loop resetting pg_stat_statements for the whole cluster every 200 ms (a stand-in for eviction: both remove the entry) (95 resets in the first loop): new test Total: 8, Failed: 0. The OLD test, rebuilt with the same loop running: GetAnalysisFactsAtOneHour_... [FAIL], Total: 8, Failed: 1. That reproduces CI's failure and shows the fix holds under the same condition.
  • Two-shard shape: not needed. The class uses its own scratch database, and no sibling class shares a store with it.
  • Gate bash .github/scripts/pre-push.sh: GATE GREEN. guard-darling Total: 4613, Failed: 0; guard-lite Total: 1281, Failed: 0; classes: darling 282, lite 32; classes-darling Total: 1479, Failed: 0, Skipped: 29 and Total: 1821, Failed: 0, Skipped: 31; classes-lite Total: 522, Failed: 0.
  • Every *PlanRegression* class: Total: 147, Errors: 0, Failed: 0, Skipped: 4 (the 4 skips are the opt-in measurement rigs PlanRegressionDailyNoSpillRig and PlanRegressionDailyTriggerCostRig).
  • CI decides.

For the coordinator: the test's new comment names two ways the entry vanishes: classes that call pg_stat_statements_reset() for the whole cluster, and classes that fill the hash table until it evicts. Only the second can overlap this class: the one full reset is in ComposeStoreRolesLiveTests, whose collection runs alone. The brief allowed one push, so that half-sentence is still in the comment. It is the next lane's one-line fix if you want it.

SECTION: Fixed
ENTRY: - Plan regression reads no longer re-read three open days of Query Store intervals, and on-demand calls reuse the candidates for one analysis cadence ([#5817]) - The PLAN_REGRESSION read took closed days from stored daily totals but read every interval row of the last three days again for every server on every pass. The hourly builder now also builds the open days as partial days, up to a watermark three hours before the current hour. For every built day the read takes the stored totals and reads only the interval rows after that day's watermark. An analyze_server or get_analysis_facts call with no as_of reuses the scheduled pass's plan regression candidates for up to one analysis cadence (30 minutes by default), and an analyze_server call saves the findings it builds from them. The scheduled pass always reads fresh. Every plan regression read, including the full-window read a server uses before its days are built, raises its work memory to 64MB for its own transaction, so neither read sorted to disk on the planted busy server. Migration V185 adds the partial-build columns and moves the late-row trigger's bound to two hours before the current hour.

…ewest days first (#5812)

The scheduled pass re-ran the 14-day plan_agg read for every server on every
pass. A current pass now reuses the candidate rows of a read of the same shape
(window day, built days, scope) for up to six hours; the #5630/#5631 input check,
the cut and the fact still run every pass. get_analysis_facts and analyze_server
with no as_of reuse them too; a past as_of reads live. The daily builder now
plans the newest closed days first, so a store that is behind gets back the days
nearest the open end first.
…ewest days first (#5812)

The scheduled pass re-ran the 14-day plan_agg read for every server on every
pass. A current pass now reuses the candidate rows of a read of the same shape
(window day, built days, scope) for up to six hours; the #5630/#5631 input check,
the cut and the fact still run every pass. get_analysis_facts and analyze_server
with no as_of reuse them too; a past as_of reads live. The daily builder now
plans the newest closed days first, so a store that is behind gets back the days
nearest the open end first.
…ution watermark (#5812)

V184 adds plan_regression_daily_built.built_through and moves the late-row
trigger's upper bound to one hour before the current hour. The builder builds
through tomorrow; an open day is rebuilt at every hourly tick to a watermark two
hours before the current hour by the store's clock. The daily read takes the
totals for every valid built day below its watermark and the live interval rows
after it, so on a current store the live half reads the last two to three hours
of first executions instead of the three open days.

Checkpoint: rung pins, the live classes that bind the daily read's parameters,
and the new live tests are not updated yet (see the PR body).
…ntal' into fix/5812-plan-regression-incremental

# Conflicts:
#	Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs
#	Darling/PerformanceMonitor.Darling.Storage/PlanRegressionDaily.cs
…SION candidates for one analysis cadence (#5812)

V184 rung pins: every probe-ordinal and mapping pin moves up one, the V183 pins relax to below the top, the rewinds drop
the built_through sentinel, and the migration-count pins count V184. The builder tests plan T-14 through T+1, and a new
live test pins ruling R2: only a day whose watermark can still advance (today, tomorrow) is due on the hourly tick, and a
complete day inside the closed-day lag is due again only after its late_seq moves. The trigger tests follow V184's
hour-aligned bound, with an hour guard. The hand-bound daily reads pass the watermark array.

Ruling R3: the candidate reuse window is one analysis cadence, set by the worker from the configured interval (default
30 minutes). The scheduled pass never reuses; it always reads and refills the entry, and MCP and viewer calls with no
as_of reuse that read within one cadence.
… and move the V184 pins (#5812)

The daily read's live half excluded built rows with NOT EXISTS over unnest($3, $6), which the planner runs as a
join above the interval scan, so the scan returned the floor day's rows and the join dropped them. It is now a
scalar lookup in the scan's filter, as #5448's <> ALL($3) was. The parity live test binds the build's watermark
and checks three partial builds against the table read; the inputs live test removes the days it built; the
compose-stamp, trigger-text and pruning pins take V184.
…5812)

One case per branch of PlanRegressionLiveFloorWithWatermarks: nothing built, contiguous
whole days, partial days, a gap, and the bound below the window floor.
…ad from its executed plan (#5812)

Half-stale: a late write marks four closed days stale through the real trigger; the read takes
totals for the current days only, reads the stale days' intervals, and equals the table read even
with the stale totals tampered. Fourteen days with three partial: the interval scan's index
condition starts at the lowest watermark and each half returns exactly its planted rows.
…PLAN_REGRESSION read (#5812)

pg_stat_statements' calls for the interval read and the cache's hit counter: no interval read
after the scheduled pass, and one fresh read once RefreshAfter has passed.
…ork_mem, and add the no-spill rig (#5812)

At the default 4MB the read sorted about 384,000 planted rows to disk; with 128MB set for the
snapshot transaction only, the plan hashes in memory. The command-count pin counts the new command.
… every read variant (#5812)

The test counted calls of the interval-table read by text. In a combined run the cluster evicts
pg_stat_statements entries constantly (pg_stat_statements.max 5000; one combined run made about
150 deallocations), so an evicted entry remade by a live read starts over at one call and the
count can stand still while a read ran. The raw read's text did not match the filter at all.
The check now reads every PLAN_REGRESSION read (all three end FROM plan_agg), holds the pass's
entry to its query id and stats_since, fails when that entry is missing or remade, and also
asserts the test's own cache made no miss; a failure message names hits, misses and every entry.
…easured value with headroom (#5812)

On one plant of the no-spill rig (15 built days, 360,000 daily rows, one open hour of 24,000
interval rows): 4MB spills (an external merge of about 15MB); 32MB, 64MB and 128MB all hash in
memory, with hash aggregate peaks of about 35MB, 37MB and 42MB. 32MB is below its own peak and
fits only because a hash may use twice work_mem, so 64MB is kept as the headroom. The rig now
measures the weighed bounds (32MB and the 128MB this replaces) on the same plant.
…days without a build, and keep partial builds invisible to pre-V184 code (#5812 fix A)

H1: queue 1 (never built or late-moved days, newest first, capped at 120) runs before
queue 2 (partial days with rows to add, oldest build first, one per server). Idle partial
days advance and tomorrow is stamped by one statement each, outside the cap, on the store
clock. M1: V184 (changed in place) adds partial_seq; a partial build stamps partial_seq and
leaves built_seq NULL. M3: the trigger's bound is a named constant the SQL interpolates,
pinned one hour under the watermark lag, with a live seam test. L4: doc comments.
… the candidate cache, and measure against dev (#5812 fix B)

M2: the exact-bound table and raw reads run in a short read-only transaction with the same SET LOCAL work_mem as the daily read. L1+L3: Forget is gone; Store evicts entries older than RefreshAfter and entries of servers outside the published set; every reuse decision is logged at Debug with its reason; the exact-bound shapes carry rawWindowStart. L2: the docs say a persisted on-demand pass may use a read up to RefreshAfter old. L7: the watermark live test asserts rows read from the plan's JSON nodes, not Index Cond text. Rigs: MeasureFallback and CompareWithDev.
…fix B)

CompareWithDev times each read with a Stopwatch for its printout only; nothing is asserted on it.
… writer tests' steady rows above the trigger's open-day bound at every hour (#5812 fix A2)
…tial day, so it skips only 02:00-04:00 (#5812 fix A2)
…own clock reads, and skip when either crosses midnight (#5812 fix A2)
… pin the second builder's one stamped day (#5812 fix A2)
…and round 2's Lows

Dev now carries #5819's V184 (the investigation tables), so this PR's rung becomes V185:
the migration and its registration, StorageVersion.SchemaVersion, the probe sentinel at the
next ordinal with its MapProbedSchemaVersion parameter and arm, DropV185SentinelAsync for the
rewinds, and every rung test's ordinals, version lists and migrate counts.

Round 2's Lows: the daily read's SQL comment and PlanRegressionLiveFloor's doc state the
partial-day rule (L1); queue 2's cap is documented as the number of builder servers (L2);
PlanRegressionDailyWorkMemSql is PlanRegressionWorkMemSql (L4); queue 1 orders by its oldest
build first, so servers whose today goes stale on every apply cannot hold a late-marked
closed day out of the cap, with a live test that fails on the old order (L5).
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 15, 2026 05:08
Comment on lines +175 to +202
public void EveryPlanRegressionRead_RunsUnderTheOneWorkMemSetting()
{
Assert.Equal("SET LOCAL work_mem = '64MB'", PgFactCollector.PlanRegressionWorkMemSql);

var fact = FactSource();
Assert.Equal(1, Count(fact, "new NpgsqlCommand(PlanRegressionWorkMemSql"));
Assert.DoesNotContain("work_mem =", fact, StringComparison.Ordinal);
Assert.Equal(2, Count(fact, "await BoundTheReadAsync();"));

var fallback = fact.IndexOf("if (snapshot is null)", StringComparison.Ordinal);
var fallbackBegin = fact.IndexOf("snapshot = await connection.BeginTransactionAsync(context.CancellationToken);", StringComparison.Ordinal);
var read = fact.IndexOf("readsDays ? PlanRegressionDailySql : readsTable ? PlanRegressionTableSql : PlanRegressionSql, connection, snapshot)", StringComparison.Ordinal);
Assert.True(fallback > 0 && fallback < fallbackBegin && fallbackBegin < read,
"the exact-bound read must begin its own transaction, and bound it, before the read command");
Assert.True(fact.IndexOf("await BoundTheReadAsync();", fallbackBegin, StringComparison.Ordinal) < read);
}

[Fact]
public void TheCacheHasNoForget_AndTheServicePublishesItsServerSet()
{
var root = Root();
var cache = File.ReadAllText(Path.Combine(root, "Darling", "PerformanceMonitor.Darling.Analysis", "PgFactCollector.PlanRegressionCache.cs"));
Assert.DoesNotContain("Forget", cache, StringComparison.Ordinal);

var worker = File.ReadAllText(Path.Combine(root, "Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"));
Assert.Contains("PlanRegressionCandidateCache.Shared.RetainServers(initialServers.Select(s => s.ServerId));", worker, StringComparison.Ordinal);
Assert.Contains("PlanRegressionCandidateCache.Shared.RetainServers(view.EnabledServers.Select(s => s.ServerId));", worker, StringComparison.Ordinal);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trap .github/REVIEW-TRAPS.md (Tests and censuses) applies here. The ruling reads, in its own words:

Ruling (#5833): tests are load-bearing. The censuses, pins and rosters named in this section are being deleted under #5833. Never add one, and never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage percentage). A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed.

Four new assertions in this file are nothing but spelling:

  • EveryPlanRegressionRead_RunsUnderTheOneWorkMemSetting (L180-189) counts occurrences of the literals "new NpgsqlCommand(PlanRegressionWorkMemSql" and "await BoundTheReadAsync();" in PgFactCollector.QueryPerf.cs, and asserts the relative IndexOf order of three more source literals.
  • TheCacheHasNoForget_AndTheServicePublishesItsServerSet (L197-201) asserts the substring "Forget" is absent from one file and two whole C# statements are present verbatim in DarlingWorker.cs.
  • AnExactBoundRead_WithADifferentWindowStart_IsADifferentShape (L167-169) asserts the collector source spells exactWindowStart: readsDays ? null : rawWindowStart, watermarkParameter: readsDays ? 5 : null, plus "out decision);" and a LogDebug format string.

Each of these has a behaviour twin already in the diff that fails on the defect: the work-memory bound is measured by PlanRegressionDailyNoSpillRig.MeasureFallback, the rawWindowStart shape by the pure-rule assertions right above at L138-163, and RetainServers by AStore_EvictsTheEntriesOfServersNoLongerMonitored_OnlyOnceASetIsPublished. What is left is brittleness: renaming a local function, moving BoundTheReadAsync above the readsTable branch, or hoisting the second SET LOCAL into a helper all go red while the behaviour is unchanged — and none of them goes red on a store that actually spills.

The PR body applies this same reasoning correctly one section down ("The source-text pin of the old ORDER BY in TheConstantsAndStatements_AreTheDocumentedOnes is deleted, not moved: its rule was spelling, and the live test now pins the order by behavior"). These three members are the same shape and want the same treatment. PlanRegressionDailyRungTests.TheStampAndTheBuiltDaysRead_KeepAPartialBuildInvisibleToPreV185Code is in the same position: every one of its six assertions is a SQL fragment's wording, and PlanRegressionDailyWatermarkLiveTests.APartialDay_IsUnbuiltToTheV183Predicate_AndAV183WholeBuildAfterItReadsWhole_WithNoDoubleCount already proves the behaviour against a real store.

(The probe-ordinal and migration-literal pins in the *RungTests classes are a different matter — CONTRIBUTING's "Adding a Darling Migration Rung" mandates them as the house form, and this PR moves them exactly as it should.)

Comment on lines +1264 to +1267
FOR EACH ROW
WHEN (NEW.first_execution_time < date_trunc('hour', now() AT TIME ZONE 'UTC') - interval '{PlanRegressionDaily.LateTriggerLagHours} hour'
AND NEW.first_execution_time >= date_trunc('day', now() AT TIME ZONE 'UTC') - interval '17 days')
EXECUTE FUNCTION collect.plan_regression_daily_mark_late();";

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 margin between the trigger's bound and a steady 60-minute interval is exactly zero, and the two sides of the comparison are two different machines' clocks.

NEW.first_execution_time is the monitored SQL Server's own reading, taken from sys.query_store_runtime_stats.first_execution_time as a datetimeoffset and normalised to UTC client-side (QueryStoreCollector.cs:964, :1644). now() here is the store's clock. The bound is date_trunc('hour', now()) - 1 hour.

With Query Store's default INTERVAL_LENGTH_MINUTES = 60, intervals are hour-aligned, so for the whole hour after an interval closes the collector keeps re-writing its rows (their last_execution_time advanced on the pass that observed the close) with first_execution_time >= <that hour's start> — which is the bound. The PR body states this as the safety argument:

Steady collection does not fire the trigger. The collector fetches only intervals whose last_execution_time advanced (QueryStoreCollector.WatermarkColumn), and those intervals start at or after the trigger's bound for intervals of 60 minutes or less.

True on one clock, with zero slack. The margin is (monitored clock − store clock) + (first execution − interval start), and for a query that runs continuously the second term is milliseconds. A store whose clock is a second ahead of the monitored server's — ordinary NTP drift, and the very condition PlanRegressionSkewMarginDays exists for ("a monitored server whose clock runs ahead of the collector's", PlanRegressionDaily.cs:40) — puts every row of the just-closed interval below the bound.

Failure scenario: store clock 2 s ahead of a monitored server with the default 60-minute Query Store interval. Every apply during the hour after an interval closes fires plan_regression_daily_mark_late() on the first such row, bumping late_seq for today and tomorrow. Both then fail built_seq = late_seq and partial_seq = late_seq, so BuiltDaysSql stops returning them, PlanRegressionLiveFloorWithWatermarks falls back to "one day below the first unbuilt day" (= today's midnight), and PLAN_REGRESSION reads the open days whole again — exactly the pre-#5812 read the PR exists to remove — while queue 1 rebuilds today on every tick. Nothing fails; the gain just silently does not materialise, for the default configuration.

The day-long-interval case is acknowledged in the body ("A server with day-long intervals fires on every apply, so its open days read live, as today, and pay a trigger call per row"), but that paragraph treats 60-minute intervals as the safe case, and it is safe only to within clock skew.

The two tests that pin this both use SteadyFirst(now) = date_trunc('hour', now) (QueryStoreIntervalLatestWriterTests.cs:610), a full hour above the bound, so neither exercises a row sitting at the bound, let alone a second below it. TheLateBatchTestsRows_HoldAgainstTheShippedTriggerBound_AtEveryHourOfAPinnedClock varies the hour, not the margin.

The knob is the pinned pair: taking LateTriggerLagHours to 2 and PartialWatermarkLagHours to 3 buys an hour of skew tolerance and costs an hour of live-half width (the read covers the last 3-4 hours of first executions instead of 2-3) — still far from the 3 open days today. If the zero margin is instead a deliberate bet that the two clocks agree to within the time between an interval's start and its first execution, that bet belongs in this doc comment and in PartialWatermarkLagHours, since it is the condition under which the whole rung pays off.

Comment on lines +261 to +267
/// <summary>
/// Darling only (#5812): true when this pass may reuse the PLAN_REGRESSION candidate rows an earlier current pass on
/// the same server read, while they are fresh (<c>PgFactCollector.PlanRegressionCandidateCache</c>). Set by the
/// current-time callers (the scheduled pass, <c>analyze_server</c> and <c>get_analysis_facts</c> with no <c>as_of</c>);
/// false everywhere else, so a past <c>as_of</c> and every other caller read live, as before.
/// </summary>
public bool ReusePlanRegressionCandidates { get; set; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This doc names the scheduled pass as one of the callers that set ReusePlanRegressionCandidates, and the code sets it to the exact complement:

ReusePlanRegressionCandidates = asOfUtc is null && !persistCollectionCaveats,
RefreshPlanRegressionCandidates = asOfUtc is null && persistCollectionCaveats,

(DarlingAnalysisService.cs:392-393, where persistCollectionCaveats is "True only for the SCHEDULED pass" per that method's own doc at :362.)

The next property's summary (L270) says the opposite — "true for the scheduled pass with no as_of. It never reuses PLAN_REGRESSION candidates" — so the pair reads as a leftover from before ruling R3 changed the window from six hours to one cadence.

This matters more than a stale sentence usually would, because this property is the seam a future scheduled caller would read, and the body's own handoff note points the other way: "R3 uses persistCollectionCaveats as the scheduled-pass marker (#5633 made it the one caller that sets it). A new scheduled caller must set it, or it would reuse." Someone trusting L264 would conclude the scheduled pass already reuses and that RefreshPlanRegressionCandidates is the odd one out.

Suggest dropping "the scheduled pass" from the list, and saying instead that the scheduled pass sets RefreshPlanRegressionCandidates and never this one.

/// and fits only through that multiplier, so 64MB keeps the headroom. A hash or sort takes memory only as it grows, so
/// a small server's read uses what it needs (<c>PlanRegressionDailyNoSpillRig</c> measures every bound on one plant).
/// </summary>
internal const string PlanRegressionWorkMemSql = "SET LOCAL work_mem = '64MB'";

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, and the body already hands this to the coordinator — recording it here so the number sits next to the constant rather than only in the PR description.

SET LOCAL work_mem = '64MB' (bound 2, and since fix B every PLAN_REGRESSION read) is a per-node figure, not a per-read cap. … The daily read has six such nodes … Scheduled reads run inside each server's sweep, under the fleet sweep gate … 4 by default (DarlingConfig.MaxConcurrentSweeps), at most 16 (StoreConfigProvider.MaxConcurrentSweepsLimit). On-demand analyze_server and get_analysis_facts calls that miss the cache run outside that gate. Nothing caps the product of nodes, bound and degree.

Confirmed from the tree: StoreConfigProvider.cs:1473 has MaxConcurrentSweepsLimit = 16, and M2 extends the setting to the exact-bound reads, which are the ones a store runs before its days are built — i.e. the first tick after this rung lands, for every server at once, is also the worst case for this bound. The measured peak is reassuring (≈37 MB in each of two hash aggregates on the plant), but it is one plant on one shape, and the ceiling the server will actually honour is 6 nodes × 64 MB (×2 for a hash, via hash_mem_multiplier) × degree.

The comment above the constant explains the floor (why not 32MB) thoroughly and says "a small server's read uses what it needs", which is true per node but does not bound the product. Two sentences recording the per-read and per-fleet ceiling — and whether the operator is expected to size MaxConcurrentSweeps against it — would stop the next person raising this to 128MB "because the rig said it also did not spill".

@claude

claude Bot commented Oct 15, 2026

Copy link
Copy Markdown

Full review pass on V185 / #5812. Four inline notes posted; the formal verdict follows in a review.

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 — V185 adds built_through + partial_seq and recreates the late-row trigger at one hour before the current hour — PgMigrations.cs:1257-1267.
  • VERIFIED — SchemaVersion = 185, rung after dev's V184, probe sentinel at the last ordinal with return 185 above return 184 — StorageVersion.cs:19, PgMigrations.cs:1600, ViewerDataService.cs:1251, :1269, :1477-1484.
  • VERIFIED — a partial build is invisible to pre-V185 code: built_seq NULL on a partial build, built_seq = late_seq read as the whole day whatever built_through holds — PlanRegressionDaily.cs:175-180, :255-265.
  • VERIFIED — the exclusion is a scalar lookup in the scan filter, and the live floor is the lower of the lowest partial watermark in the contiguous run and a day below the first unbuilt day — QueryPerf.cs:545, :588-605, five cases in PlanRegressionDailyBoundaryTests.TheWatermarkFloor_*.
  • VERIFIED — RefreshAfter is one analysis cadence from the clamped interval (5-360, default 30) — DarlingWorker.cs:543-544, :4684.
  • VERIFIED — the scheduled pass never reuses and always refills; only no-as_of analyze_server / get_analysis_facts reuse — DarlingAnalysisService.cs:392-393 against :362, and :836.
  • VERIFIED — two queues with the documented orders and caps; the two lock-free stamps run before the plan and write only while late_seq holds — PlanRegressionDaily.cs:287-301, :318-336, :348-372, :631.
  • VERIFIED — M2: both exact-bound reads take the work memory in a short read-only transaction disposed right after the read — QueryPerf.cs:839-848, :946-950. Lite unchanged, its own 14-day read at Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:523, deferred to #5823.
  • UNVERIFIED — every rig figure (4MB spills; 32/64/128MB do not; peaks ~35/37/42MB) and CompareWithDev — needs a live store and the plant, opt-in behind DARLING_NOSPILL_RIG_PLANS (PlanRegressionDailyNoSpillRig.cs:46-49).
  • UNVERIFIED — every suite result and the 200-server plan times — no store here, and the body leaves CI unchecked.
  • UNVERIFIED — "Steady collection does not fire the trigger … intervals of 60 minutes or less" — true with zero slack, across two machines' clocks; needs two real clocks with drift. Inline note on PgMigrations.cs:1264-1267.
  • UNVERIFIED — the RED mutations, "each run once and reverted", including M3's PartialWatermarkLagHours = 0 plant.
  • REFUTED — the title, "once per server per six hours" — the tree reuses for one analysis cadence (30 min default) and the scheduled pass never reuses. R3 supersedes the title; the title did not follow.
  • REFUTED — AnalysisContext.cs:264 names "the scheduled pass" among the callers setting ReusePlanRegressionCandidates; DarlingAnalysisService.cs:392 sets the exact complement and :270 says the opposite. Inline note posted.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs:1265 — the only guarantee that a row below a partial day's watermark invalidates that day, comparing the monitored server's first_execution_time against the store's now() with zero margin at 60-minute intervals — pinned by QueryStoreIntervalLatestWriterTests.TheLateBatchTestsRows_HoldAgainstTheShippedTriggerBound_AtEveryHourOfAPinnedClock for the hour dimension, UNPINNED for the margin (SteadyFirst sits an hour above the bound, :610).
  2. Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs:545 — the watermark lookup alone decides whether a row is counted once, twice or not at all — pinned by PlanRegressionDailyWatermarkLiveTests.FourteenDays_ThreePartial_... (RED with the old exclusion: 0 of 692) and PlanRegressionDailyParityLiveTests.
  3. Darling/PerformanceMonitor.Darling.Storage/PlanRegressionDaily.cs:175-180 — a wrong whole/partial arm would leave built_seq = late_seq on a partial build, so every reader takes a partly-built day as whole and loses its newest rows permanently — pinned by APartialDay_IsUnbuiltToTheV183Predicate_AndAV183WholeBuildAfterItReadsWhole_WithNoDoubleCount.

Traps near this diff

  • "Ruling (#5833): tests are load-bearing. … never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage percentage)." — applies — three new members of PlanRegressionCandidateCacheEvictionTests (:167-169, :175-190, :192-202) and PlanRegressionDailyRungTests.TheStampAndTheBuiltDaysRead_KeepAPartialBuildInvisibleToPreV185Code assert only spelling, each with a behaviour twin already in this diff. Inline note posted; the *RungTests probe-ordinal pins are excluded, CONTRIBUTING mandates them.
  • "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 — a PostgreSQL-store mechanism with no DuckDB analogue; Lite reads one local store in one process (Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:523), has no plan_regression_daily twin, and the two new AnalysisContext flags default false.
  • "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" — does not apply — every day is covered by exactly one tier out of one REPEATABLE READ READ ONLY snapshot (QueryPerf.cs:819-824), the live tier shrinks, and the source is still stamped (:1080, :1042).
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — nothing pins two copies equal; the shape's readsDays ? 5 : readsTable ? 4 : 3 / watermarkParameter: readsDays ? 5 : null (QueryPerf.cs:898-899) are magic ordinals over the bind block above, not a copy of it.
  • ".gitattributes says text=auto eol=crlf … A diff that touches every line of a file is a line-ending conversion until proven otherwise" — does not apply — the body records this biting during the merge, and the fix held: PgMigrations.cs is two localised hunks, and no file of the 42 is a whole-file rewrite.

Divergence (blind read vs PR body)

  1. The title has not followed R3. It sells "once per server per six hours"; the shipped mechanism is one analysis cadence with the scheduled pass excluded. The body is internally honest, but the title is what the changelog carries.

  2. The trigger's safety argument is thinner than stated, and the test does not test the thin part. The body treats day-long intervals as the one degraded case; on my read the default 60-minute case degrades the same way under ordinary clock drift between the monitored server and the store — silently, back to the pre-#5812 read this PR exists to remove, with queue 1 rebuilding today every tick. SteadyFirst sits an hour above the bound, so nothing exercises a row at it. Inline note with the knob and its cost.

  3. The #5833 ruling is applied in one section and violated in three others. The merge prep deletes the ORDER BY pin because "its rule was spelling", while fix lane B adds source-literal counts, IndexOf orderings and a DoesNotContain("Forget"), and fix lane A adds a SQL-wording pin the live test already covers. The body lists these as achievements.

Everything else held. The partial-day arithmetic is where I expected a hole and found none: double-counting and dropped rows check out across the whole-day, partial-day, gap, pre-V185-builder, around-midnight and tomorrow-stamped cases, and the three invariants that close it — a row's side of the watermark fixed for its life, the floor never above any partial watermark in the run, and both stamps writing only while late_seq holds — are as the comments claim.

Reviewer checklist

  • Measure the trigger's real firing rate on a 60-minute-interval server with the store on another host: if built_seq/partial_seq disagree with late_seq for today and tomorrow after an hour of steady collection, the open days read live and the rung is inert there.
  • Watch resident memory on the store through the first tick after the rung lands, when every server runs the exact-bound read at 64MB at once under MaxConcurrentSweeps — per-node bound, node count and concurrency all peak together, which the rig's single plant does not cover.
  • Open the viewer against a V184 and a V185 store: the pg_attribute sentinel is a column probe rather than this probe's usual to_regclass table probe, so the attisdropped arm and the NULL-attrelid path each want one live read.
  • Run a tick with the service clock minutes off the store's: PlanSql plans from $1 while SeqSql stamps from now(), so a service running ahead can plan a build that restamps the watermark it already had.
  • Confirm the write amplification at fleet scale: today is rebuilt from scratch once per tick per server, so late in the day each rebuild re-reads most of a day of interval rows.

…e trigger, spelling pins dropped (#5812 review 1)

- LateTriggerLagHours 1 -> 2 and PartialWatermarkLagHours 2 -> 3 in the unreleased V185: the just-ended
  60-minute interval no longer sits exactly on the trigger's bound, so a store clock ahead of the monitored
  server no longer fires it on every steady apply. The bet is stated on both constants and on V185.
- Live tests: rows at the old bound and a second below it stay quiet, also with the store's clock past
  the hour; a row below the new bound fires. SteadyFirst is the just-ended interval's start.
- Four source-spelling test members (and two '2 hours' SQL-text lines) deleted; behavior twins cover them.
- AnalysisContext: the scheduled pass sets RefreshPlanRegressionCandidates, never ReusePlanRegressionCandidates.
- PlanRegressionWorkMemSql: the per-read and per-fleet work_mem ceiling.
@erikdarlingdata erikdarlingdata changed the title Run the plan regression read once per server per six hours instead of every analysis pass (#5812) Plan regression reads stop re-reading three open days of Query Store intervals, and on-demand calls reuse the candidates for one analysis cadence (#5812) Oct 15, 2026
…pg_stat_statements (#5812 CI red)

The get_analysis_facts reuse test read the pass's pg_stat_statements entry
back. That view is cluster-wide, and this own-store class runs in parallel
with classes that reset it for the whole cluster or force evictions, so on
CI the entry vanished while the cache counted a hit and no miss. The test
now deletes the costly plan's rows after the pass, as the class's first
test does: the call reports the regression only by reusing the pass's
candidates, and the read past RefreshAfter finds none.
@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 15, 2026 10:05

Claude posting for Erik Darling. Round 1's four findings were answered at 78ec763 (lags 2/3 h, four spelling pins deleted, both doc summaries true, the 64MB ceiling note, new title); the CI-red reuse test was fixed at d334868 and 80fca7c (test-only). Rounds 2 and 3 hit the 30-turn cap with no verdict (error_max_turns, 31 turns), so the 2-round cap applies; no round-3 findings exist to file. Own Build run 38712490231 green on 80fca7c.

@erikdarlingdata
erikdarlingdata merged commit fb40541 into dev Oct 15, 2026
25 of 27 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5812-plan-regression-incremental branch October 15, 2026 10:05
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