Parallel/morsel vacuum: deferred-cleanup pages double-count dead_items_info.num_items on a coverage-guard trip

[MauricioPerera]

Summary

crates/backend/access/heap/vacuumlazy/src/morsels.rs implements pgrust's novel parallel/morsel-driven heap-scan phase of VACUUM (gated behind PGRUST_RUNTIME_VACUUM=1, default OFF, experimental). It has a documented "fail-closed coverage guard" (§5.2, header comment lines 19-46): if verify_prefix_coverage detects a scan-coverage hole (e.g. a lost worker/Local), it sets coverage_hole/coverage_failed, rewinds resume_g back to the hole (morsels.rs:397-402), and hands the remainder off to the serial vacuum arm to re-scan from there — a safe, documented, "re-scanning is idempotent" recovery strategy for page pruning and relfrozenxid.

However, the leader's processing of workers' deferred blocking-cleanup pages (§5.3 — pages a worker skipped because it couldn't get a non-blocking cleanup lock) is not gated on coverage_failed:

// morsels.rs:481-487
let mut deferred: Vec<BlockNumber> =
    locals.iter().flat_map(|l| l.deferred_cleanup.iter().copied()).collect();
deferred.sort_unstable();
for blk in deferred {
    leader_deferred_block(vacrel, &source, blk)?;
}
...
if coverage_failed {                      // morsels.rs:508
    clock.emit(vacrel.rel.name(), rounds, k, "coverage-hole");
    return Ok(ScanHandoff::Resume { block: resume_block, next_fsm });
}

deferred is collected from all surviving workers' Locals regardless of whether their claims lie above or below the (possibly rewound) resume_g, and leader_deferred_block fully prunes/freezes each such block and adds its dead TIDs to vacrel.dead_items before the coverage_failed early-return.

Concrete trigger scenario

Workers claim granules off a shared cursor in completion order, not claim order. If:

1. A worker owning a lower-numbered granule range is lost (a genuine crash-recovery gap, or via the test-only fault hook PGRUST_RUNTIME_VACUUM_FAULT=lose-local), while
2. A different worker, owning a higher-numbered granule range, completes first and records a deferred blocking-cleanup page in local.deferred_cleanup (§5.3 residue),

then in the same round: the coverage guard trips and rewinds resume_g to below that higher-numbered deferred block's granule — but the deferred block has already been fully processed by leader_deferred_block (dead TIDs added, dead_items_info.num_items incremented) at lines 481-487, before the rewind's consequence (handing off to the serial arm) takes effect. The serial fallback then resumes scanning from the rewound resume_g, which is before that block, and will re-encounter and re-prune the same block.

dead_items_add (vacuumlazy/src/lib.rs:774-788) calls dead_items.set_block_offsets(blkno, offsets) (an idempotent replace-by-block operation) but then does:

dead_items_info.num_items += offsets.len() as i64;   // lib.rs:781

This addition is not deduplicated per block — re-processing the same block a second time (once via the deferred-cleanup path, once via the serial re-scan) double-adds that block's dead-tuple count to num_items, even though the actual TidStore content for that block is correctly replaced (not duplicated).

Why this matters

This corrupts the invariant checked elsewhere in the same file:

debug_assert_eq!(tids.len() as i64, vacrel.dead_items_info.num_items);  // collect_dead_tids, lib.rs:816

In a debug build this trips the assertion (a crash). In a release build the assertion is compiled out, so this becomes a silently-wrong dead_items_info.num_items — which feeds pgstat_progress_update_multi_param (progress reporting) and potentially downstream dead-tuple-count-based decisions (e.g. whether to run index vacuuming, memory/space accounting for the dead-items store). It's a genuinely incorrect count, not just cosmetic, though I want to flag explicitly that I have not traced every consumer of dead_items_info.num_items to confirm the full blast radius beyond progress reporting and the debug assertion.

This is worth noting alongside the header comment's own reasoning about overcounting (lines 393-397: "counter folds above the hole may overcount page visits, which reltuples estimation tolerates and advancement no longer consumes") — that comment explicitly anticipates and tolerates overcounting for reltuples estimation and relfrozenxid advancement (both suppressed/tolerant by design on a coverage trip), but does not appear to account for this specific double-count of dead_items_info.num_items via the deferred-cleanup path.

Suggested fix

Filter deferred entries to only those >= resume_g before processing when a coverage hole is detected — i.e. compute resume_g (or at least know a coverage hole occurred) before draining deferred, and skip/re-defer entries at or above the (final) resume point, mirroring how skippedallvis_before(resume_g) (morsels.rs:411) already correctly filters skip-decisions relative to the resume point.

Environment / where found

Found via static source review of a fresh clone of this repo. This feature is explicitly experimental and opt-in (PGRUST_RUNTIME_VACUUM=1, default OFF), and the existing test suite (vacuum_morsels/src/tests.rs) tests the coverage-guard and merge logic only via synthetic in-process simulation — it does not exercise real threads/buffers or the leader_deferred_block/coverage interaction described here (no test sets PGRUST_RUNTIME_VACUUM_FAULT=lose-local in combination with a deferred-cleanup page above the resulting resume point). I have not attempted to build/run pgrust myself or construct a live repro; this report is based on reading crates/backend/access/heap/vacuumlazy/src/morsels.rs and crates/backend/access/heap/vacuumlazy/src/lib.rs directly.