Skip to content

Read the Query Store chunk floors once per fleet eligibility check (#4605) - #5001

Merged
erikdarlingdata merged 5 commits into
devfrom
fix/4605-fleet-eligibility
Oct 3, 2026
Merged

erikdarlingdata merged 5 commits into
devfrom
fix/4605-fleet-eligibility

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

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.StoreWideInputs holds the three floors: the raw floor, whether the table is a hypertable, and the table floor. StoreWideInputsCache is a small per-check holder. ResolveReadAsync takes an optional storeWide cache. It reads ChunkFloorsSql at 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: ReadSourceInputsSql already reports it per server.
  • ResolveQueryStoreWideEligibleAsync passes one cache to each server's ResolveReadAsync. 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.
  • The server list query now ends in ORDER BY server_id. When two servers tie for the latest read start, the one that names the bound is now the lower server_id. Before, it depended on the table's physical row order.

Store-wide versus per-server inputs

  • Store-wide: ChunkFloorsSql, which takes no parameter.
  • Per-server: ReadSourceInputsSql (coverage and pending columns), PlainTableFloorSql, HasLegacyRowSql and the cadence probe. Each filters on server_id, so they stay per server.

Tests

QueryStoreWideFleetEligibilityLiveTests runs live, each test on its own scratch store, and counts commands with NpgsqlCommandCounter (scoped to the test's database, with a control command that must count once):

  • Six seeded servers with different read starts, two of them tied for the latest: the fleet answer equals a serial per-server answer computed the old way, identical across 20 runs, and the floors statement runs once per check. RED with the cache never filled: Expected: 1, Actual: 6.
  • First server refuses before the floors step (literal end before applied-through): zero floors reads and the raw answer.
  • A fleet with one server holding legacy rows reads raw, the same as the serial answer.

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

@erikdarlingdata erikdarlingdata left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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 oldest query_store_stats chunk, whether query_store_interval_wide is a hypertable, and that hypertable's oldest chunk. It has no $1 and no server filter. Everything keyed by server still runs per server inside ResolveReadAsync: filled_since, applied_through and has_pending (ReadSourceInputsSql, :427-441, read at :1039-1048), PlainTableFloorSql (:476, which still overwrites tableFloor on the non-hypertable path, :1092-1097), HasLegacyRowSql (:1108-1118) and the cadence probe (:1125). Clause 3 of UseTable (the table-floor clause) gets a store-wide tableFloor only 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. ReadStoreWideInputsAsync returns null when to_regclass is NULL (:991-999). ResolveReadAsync then takes else if (hasTimescale) (:1073) using its own per-server has_timescale, which is false, so the floors stay null as before. A non-null storeWide is 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. OperationCanceledException still propagates.
  • ORDER BY server_id only makes ties deterministic: VERIFIED (:1477). The eligibility answer is "all servers pass", and WideStart is a maximum. Neither depends on order. Only Bound and SettingServer on an exact ReadStart tie change, and those now go to the lowest server_id.
  • The pin cannot see other test classes: VERIFIED. The class line is column 0 at test :29, with the #1776 own-store marker on :28. ScratchPostgres names 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 inside ReadStoreWideInputsAsync, 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-recommended level (QueryStoreBackfill.cs:614, DarlingCollectorRunner.cs:6356).
  • CA1859 and xUnit2013: VERIFIED, not hit. floorReads is an int, not a collection.
  • CRLF: VERIFIED, not applicable. The PR adds no source-text pin.

Counts: Defect 0, Note 2, Nit 3.

Comment thread Darling/PerformanceMonitor.Darling.Service/DarlingWebEndpoints.cs Outdated
Comment thread Darling/PerformanceMonitor.Darling.Storage/QueryStoreIntervalWide.cs Outdated
Comment thread Darling/Darling.Tests/QueryStoreWideFleetEligibilityLiveTests.cs Outdated
Comment thread Darling/Darling.Tests/QueryStoreWideFleetEligibilityLiveTests.cs Outdated
Comment thread Darling/Darling.Tests/QueryStoreWideFleetEligibilityLiveTests.cs Outdated

@erikdarlingdata erikdarlingdata left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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. In ResolveReadAsync, the first server to reach the floors step reads ChunkFloorsSql and 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 in ResolveReadAsync is unchanged. The cache is filled only on a TimescaleDB store; on any other store, hasTimescale keeps 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.

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 3, 2026 06:05
@erikdarlingdata
erikdarlingdata merged commit fcba539 into dev Oct 3, 2026
19 of 20 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4605-fleet-eligibility branch October 3, 2026 06:06
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