[PATCH] Check dead heap items before marking them unused

Hi,

On Thu, Jun 25, 2026 at 1:32 AM <wufengwufengwufeng@gmail.com> wrote:

While reviewing lazy_vacuum_heap_page(), I noticed that the code relies on
an Assert() before marking items from the dead-items store LP_UNUSED.

That Assert checks that each item is LP_DEAD and has no storage. In
production builds, however, the check is compiled out. If the invariant is
ever broken by a future code change, stale dead-items tracking, or page
corruption, lazy_vacuum_heap_page() can mark an unexpected item unused
without detecting the problem.

All the dead line-pointers are collected during the heap page scan
(vacrel->dead_items) and their offsets are passed to
lazy_vacuum_heap_page(), which has this invariant as an assert. I
understand that page corruption could theoretically break this
invariant, but that's a rare scenario and without a real use-case it's
hard to argue in favor of adding a runtime check.

I verified this locally by manually constructing a page with an LP_DEAD item
that still had storage. On an assert build, VACUUM trips the existing
Assert. On a production build without the patch, VACUUM succeeds and marks
the item LP_UNUSED. With the patch, VACUUM reports ERROR before modifying
the page.

I am not aware of a normal SQL-level path to create this state; the patch is
intended as a defense-in-depth check around a data-integrity invariant.

I'm having a hard time understanding how this can actually happen in a
production system. For theory's sake: say the heap page scan
identifies a dead item and adds it to the TID list. Before vacuum
reaches lazy_vacuum_heap_page(), this window could be long for tables
of hundreds of GBs or even TBs with many dead TIDs. Now, for the
invariant to break, vacuum's dead TID store would say the line-pointer
is dead, but what's read back from storage doesn't agree - and that is
exactly corruption. But for that page to be read back from storage in
the first place, it has to have been evicted and re-read, and if page
checksums are enabled, wouldn't that corruption already be caught at
read time before we ever reach lazy_vacuum_heap_page()?

@@ -2777,6 +2777,22 @@ lazy_vacuum_heap_page(LVRelState *vacrel,
BlockNumber blkno, Buffer buffer,
VACUUM_ERRCB_PHASE_VACUUM_HEAP, blkno,
InvalidOffsetNumber);

+       /*
+        * Validate all dead items before doing visibility checks or entering
+        * the critical section.  The pruning phase should have left every
+        * item in the dead-items store as LP_DEAD with no storage.  If this
+        * invariant is broken, by a bug in pruning, tracking, or page
+        * corruption, proceeding would silently free tuple storage.
+        */
+       for (int i = 0; i < num_offsets; i++)
+       {
+               ItemId          itemid = PageGetItemId(page, deadoffsets[i]);
+
+               if (!ItemIdIsDead(itemid) || ItemIdHasStorage(itemid))
+                       elog(ERROR, "unexpected non-dead or non-empty item at offset %u in
block %u of relation \"%s\"",
+                                deadoffsets[i], blkno, RelationGetRelationName(vacrel->rel));
+       }
+

This loop isn't cheap. In the worst case, all line-pointers on a page
are marked dead (a bulk deletion for example), and this loop plus the
same loop below in the critical section would run roughly 2*2K times
the number of heap pages per table.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

Thanks for looking at this.

I agree that I do not have a normal SQL-level reproducer for this state.
The motivation was not a known reachable SQL bug, but that the code is
about to perform an irreversible page change based on an invariant that
is only checked in assert builds.

On the checksum point, I think checksums only cover one subset of the
possible failures here. They would catch many on-disk page corruptions
when the page is read back, but they would not catch an internal bug that
puts the wrong offset into the dead-items store, a future pruning/tracking
bug, an in-memory page state problem, or systems running without
checksums. In those cases the existing Assert documents the invariant,
but production builds would proceed to ItemIdSetUnused().

That said, I agree with your concern that the patch adds another pass
over the dead offsets. My intent in placing the check before the critical
section was to report a plain ERROR before modifying the page, but I
understand that doing so has a cost on pages with many dead line pointers.

If the extra pass is considered too expensive for this path, perhaps the
right answer is to keep this as an Assert-only invariant, or to look for a
cheaper place to validate it. I would be interested in whether others
think this invariant violation should be treated as corruption worth a
runtime check, or whether the current Assert is considered sufficient.