Repository navigation
Bound PLAN_REGRESSION on the partitioning column too - #2390
Conversation
The comparison window is on last_execution_time, which is right -- a plan regression is about when the query last RAN, not when we collected it. But query_store_stats is partitioned and compressed on collection_time, so a predicate on last_execution_time alone prunes nothing: every analysis cycle read the server's whole history and decompressed whatever was compressed, per server, every cycle. Cost scaled with STORE SIZE rather than with the configured window, which is why the reporter's box sat at 100% CPU with 34 postgres backends and no VM size fixed it. Reported by @ethosthalsell, who identified the mechanism, the reason the covering indexes do not rescue it, and the fact that the code comment claimed the tradeoff was deliberate. All three held up. Same class as #2344's unbounded MAX(last_execution_time) in the watermark read, one query over. Fixing that one and leaving this is on us. The added predicate cannot change the result. A row cannot be collected before the execution it reports, so last_execution_time >= X already implies collection_time >= X. It is redundant to the ANSWER and load-bearing for the PLANNER -- #2344's "provably free" argument. $3 sits one skew margin BELOW the window rather than equal to it. last_execution_time comes from the monitored server's clock and collection_time from the store's, so a monitored server running fast can report an execution later than the collection carrying it. Without the margin those newest rows would be silently excluded, which is a worse bug than the one being fixed. Bound as its own parameter rather than "$2 - INTERVAL '1 day'" because a bare parameter comparison is the form runtime chunk exclusion handles most reliably. Measured on the live use1 store, 44 GB, 6 chunks, 4 compressed. The honest version: a 15-day bound excludes NOTHING there, because that table spans only 08-15..08-21 -- retention keeps the raw tier short, so there is no history beyond the window to prune. My first reading showed a 199x drop in buffer reads and was wrong: it was cache warming from running the unbounded query first. Reversing the order reproduced the same read counts both ways. What IS demonstrated is the mechanism. With a collection_time bound tight enough to bite: Chunks excluded during startup: 4 (of 6) against 0 with no such bound. So the predicate does prune; the magnitude depends entirely on how much history the store holds beyond the window. On a store whose rollups are armed this costs nothing and saves nothing. On the reporter's -- months of history, rollups not armed -- it is the whole fix. Fixes #2387
|
Reviewed the Darling-side fix for #2387. Overall the fix is sound: adding a redundant Two things worth a look before merge, left as inline comments:
No correctness, security, or style issues found otherwise — comment style, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /* #2387: the chunk-exclusion bound, one CLOCK-SKEW MARGIN below the comparison window. Bound as | ||
| its own parameter rather than written as "$2 - INTERVAL '1 day'" so the planner compares | ||
| against a bare parameter, which is the form runtime chunk exclusion handles most reliably. */ | ||
| cmd.Parameters.AddWithValue(AsNaive( | ||
| context.TimeRangeStart.AddDays(-(PlanRegressionWindowDays + PlanRegressionSkewMarginDays)))); |
There was a problem hiding this comment.
This comment's own justification for using a bare $3 parameter — "a bare parameter comparison is the form runtime chunk exclusion handles most reliably" — directly contradicts the sibling query in PgDrillDownCollector.Queries.cs (line ~365), which does the identical chunk-exclusion job for the same v_query_store_stats hypertable via inline arithmetic: AND collection_time >= $2 - interval '1 day'.
Both queries exist to solve exactly the same problem (#2387-class: bound the partitioning column so TimescaleDB can exclude chunks), on the same table, with the same 1-day skew margin — but now use two different techniques, and this PR's rationale implies the drill-down query's technique doesn't reliably trigger chunk exclusion. If that's true, the drill-down query likely has the same unbounded-scan cost this PR fixes here and should get the same treatment. If it's not true (the inline form works fine, as its own comment claims), then this PR's stated reason for introducing a third parameter is unfounded and worth double-checking with EXPLAIN (ANALYZE, BUFFERS) against a compressed chunk before it becomes the template other collectors copy.
Either way, this is worth reconciling rather than leaving two contradicting approaches to the same known bug class in the codebase.
1 participant
Fixes #2387, reported by @ethosthalsell — who identified the mechanism, the reason the covering indexes do not rescue it, and the fact that the code comment claimed the tradeoff was deliberate. All three held up.
The bug
The comparison window is on
last_execution_time, which is correct — a plan regression is about when the query last ran, not when we collected it. Butquery_store_statsis partitioned and compressed oncollection_time, so a predicate onlast_execution_timealone prunes nothing. Every analysis cycle read the server's whole history and decompressed whatever was compressed, per server, per cycle, with cost scaling on store size rather than on the configured window. That is why the reporter's box sat at 100% CPU with 34 postgres backends and why no VM size fixed it.Same class as #2344's unbounded
MAX(last_execution_time)in the watermark read, one query over. Fixing that one and leaving this is on us.Why the added predicate is safe
It cannot change the result. A row cannot be collected before the execution it reports, so
last_execution_time >= Xalready impliescollection_time >= X. Redundant to the answer, load-bearing for the planner — #2344's "provably free" argument exactly.$3sits one skew margin below the window rather than equal to it.last_execution_timeis the monitored server's clock andcollection_timeis the store's, so a server running fast can report an execution later than the collection carrying it; without the margin those newest rows would be silently excluded, which is a worse bug than the one being fixed. Bound as its own parameter rather than"$2 - INTERVAL '1 day'"because a bare parameter comparison is the form runtime chunk exclusion handles most reliably.Measurement, including the part I got wrong
Measured on the live use1 store — 44 GB, 6 chunks, 4 compressed.
My first reading was wrong and I nearly reported it. Running the unbounded query first and the bounded one second showed reads dropping 41,379 → 208, which looked like a 199x win. Reversing the order killed it:
Same reads either way. The "improvement" was cache warming from the first query, and
Chunks excluded during startup: 0in every run was the tell I should have taken seriously immediately.The reason is that use1's
query_store_statsspans only 08-15 to 08-21. Retention keeps the raw tier short, so a 15-day bound has no history beyond the window to prune — this store structurally cannot demonstrate the fix.What IS demonstrated is the mechanism. With a
collection_timebound tight enough to bite:against
0with no such bound. The predicate prunes; the magnitude depends entirely on how much history the store holds beyond the window. On a store whose rollups are armed this costs nothing and saves nothing. On the reporter's — months of history, rollups not armed — it is the whole fix.So this ships on a proven mechanism plus the reporter's field evidence, not on a benchmark I can reproduce here. Worth someone re-running
EXPLAIN (ANALYZE, BUFFERS)on a store that actually holds months, which is the only place the numbers will be dramatic.Tests
Two, both source-level since the SQL is a constant: that the semantic window stays on
last_execution_timewhilecollection_timeis bounded as its own$3, and that the exclusion bound reaches further back than the comparison window rather than less far.