Bound pg_index_bloat by what pgstatindex is applied to, not by a join qual

[erikdarlingdata]

pg_index_bloat's size ceiling and both work budgets were quals on a LEFT JOIN LATERAL to pgstatindex. That placement bounds nothing: the planner cannot skip an inner side it has not evaluated, so the function is invoked once per candidate row and the qual only decides whether its answer is kept. Every attempt read every b-tree index on the instance — 1,517 indexes, 461 GB on the reported target — while every skipped_reason in the output was correct. So the collector has been indistinguishable from a working one in everything except whether it ever returns, across #2561's original shape, #2617's count budget and #2997's byte budget alike.

What changed

The bounds select the relation pgstatindex is applied to. A fenced in_budget relation carries the ceiling, the count bound and the byte bound; measured cross-joins the function to that relation only; the final SELECT still drives off the ungated ranked and joins the measurements back. Every candidate returns exactly one row, unmeasured ones with their reason, which is the property the LEFT JOIN LATERAL was there to provide and the reason a bare WHERE on the output would be wrong.

This is the same category as the btree filter three lines above it, which is fenced with OFFSET 0 for the same stated reason — correctness should not depend on plan shape when the failure is total. The budget was the one bound in the query that still rested on it.

The ceiling and the cycle budget both drop from 20 GB to 2 GB. They move together because the pinned invariant forbids the band between them, and 2 GB is derived rather than guessed: pgstatindex walks the index one block at a time with no prefetch, so its cost is a count of potentially-synchronous single-block reads, and a bulk-throughput estimate overstates the achievable rate by orders of magnitude on network-attached storage. 2 GB is 262,144 blocks, ~131 s at a pessimistic 2,000 blocks/s, inside half the 300 s deadline. The deadline is untouched, per the collector's own error text.

The coverage that was missing, and where it now lives

PgIndexBloatBudgetLivePostgresTests (Darling/Darling.Tests/) runs the shipped query against a live PostgreSQL and counts Actual Loops on the pgstatindex function-scan node of its own EXPLAIN ANALYZE. Three numbers come out of the query itself rather than being written into the test: C candidates returned, M reported as measured, L invocations. It asserts L == M, and M < C so the first assertion cannot pass vacuously — where everything fits the budget, L == M == C holds under the broken shape too.

No assertion on query text can distinguish these two shapes, which is why both prior budgets shipped pinned and inert. The broken one and the fixed one differ in what the executor does, not in what the SQL says. Measured on PostgreSQL 17.11 with 215 candidates against the untouched 200-index count bound:

	origin/dev	this branch
C candidates	215	215
M measured	200	200
L pgstatindex invocations	215	200
plan	Nested Loop Left Join, Rows Removed by Join Filter: 15	Filter on the input, then loops=200

Nothing about the query is rewritten for that test — not the literals, not the shape. It belongs in Darling on its merits: DarlingWorker is what dispatches this collector, the target that has never returned a row is a Darling target, and this is the only suite with a live PostgreSQL to execute against.

Two things that test has to get right to run at all, both of them properties of the store rather than of the collector. It creates pgstattuple with SCHEMA public, because the store's search_path is collect, config, public and a bare CREATE EXTENSION therefore installs into collect — after which the collector's deliberately-qualified public.pgstatindex raises 42883 while the extension is by every other measure installed. The precondition is asserted as the function being callable where the collector calls it, read from the catalog, rather than as CREATE EXTENSION having returned without error; those two come apart exactly here. Teardown goes through LiveStoreCleanup.RunAsync with a bodySucceeded flag (#1902), so a cleanup failure cannot replace the body's exception — verified by the mutation below, which reports the assertion rather than cleanup noise and still drops the probe schema.

What else is pinned

TheCycleBudget_FitsTheDeadline_AtThePessimisticBlockRate relates the budget, the deadline and a named rate assumption — three numbers that each had their own justification and nothing comparing them. It is the pin that makes raising any one of them argue against the other two.

NoWorkBoundSitsOnTheNullableSideOfAnOuterJoin is stated as an absence rather than a shape, because this failure has now arrived twice by two different expressions and the next one will not look like either.

TheFunctionCall_IsAppliedToTheGatedRelation, TheGateRelation_IsFencedLikeTheCandidateSet and EverySkippedIndexStillReturnsARow_ThroughTheOuterJoin cover the restructure; TheCycleHasAWorkBudget_NotJustAPerIndexCeiling, TheCycleBudget_BoundsBytes_NotJustIndexCount and OverBudgetIndexesAreReturnedWithAReason previously asserted the ON-clause placement as the fix and now read the bounds out of the gate relation instead.

Red-first, both halves, by mutation against the committed branch. Reverting only the two values fails the arithmetic pin (1311s against 150s allowed). Splicing in origin/dev's own QueryText and keeping the values fails 7 of the 21 Lite pins and fails the live test with Expected: 200, Actual: 215 — it names the 15 indexes that were read and discarded.

Verification scope

The live test and the 21 PgIndexBloatCollectorDefinitionTests facts were executed on net10.0 by compiling the actual test files into console harnesses, against PostgreSQL 17.11; both suites target net10.0-windows and cannot run on macOS. CI is the arbiter for the suites as a whole.

The acceptance criterion is not verifiable pre-merge. It requires the next drifted attempt after a deploy to complete SUCCESS with a non-zero row count, so it is a POST-merge check. What is verified here is that the bound now binds, and that it binds without changing the answer. Whether 2 GB fits the deadline on the real target is an argument from a cost model, not a measurement — no SUCCESS row has ever supplied a duration for this collector, and producing the first one is what turns the next adjustment into a measurement.

Per #3109's own follow-up, verify against the launch instant, not the completion instant: the drift prediction holds only while every attempt dies on the same deadline, and the first attempt that does not breaks it.

The bar for the post-merge check — the pre-registered one no longer applies

#3109's criteria (~1,517 rows, ~200 carrying avg_leaf_density) were set against a 20 GB ceiling. Both bounds are now 2 GB, so the second figure is wrong in a way that matters: a run reporting ~200 measured would mean the byte budget is NOT binding, which is the defect this PR fixes, not the success it used to describe. Stated here so the check has something to fail against rather than reading "it returned some rows" as success.

What is unchanged, and still the right first assertion:

• ~1,517 rows. The budget removes no rows — every valid b-tree index outside the system schemas returns one, measured or with a reason. A row count far below this means something other than the budget is filtering.

What changes, with the arithmetic behind it. The 200 largest sub-20 GB indexes on that target totalled 286 GB, a mean of ~1.43 GB, so the distribution is dense right where the new 2 GB ceiling cuts. Largest-first into a 2 GB budget therefore admits very few:

• Rows carrying avg_leaf_density: expected single digits, floor 1, hard ceiling 200. Not ~200.
• SUM(index_bytes) WHERE avg_leaf_density IS NOT NULL ≤ 2,147,483,648. The sharpest check available and the one that tests this PR's actual claim — it is the budget, read straight off the stored rows. Over it means the bound is not binding in production.
• MAX(index_bytes) WHERE avg_leaf_density IS NOT NULL < 2,147,483,648, the ceiling.
• Zero rows with skipped_reason IS NULL AND avg_leaf_density IS NULL — every admitted index came back with a measurement.
• The largest indexes now carry the ceiling reason rather than the budget one, which is the honest label for permanently-unmeasured and the capability this trade gives up.

Three outcomes that must NOT be read as success:

• rows > 0 with zero carrying density — the budget admitted nothing, a distinct failure with its own reason string.
• Measured bytes over 2 GB — labels correct, bound not binding.
• sql_duration_ms at or near 300,000 — still does not fit, and the next move is a smaller budget, not a longer deadline.

sql_duration_ms on the first SUCCESS is the number that retires the guesswork. ~131 s would vindicate the pessimistic 2,000 blocks/s; far below it means the rate assumption was too conservative and the budget can be raised on a measurement for the first time in this collector's life.

Two cautions for whoever runs the check. Verify against the launch instant, not completion — the drift line holds only while every attempt dies on the same deadline. And prefer collection_log plus a direct row count over reading only through get_pg_index_bloat: an empty b-tree index reports avg_leaf_density as NaN, and the reader's reclaimable-bytes cast raises bigint out of range on such a row, so a successful collection can present as a broken read. Filed separately; this change is what makes it live, since the collector previously returned nothing to store. Where it bites is specific, and it is not the target in #3109. An empty index is 8 KB and therefore sorts LAST under largest-first, so it is admitted only where a database's entire sub-ceiling index footprint fits inside the 2 GB budget — small databases, which this per-database collector meets on almost any real cluster. On a target whose sub-ceiling indexes run to hundreds of GB the budget is spent long before it reaches an 8 KB index, so the ordering rules it out there. Do not read a clean get_pg_index_bloat on the big target as evidence the NaN defect is absent.

Deliberately left out

Rotation of the measured set. Coverage of the tail still depends on it and it still does not happen, so an index below the ceiling but past the budget is reported as deferred every run. That is recorded in CycleMeasureBudgetBytes' own summary as a standing gap rather than fixed here: it needs stored per-server state to be anything other than a clock trick, and it does not help the acceptance criterion, which is a first success.

Raising the budget on the strength of the new headroom. The point of the small value is to produce the sql_duration_ms that makes the next choice a measurement.

The Darling PostgreSQL tests path gate, which is #3116 and not this PR's to change. Worth stating plainly: on the first push here that job reported success in 17 seconds with Run Darling PG tests skipped, because the filter is Darling/**/!(*.md) and this change began life entirely outside Darling/. The Darling test above makes the gate true as a side effect, but it earns its place on coverage grounds, not as a gate flip.

CHANGELOG entry text

Not applied to CHANGELOG.md in this branch on purpose: every open lane appends to the same [Unreleased] block, so each one that edits the file conflicts with the rest. The entry is stated here for whoever sequences the merge.

### Fixed

• pg_index_bloat's size ceiling and work budgets are enforced by selecting the relation pgstatindex is applied to, rather than by quals on a LEFT JOIN LATERAL to it. Placed on the join, they bounded nothing — the function ran once per candidate row and the qual only chose whether to keep its answer — so every attempt read every b-tree index on the instance while reporting correct skip reasons for the ones it had already paid for. The per-index ceiling and the cycle budget drop to 2 GB together, sized against a per-block read rate rather than a bulk-throughput estimate; a new pin relates the budget, the command deadline and that rate so none of the three can move alone; and a live-PostgreSQL test counts pgstatindex invocations on the shipped query, which is the only way to tell a budget that bounds reads from one that labels rows.

[claude]

Reviewed the diff (PgIndexBloatCollector.cs + its test file). The core fix is sound: the old shape put the three work bounds as an ON-clause qual on LEFT JOIN LATERAL public.pgstatindex(...), which cannot bound anything — a lateral function on the inner side of a join is invoked once per outer row regardless of the join qual, so the qual only decided whether to keep the (already paid-for) result. Moving the bounds into a fenced in_budget CTE that is then CROSS JOIN LATERAL'd, with measured results joined back onto the ungated ranked set via a plain LEFT JOIN, is the correct fix: pgstatindex can now only ever be invoked once per admitted row, independent of plan shape (the OFFSET 0 fence blocks CTE inlining regardless, consistent with the existing candidates CTE pattern). I traced the new query text end-to-end (in_budget's WHERE matches the old ON's three predicates exactly, index_oid join-back is 1:1, the skipped_reason CASE arms are still the logical complements of the in_budget filter) and didn't find a correctness bug. The new/updated tests (TheFunctionCall_IsAppliedToTheGatedRelation, NoWorkBoundSitsOnTheNullableSideOfAnOuterJoin, TheGateRelation_IsFencedLikeTheCandidateSet, EverySkippedIndexStillReturnsARow_ThroughTheOuterJoin, TheCycleBudget_FitsTheDeadline_AtThePessimisticBlockRate) exercise exactly the distinction that mattered and the arithmetic in the last one checks out against the stated constants (262,144 blocks / 2,000 blocks/s ≈ 131s ≤ 150s = half of the 300s deadline). No Lite/Darling parity concern — this collector lives in the shared PerformanceMonitor.Collectors library both apps consume, and no Darling-specific test duplicates this logic. No security or injection concerns (all literals are compile-time constants, not user input).

Two minor nits, neither blocking:

1. CHANGELOG.md wasn't touched. The PR description includes a ready-to-use "CHANGELOG entry text" section, but the diff only contains the two .cs files — the immediately-preceding merged PR (Part of #3099 #3108) did update CHANGELOG.md's [Unreleased] section, so this looks like an oversight rather than a deliberate skip.
2. Stale "20 GB" references in Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs doc comments at lines 203 ("the 20 GB ceiling bounds one index") and 315 ("71 GB against a 20 GB budget") — both now inconsistent with the 2 GB ceiling/budget this same PR introduces. They're outside the diff's hunks so I couldn't leave inline comments on them, but worth a follow-up cleanup since they're in the very file this PR spent effort keeping accurate everywhere else.

[claude]

Reviewed the query restructure, the new pins, and the live-Postgres test. This is solid — the in_budget CTE genuinely fixes the bug (the fenced WHERE now gates the relation pgstatindex is applied to, rather than sitting as an ON-clause qual on a LEFT JOIN LATERAL that the planner could — and did — ignore for cost purposes), the boundary conditions in the final CASE are the exact complements of the in_budget filter (< vs >=, <= vs >, <= vs >), and the new live-Postgres test asserting L == M (with M < C so it can't pass vacuously) is exactly the right test for a bug that no query-text assertion could have caught. Confirmed PgIndexBloatCollector is Darling-only in practice (CollectorTargetEngine.PostgreSql is never set in Lite — see Lite/Mcp/McpEngineCapability.cs:60), so there's no Lite/Darling parity gap to flag here despite the shared collector library.

Two small things worth a look before merge:

1. Stale figures in Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs. Lines 203 and 315 still say "the 20 GB ceiling" and "71 GB against a 20 GB budget" — leftover from before this PR dropped MeasureCeilingBytes/CycleMeasureBudgetBytes to 2 GB. They're outside the diff so I couldn't anchor inline comments on them, but worth a follow-up edit since this codebase's comments are relied on to carry the correct numbers.
2. CHANGELOG.md wasn't touched. The PR description has a fully-written ### Fixed entry ready to paste in, but the diff doesn't include a CHANGELOG.md change. Given how consistently every other entry in that file corresponds to a merged PR, this looks like it just needs to be carried over before merge.

Everything else — the 2 GB derivation (262,144 blocks at a pessimistic 2,000 blocks/s ≈ 131s, inside half the 300s deadline), the TheCycleBudget_FitsTheDeadline_AtThePessimisticBlockRate pin tying budget/deadline/rate together, and the SQL style (AS-aliasing, block-comment-only, AND alignment, OFFSET 0 fencing consistent with the existing candidates CTE) — all check out.

[claude]

Reviewed the diff (collector SQL + both test suites). Summary:

Correctness — the fix is sound. The core bug (budget quals on the ON clause of a LEFT JOIN LATERAL to pgstatindex) is real: Postgres can't skip an unevaluated inner side of a join, so the qual only filtered the output, not the input. Moving all three bounds (index_bytes < ceiling, size_rank <= 200, measured_bytes_through_here <= budget) into a WHERE on a fenced in_budget CTE, then CROSS JOIN LATERAL-ing only that relation into pgstatindex, and rejoining the full ranked set via a plain LEFT JOIN measured ON m.index_oid = k.index_oid for the final output — that's the right shape. Verified by hand:

• Every index_oid is unique (from pg_class.oid), so the final LEFT JOIN can't fan out or drop rows.
• Skipped indexes still get exactly one output row (via the outer join), carrying skipped_reason and NULL measurements — matches the "never silently skipped" invariant elsewhere in this collector.
• in_budget's OFFSET 0 fence is the same idiom already used on the candidates CTE a few lines up, so it's consistent with existing convention rather than a new pattern.
• The ranked CTE's window function still runs over all candidates (catalog-only, cheap) — only the actual page-reading call (pgstatindex) is gated, which is the right thing to gate.

Budget math checks out. MeasureCeilingBytes = CycleMeasureBudgetBytes = 2 GiB = 2,147,483,648, matching both CeilingLiteral and CycleByteBudgetLiteral. 2 GiB / 8192-byte blocks = 262,144 blocks; at 2,000 blocks/s that's ~131s, inside half of the 300s CommandTimeoutSecondsOverride (150s) — matches the new TheCycleBudget_FitsTheDeadline_AtThePessimisticBlockRate pin.

Lite/Darling parity — no drift. PgIndexBloatCollector is the single shared definition in PerformanceMonitor.Collectors, consumed by both apps through CollectorCatalog; there's no separate Lite copy of this query to fall out of sync. DarlingPgIndexBloatReader reads the stored columns by name, and the final SELECT's column list/names are unchanged (only the source alias moved from s. to m.), so the storage/reader contract is untouched.

Tests. The Lite.Tests regex pins were reworked to assert on the new shape (in_budget body specifically, absence of any outer JOIN LATERAL, the plain equi-join back) rather than re-testing string presence alone — good, since the old pinned assertions are exactly what let both prior "budgets" ship as no-ops. The new Darling.Tests live-Postgres test is the piece that actually proves the fix: it counts Actual Loops on the pgstatindex function-scan node via EXPLAIN (ANALYZE, FORMAT JSON) and asserts L == M with M < C so it can't pass vacuously. That's the only way to distinguish "labeled" from "bounded," as the PR description notes, and the test earns that claim.

Security/style. No user input reaches the query (all bounds are compile-time constants substituted into the SQL string); the new test file only ever writes to a hardcoded, test-only schema. Style (uppercase keywords, AS table aliases, AND alignment, /* */ comments, trailing commas) is consistent with the surrounding file and the rest of this collector.

No correctness, parity, security, or performance issues found. This is a substantial and well-verified fix.

[claude]

Reviewed the collector rewrite and both test additions.

Correctness of the core fix: the restructure is sound. in_budget now correctly gates the relation pgstatindex is applied to (WHERE on a materialized CTE, fenced with OFFSET 0), measured cross-joins only that gated set, and the final SELECT drives off the ungated ranked with a plain LEFT JOIN back onto measured. I checked the three gate predicates (index_bytes < ceiling, size_rank <= budget, measured_bytes_through_here <= byte budget) against the CASE's skipped_reason arms and they're exact complements with matching boundary operators (< vs >=, <= vs >), so there's no off-by-one drift between what's admitted and how it's labeled. index_oid is unique per candidate set within one per-database run, so the final LEFT JOIN measured AS m ON m.index_oid = k.index_oid can't fan out rows. The 2 GB ceiling/budget equality is preserved (still pinned by TheCycleBudget_IsNeverBelowThePerIndexCeiling), and the new TheCycleBudget_FitsTheDeadline_AtThePessimisticBlockRate pin checks out arithmetically (262,144 blocks / 2,000 blocks/s ≈ 131s, under the 150s half-deadline).

Live-Postgres test: PgIndexBloatBudgetLivePostgresTests is a good addition — it's the right response to "no query-text assertion can distinguish a budget that bounds reads from one that only labels rows." The pgstatindex-resolves-in-public precondition check (reading the catalog rather than trusting CREATE EXTENSION's exit code) and routing teardown through LiveStoreCleanup.RunAsync both match established patterns elsewhere in Darling.Tests. One thing worth noting for whoever runs this in CI: the collector's candidate query scans every non-system-schema btree index in the current database, not just the probe schema, so C/M/L will include whatever else lives in the shared live-postgres database at test time. That's presumably fine given the [Collection("live-postgres")] serialization, but it means the test's actual runtime scales with the store's real schema size, not just ProbeIndexCount.

One correctness note posted inline: the reduction to a 2 GB ceiling means avg_leaf_density will come back populated on real targets for the first time in this collector's life, which makes a pre-existing, separately-filed bug (NaN::bigint overflow in DarlingPgIndexBloatReader's reclaimable-bytes cast, for empty indexes) much more likely to actually fire. Not asking for it to be fixed in this PR since it's out of scope by the PR's own description, but flagging the sequencing risk.

Parity: no concerns. This only touches the shared PerformanceMonitor.Collectors definition plus Lite.Tests (text-level pins) and a new Darling.Tests live-execution test — no schema/migration/interface change, so the Two-Store Parity rules in CONTRIBUTING.md don't apply here.

Style: T-SQL style guide doesn't apply (this is a PostgreSQL query embedded in C#), and the AS-aliasing / comment conventions used match the rest of this file and the codebase's other Postgres collectors.

No missing-index-DMV concerns (not applicable — this is a Postgres bloat collector) and no security issues (all dynamic SQL in the new test uses compile-time constants, not external input).