Size pg_index_bloat's ceiling and cycle budget against the measured block rate, not an assumed one (#3164)

[erikdarlingdata]

Fixes #3164.

PgIndexBloatCollector.PessimisticBlocksPerSecond was 2_000 with no measurement behind it. The name asserted pessimism and the value was optimistic by roughly 2x: the first SUCCESS row this collector has ever produced puts the real rate at ~1,013 blocks/s. MeasuredBlocksPerSecond replaces it at 1,013, and MeasureCeilingBytes and CycleMeasureBudgetBytes both drop from 2 GiB to 1 GiB.

The rate, and the instrument that produced each number

The instrument CycleMeasureBudgetBytes' own summary pre-registered is a SUCCESS row's collection_log.sql_duration_ms (#2997). One exists: a 252,940 ms SUCCESS row, 2,500 rows collected, the first this collector has ever produced on any target.

That row alone bounds the rate from above with no further data. The gate admits an index only when the running total through that index is still within budget, so the admitted set is a prefix and the bytes a run reads cannot exceed the budget — and that run's budget was 2 GiB. So the rate is at most 262,144 blocks / 252.94 s = 1,036 blocks/s, i.e. at most 52% of the assumed 2,000. The ~1,013 figure derived from the bytes that run actually measured sits just under that bound, so the issue's number is confirmed rather than merely repeated.

The denominator is not pgstatindex time alone, and that matters for reading the constant. sql_duration_ms on a RunsPerDatabase collector is the sum across databases of connect + execute + drain, and that run swept two databases, one of which failed fast on a missing extension. Every one of those terms inflates the denominator, so the true per-block read rate is faster than this figure. That is the safe direction and the right quantity: what the constant feeds is a comparison against a command deadline, and the deadline covers connect and drain too — a pure I/O rate would understate the budget's real cost by exactly the terms it left out. The constant's doc says so, because "blocks per second" reads like a pure I/O rate and is not one.

Three things about provenance, because two of them read like corroboration and are not:

• MeasureCeilingBytes' "about 11 hours" for the over-ceiling set and "roughly an hour" for its largest member are this same rate restated at coarse precision, not independent derivations. 311 GiB at 1,013 blocks/s is 11.2 h; 27 GiB is 58 min. Citing them as agreement would be citing one measurement three times.
• n = 1. One run, one target, one night. The only independent check is the 1,036 blocks/s upper bound above, which agrees on the order and the direction.
• get_store_metrics produced none of the figures here. Its background-job series samples one reading an hour and its daily point is the day's last, so it cannot answer a maximum question (The hourly refresh envelope is stated at 306 s of slot margin; a measured run leaves 121.6 s and sits inside the warning band, and the daily series structurally cannot show it #3119). The duration above came from a collection_log read (wide window, truncated: false); the ceiling/census figures are the collector's own committed doc comments.

The run also spent 84% of its 300 s deadline, against a pin that allows half. That falsifies the pin's premise directly, before any rate arithmetic.

Why the ceiling had to move, and why the decoupling #3153 deferred could not have saved it

#3164 recommends the decoupling #3153 left undone — an unconditional first-admission rule plus the deadline constraint restated as a sum (ceiling + budget) — so the budget could fall to fit the real rate while the ceiling stayed at 2 GiB. That goal is unreachable at the measured rate, and the arithmetic is one line.

One index just under the ceiling is the largest amount of work a single statement can be asked to do. At 1,013 blocks/s, 2 GiB is 262,144 blocks and 259 s in one statement — past the entire 150 s allowance before a budget is chosen at all. ceiling + budget <= allowance therefore has no solution with a 2 GiB ceiling, and neither does max(ceiling, budget) <= allowance. The binding constraint was never the ordering between the two figures; it was the ceiling's own deadline cost, which equal values had been hiding.

So the ceiling comes down regardless of what the budget does. Once it does, setting the budget equal to it satisfies every constraint with no new mechanism, and decoupling would only permit the one direction (budget < ceiling) that reopens the band. 1 GiB is 131,072 blocks and ~129 s, inside the 150 s allowance with 21 s of margin — it absorbs a further 14% rate degradation (it breaches at 874 blocks/s), and it is off the edge of its own plausible range rather than at it.

How the band-emptiness floor survives

By construction, and unchanged: CycleMeasureBudgetBytes == MeasureCeilingBytes == 1 GiB, so budget >= ceiling still holds and TheCycleBudget_IsNeverBelowThePerIndexCeiling is green without modification. There is no band between the two figures, because there is no gap.

#3163's liveness guarantee is untouched for the same reason. Because budget >= ceiling, the first sub-ceiling candidate at or below the rotation cursor is still admitted unconditionally — its own size is under the ceiling, hence under the budget, so it is the first FILTERed row in the window frame and measured_bytes_through_here for it equals its own size, which cannot already have exceeded the budget. The cursor therefore still advances by at least one index every cycle, which is what makes "a later run in this pass reaches this index" a fact. Nothing about this change touches the gate's shape, the four reason arms, the cursor, or the prune.

The other arithmetic route, investigated rather than assumed

#3164 asks that raising CommandTimeoutSecondsOverride above 300 s not be taken without first establishing how it coexists with "the 120-second whole-server wall-clock budget that abandons cycles". Established: they do not interact, because that budget does not apply to this collector.

120 s is not a product-wide constant. It is the value three SQL Server collectors (procedure_stats, query_stats, plan_correction) chose for ICollectorSchemaInfo.PerItemWallClockBudget, the opt-in per-item budget #2673 added. CollectorDefinitionBase.PerItemWallClockBudget defaults to null; query_store already sets 600 s, longer than this deadline. PgIndexBloatCollector does not override it at all, so StartItemBudget returns null, the item token is the cancellation token, and no wall clock bounds this collector. A per-collector budget above 300 s is established precedent, not novel.

It is still not the move, for reasons about cost rather than mechanism, and they are now recorded on CommandTimeoutSecondsOverride: ~518 s would fit 2 GiB, which is over eight minutes of pgstatindex in one statement against a production instance — and because the deadline is a backstop rather than a bound, it is also eight minutes a mis-budgeted run spends before reporting nothing. It would be sized to make one n=1 measurement fit, which is the move the budget's own pin exists to prevent. And it would leave the rate constant wrong, which is the actual defect.

A second correction, stated rather than quietly rewritten: what the half-deadline reserve covers

TheCycleBudget_FitsTheDeadline_...' doc said the reserve paid for the catalog scan, connection setup, and "the tail index admitted while the running total was still just under budget — that index is charged for itself, so the last admission can be almost a whole index past the point where the budget was nearly spent."

That describes a gate of the form total BEFORE this row <= budget. The shipped gate is not that shape: measured_bytes_through_here is a window sum over ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW, so it includes the row being tested, and measured_bytes_through_here <= budget admits the tail index only if it fits in the headroom left. Prefix sums are non-decreasing, so the admitted set is a prefix and the total read is bounded by the budget exactly. There is no overshoot to reserve against.

Two reasons this is worth a paragraph rather than a silent edit. The wrong version made the reserve look as though it were covering a possible ~2x read, which would leave the actual rate error — the thing that just went wrong — uncovered. And the same prefix property is what lets a SUCCESS row's duration bound the block rate from above at all, so the upper bound above rests on it.

The reserve's job is now stated as being wrong about the rate, and only that.

Cadence is deliberately not touched

Coverage is cadence-limited after #3153. Lowering the budget makes passes longer, so moving both at once would conflate two effects and leave neither figure meaning anything. The cadence stays at the 1,440 minutes in CollectorScheduleDefaults and gets re-derived from whatever the budget is, separately.

What this costs

Coverage rate: at most 2x. Pass length is measurable blocks over the budget, so halving the budget at most doubles the pass — ~75 cycles becomes at most ~150, and at a daily cadence at most ~150 days rather than ~75. "At most", because the same change lowers the ceiling, which removes the indexes between the old and new ceilings from the measurable set entirely: a smaller numerator against the halved denominator, so the true figure lands below the doubling.

Permanently lost: the indexes between 1 and 2 GiB. Over-ceiling indexes are terminal, so these move from "measurable, eventually" to "recorded but never measured". How many there are on the real target is not measured — see below. The old census of 43 indexes / 311 GB is a figure at a 2 GiB ceiling, so it is now restated in the doc as belonging to the ceiling it was taken at rather than left attached to a ceiling it no longer describes. What is claimed is the direction and the mechanism, not a new count.

Their skipped_reason already states permanence rather than implying deferral, and already names pg_index_usage_stats as what carries their size trend instead — same target, same daily cadence, same retention, no extension needed, and it covers them whatever this ceiling is.

Evidence

Lite.Tests cannot execute on macOS, so the pins were run by compiling the actual Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs (unmodified, plus its real CollectorDefinitionTestFakes.cs) into a net10.0 console harness against a minimal xUnit shim, and reflecting over [Fact]. Baseline on the committed tree: 35 ran, 35 passed. CI is the arbiter; this is what was checkable locally.

Red first. Changing only PessimisticBlocksPerSecond from 2,000 to 1,013 and touching nothing else reds TheCycleBudget_FitsTheDeadline_AtThePessimisticBlockRate with its own failure text: "a full cycle budget of 2147483648 bytes is 262144 blocks, which at 1013 blocks/s takes 259s — past the 150s..."

Nine mutations, each confirmed applied by git diff --numstat before the run, each restored afterwards with the tree verified clean. Anchor counts asserted at 1 before every edit.

mutation	caught by
rate 1,013 → 800 (slower than measured)	both deadline pins, and the census pin
ceiling back to 2 GiB, budget stays 1 GiB — the decoupled state	ThePerIndexCeiling_FitsTheDeadline_OnItsOwn, TheCycleBudget_IsNeverBelowThePerIndexCeiling, TheBudgetLiterals_AgreeWithTheirConstants
budget back to 2 GiB, ceiling stays 1 GiB	TheCycleBudget_FitsTheDeadline_AtTheMeasuredBlockRate, TheBudgetLiterals_AgreeWithTheirConstants
CeilingLiteral left at the old 2 GiB SQL value	TheBudgetLiterals_AgreeWithTheirConstants
CycleByteBudgetLiteral left at the old 2 GiB SQL value	TheBudgetLiterals_AgreeWithTheirConstants
the ceiling pin reads the BUDGET instead of the CEILING	initially NOTHING. See below
the ceiling pin reads the budget and the ceiling goes to 2 GiB	EveryDeclaredByteBound_FitsTheDeadline_CensusedFromTheType, naming MeasureCeilingBytes
census filter changed to match nothing	its Assert.NotEmpty
census filter matches nothing and the non-empty assertion removed	its required-by-name check

The uncaught mutation, and the pin it produced

Mutating ThePerIndexCeiling_FitsTheDeadline_OnItsOwn to read CycleMeasureBudgetBytes instead of MeasureCeilingBytes is one token, and left all 34 pins green. A pin's subject is the one thing it cannot check about itself, and here the two figures are equal — which is exactly the value TheCycleBudget_IsNeverBelowThePerIndexCeiling documents as preferred. So the two pins are arithmetically indistinguishable for as long as this collector is correct, and diverge only in the decoupled state one of them exists to catch.

EveryDeclaredByteBound_FitsTheDeadline_CensusedFromTheType removes the hand-typed subject: it reads the bounds off the type's own public const long fields and requires each to fit the same allowance, so the ceiling's coverage no longer depends on any single pin naming it, and a third byte bound added later is covered without anyone remembering. It asserts a non-empty census and requires both figures by name, because a reflection filter that matches nothing reports success for having looked — and those two guards catch different mutations, as the last two rows above show.

ThePerIndexCeiling_FitsTheDeadline_OnItsOwn stays, because it is where the decoupling argument is written down; it is no longer where the coverage lives.

The census filter takes every long constant rather than every name ending in Bytes. The suffix reads more precisely and fails the wrong way — a byte bound added under some other name escapes the census and is never checked, which is green and wrong — whereas the broad filter means an unrelated long added later gets asserted against the deadline too, which is noisy and wrong and someone has to come and look at it. Fail toward the label that costs attention, not the one that costs coverage. The population today is exactly MeasureCeilingBytes and CycleMeasureBudgetBytes: the type's other constants are int (BlockSizeBytes, MeasuredBlocksPerSecond, MaxSplicedCursors) or string, and neither base class declares any.

Not verified

• The count and byte share of indexes between 1 and 2 GiB on the real target, which is the permanent loss this change accepts. It needs the pgmonitor store, and the AWS SSO token expired mid-session (Token has expired and refresh failed) with no darling-pgmon MCP in this session — so neither route was available. aws is not classifier-blocked here; the token is expired, which is a credential path not pursued. An inbox request is filed.
• The ~1,013 figure's own arithmetic. The bytes that run measured come from another lane's store read; what is independently re-derived here is the ≤1,036 blocks/s upper bound from the row's duration and the shipped gate.
• The rate as a distribution. n=1. A lower measurement will red these pins and bring the figures down again, which is the mechanism working.
• Whether a post-Rotate which indexes pg_index_bloat measures, so its deferral claim is true (#3153, #3158) #3163 (rotation-era) run has landed. The 252,940 ms row ran on a pre-rotation build, so it measured a top-of-census prefix rather than a rotating slice. That does not affect the rate — the gate's byte bound was the same 2 GiB — but it means no rotation-era duration exists yet.

No issue is referenced by a claim I did not check

CHANGELOG.md:926 contains the digits "1,013" in an unrelated Query Store row-count context. It is a numeral collision, not a second sighting of this rate.

[claude]

Reviewed. This is a shared-library change (PerformanceMonitor.Collectors), so it applies identically to Lite and Darling — no parity drift possible here, both consume the same collector code.

Checked the substance:

• MeasureCeilingBytes/CycleMeasureBudgetBytes (2 GiB → 1 GiB) and the SQL literals CeilingLiteral/CycleByteBudgetLiteral ("1073741824") are updated together and stay in lockstep, and the pre-existing TheBudgetLiterals_AgreeWithTheirConstants pin (untouched by this PR) already asserts the SQL literal strings match the public constants — so the new byte figure is automatically covered by that existing test, no need to touch it.
• Arithmetic checks out: 1 GiB = 131,072 blocks at 1,013 blocks/s ≈ 129s (< 150s half-deadline); 2 GiB at the same rate ≈ 259s (breaches it) — matches the stated rationale for why the ceiling had to come down, not just the budget.
• The upper-bound derivation (262,144 blocks / 252.94s ≈ 1,036 blocks/s) is consistent with the measured 1,013 kept in the constant.
• New tests (ThePerIndexCeiling_FitsTheDeadline_OnItsOwn, EveryDeclaredByteBound_FitsTheDeadline_CensusedFromTheType) reference the constants dynamically rather than hardcoding values, so they don't need to be touched again if the numbers move later. The reflection filter (IsLiteral + FieldType == typeof(long)) correctly picks up only MeasureCeilingBytes and CycleMeasureBudgetBytes — confirmed no other public const long fields exist on this type or its base classes that could either pollute or under-populate the census, and the "must contain both names" assertion guards against the census silently narrowing.
• No stale references to the renamed PessimisticBlocksPerSecond remain anywhere in the tree.
• No SQL injection surface — the changed literals are compile-time constants spliced into the query text, not user/runtime input.

No correctness, parity, or security issues found. Nothing to flag on T-SQL style either — this collector's query targets PostgreSQL and already follows the repo's AS-aliasing/column_name = expression conventions; this PR doesn't touch the query shape, only the numeric literals.

[claude]

Reviewed. This PR only touches doc comments, two constants (PessimisticBlocksPerSecond → MeasuredBlocksPerSecond = 1,013; MeasureCeilingBytes/CycleMeasureBudgetBytes 2 GiB → 1 GiB), their paired SQL literal strings, and the test file — no changes to the actual gate query logic (window functions, CASE arms, cursor handling all untouched).

Checked and confirmed:

• Arithmetic: 1 GiB / 8192 = 131,072 blocks; at 1,013 blocks/s ≈ 129s, under the 150s half-deadline allowance. 2 GiB at the same rate ≈ 259s, which is why the ceiling had to move rather than just the budget.
• CeilingLiteral/CycleByteBudgetLiteral SQL string literals were updated in lockstep with the C# constants (1073741824), and TheBudgetLiterals_AgreeWithTheirConstants still pins that agreement by regex against the generated SQL.
• No leftover references to the old PessimisticBlocksPerSecond name anywhere in the tree.
• New EveryDeclaredByteBound_FitsTheDeadline_CensusedFromTheType reflection census: verified neither CollectorDefinitionBase nor PostgresCollectorDefinitionBase declares any public const long fields, so the census picks up exactly MeasureCeilingBytes and CycleMeasureBudgetBytes as claimed, and the int consts (BlockSizeBytes, MeasuredBlocksPerSecond, MaxSplicedCursors) are correctly excluded by the FieldType == typeof(long) filter.
• PgIndexBloatCollector lives in the shared PerformanceMonitor.Collectors project referenced by both Lite and Darling, so this is not a Lite/Darling parity split — both apps pick up the change identically.
• No SQL injection surface: the literals interpolated into the query text are compile-time const values, not external input.

No correctness, parity, security, or performance concerns found. The self-documented "not verified" caveats in the PR description (real-target index count between 1–2 GiB, n=1 rate sample) are honestly disclosed rather than hidden, and the mitigation (band-emptiness floor, liveness guarantee) is unaffected since budget == ceiling is preserved.

[claude]

Reviewed this against CONTRIBUTING.md's parity and style rules. No correctness, parity, security, or performance-regression findings.

Parity check: PgIndexBloatCollector.cs lives in the shared PerformanceMonitor.Collectors project, referenced by both Lite/PerformanceMonitorLite.csproj and every Darling project — there's no duplicated copy of the constants or the SQL to drift. Confirmed no other file (Darling.Tests included) hardcodes the old 2_000 rate or the 2147483648 / 1073741824 byte literals, and the one Darling-side live-Postgres test that exercises this collector (PgIndexBloatBudgetLivePostgresTests) drives the count bound (200 indexes), not the byte bound, so it's insensitive to the ceiling/budget change.

Arithmetic spot-check, since the whole PR rests on it:

• 1 GiB / 8192 = 131,072 blocks; at 1,013 blocks/s ≈ 129.4s — inside the 150s half-deadline. ✓
• 2 GiB / 8192 = 262,144 blocks; at 1,013 blocks/s ≈ 258.8s — matches the "259s" cited as now-failing. ✓
• Breach point for the new 1 GiB ceiling: 131,072/150 ≈ 874 blocks/s, matching the stated 14% margin. ✓
• CeilingLiteral/CycleByteBudgetLiteral SQL string constants were updated in lockstep with the C# long constants (1073741824 in both places), so TheBudgetLiterals_AgreeWithTheirConstants stays meaningful rather than silently comparing against a stale figure.

New test (EveryDeclaredByteBound_FitsTheDeadline_CensusedFromTheType): verified by reflection that CollectorDefinitionBase/PostgresCollectorDefinitionBase declare no public const long fields and that PgIndexBloatCollector declares exactly the two the test expects (MeasureCeilingBytes, CycleMeasureBudgetBytes) — the "population today is exactly these two" claim in the PR body holds, so the census isn't vacuous and won't silently pick up unrelated constants later.

Not a T-SQL change (Postgres query embedded as a C# string, existing style untouched), so OPTION(RECOMPILE) doesn't apply here, and no missing-index DMV recommendations to worry about.

One non-blocking observation: this doesn't add a CHANGELOG.md entry, but neither did the related #3153/#3163 PRs that touched the same file, so that looks consistent with existing practice rather than an omission.