Repository navigation
Read the Query Store chunk floors once per fleet eligibility check (#4605) - #5001
Conversation
…ts scratch database, close the connection before the fan-out (#4605)
erikdarlingdata
left a comment
There was a problem hiding this comment.
Claude posting for Erik Darling
The change is correct. I found no defect. The three values the fleet check now reads once are store-wide: ChunkFloorsSql takes no parameter and reads only TimescaleDB's catalog. Every input keyed by server is still read per server. The fallback when TimescaleDB is absent is today's path. My findings are two Notes about cost and test coverage, and three Nits about code shape and test hygiene.
Claims checked:
- The store-wide values are store-wide: VERIFIED.
ChunkFloorsSql(QueryStoreIntervalWide.cs:449-471) returns raw's oldestquery_store_statschunk, whetherquery_store_interval_wideis a hypertable, and that hypertable's oldest chunk. It has no$1and no server filter. Everything keyed by server still runs per server insideResolveReadAsync:filled_since,applied_throughandhas_pending(ReadSourceInputsSql, :427-441, read at :1039-1048),PlainTableFloorSql(:476, which still overwritestableFlooron the non-hypertable path, :1092-1097),HasLegacyRowSql(:1108-1118) and the cadence probe (:1125). Clause 3 ofUseTable(the table-floor clause) gets a store-widetableFlooronly on the hypertable arm, and the old code did the same. So no server is routed to the wrong source. - The fallback without TimescaleDB is exactly today's path: VERIFIED.
ReadStoreWideInputsAsyncreturns null whento_regclassis NULL (:991-999).ResolveReadAsyncthen takeselse if (hasTimescale)(:1073) using its own per-serverhas_timescale, which is false, so the floors stay null as before. A non-nullstoreWideis used only when the server's own probe also says TimescaleDB is present (:1067). - The early exit, cancellation and catch are unchanged: VERIFIED (DarlingWebEndpoints.cs:1544-1563). A fault in the new up-front read lands in the same outer catch and gives the same answer, raw.
OperationCanceledExceptionstill propagates. ORDER BY server_idonly makes ties deterministic: VERIFIED (:1477). The eligibility answer is "all servers pass", andWideStartis a maximum. Neither depends on order. OnlyBoundandSettingServeron an exactReadStarttie change, and those now go to the lowestserver_id.- The pin cannot see other test classes: VERIFIED. The class line is column 0 at test :29, with the
#1776 own-storemarker on :28.ScratchPostgresnames each database with a GUID, and the listener filters on that name (:139, :146). xUnit runs a class's facts one at a time. The reader is disposed insideReadStoreWideInputsAsync, so the count is final before the call returns. If the tag were renamed, the count would be 0 and the test would go red, so it cannot pass silently. - The RED at Expected 1 / Actual 7: UNVERIFIED. Confirming it needs a database run.
- The new commands have an explicit CommandTimeout: VERIFIED (:991, :1001).
- The named-SQL-constant shape: REFUTED in one place, :991 (Nit below).
- ComposeParameterCoverageTests: VERIFIED, no drift. The compiler is untouched.
- CA1068: VERIFIED, not hit. An optional parameter after the CancellationToken has precedent in projects built at the same
latest-recommendedlevel (QueryStoreBackfill.cs:614, DarlingCollectorRunner.cs:6356). - CA1859 and xUnit2013: VERIFIED, not hit.
floorReadsis an int, not a collection. - CRLF: VERIFIED, not applicable. The PR adds no source-text pin.
Counts: Defect 0, Note 2, Nit 3.
erikdarlingdata
left a comment
There was a problem hiding this comment.
Claude posting for Erik Darling
Round 2 at 964bf5692: all five round-1 items are answered, there are no new findings, and nothing blocks the merge. I read the delta, one commit in three files; I didn't run it here.
- The cheap-refusal path: FIXED. The endpoint now passes an empty
StoreWideInputsCache. InResolveReadAsync, the first server to reach the floors step readsChunkFloorsSqland fills the cache, and every later server reuses it. A check that refuses before that step reads no floors at all. The early-exit order inResolveReadAsyncis unchanged. The cache is filled only on a TimescaleDB store; on any other store,hasTimescalekeeps today's path. The loop is serial, so the unsynchronised field is safe. - The statement counter: FIXED. The fact now uses
NpgsqlCommandCounter, which matches by value, scoped to the scratch database, and first asserts a control count of exactly one. - The inline probe literal: FIXED. The probe went away with the eager read, so the only store command left is the named
ChunkFloorsSql. - The leaks: FIXED. The fact takes the seeded data source, scratch database and connection with
await using. EndAsync's unused parameter and stray$: FIXED in the same commit.
Counts: no Defects, Notes or Nits.
A fleet Custom Views Query Store panel of 12 h or more first checks, for each server in scope, whether it may read the per-interval table. Each server's check re-read
ChunkFloorsSql. That read takes no parameter and reads TimescaleDB's catalog for the whole store, so the answer was the same every time. This change reads it at most once per check, and only when a server reaches that step.The saving is small. The floors read is a cheap catalog read. The per-server cost is unchanged, and most of it is the legacy-row probe (
HasLegacyRowSql). #4969 measured that probe on a production store at a mean of 4.1 s and 6.8 M blocks per call. Its own fix is #4972. The servers are still checked one after another on one connection, as before. Running them at once would stack those probes on a store its collectors write to every minute.Change
QueryStoreIntervalWide.StoreWideInputsholds the three floors: the raw floor, whether the table is a hypertable, and the table floor.StoreWideInputsCacheis a small per-check holder.ResolveReadAsynctakes an optionalstoreWidecache. It readsChunkFloorsSqlat the same point as before, only when the cache is empty, then fills it. Null keeps today's behavior, so the MCP and grid callers are unchanged. There is no separate TimescaleDB probe:ReadSourceInputsSqlalready reports it per server.ResolveQueryStoreWideEligibleAsyncpasses one cache to each server'sResolveReadAsync. A check whose first server refuses before the floors step reads no floors. The loop, the early exit on the first refusal (the panel then reads raw), cancellation and the read-raw catch are as on dev.ORDER BY server_id. When two servers tie for the latest read start, the one that names the bound is now the lowerserver_id. Before, it depended on the table's physical row order.Store-wide versus per-server inputs
ChunkFloorsSql, which takes no parameter.ReadSourceInputsSql(coverage and pending columns),PlainTableFloorSql,HasLegacyRowSqland the cadence probe. Each filters onserver_id, so they stay per server.Tests
QueryStoreWideFleetEligibilityLiveTestsruns live, each test on its own scratch store, and counts commands withNpgsqlCommandCounter(scoped to the test's database, with a control command that must count once):Expected: 1, Actual: 6.Also green: ComposeQueryStoreWideExactnessLiveTests, ComposeQueryStoreWideStartLiveTests, QueryStoreIntervalWideBelowFloorLiveTests, QueryStoreIntervalWideBelowFloorRoutesLiveTests, QueryStoreIntervalWideGridLiveTests, DarlingComposeTests, TsqlConventionGuardTests, DocCommentHygieneTests, RepoFileAdoptionTests, CommandDeadlineScannerAdoptionTests.
Darling.Tests and Lite.Tests build with 0 warnings and 0 errors (
--no-incremental).Refs #4605
SECTION: Changed
ENTRY: - Fleet Custom Views Query Store panels read the store's chunk floors once ([#5001]) - a fleet panel's table-eligibility check reads the store-wide floors at most once instead of once per server; the per-server checks are unchanged.
REF: [#5001]: #5001