| From: | wufengwufengwufeng(at)gmail(dot)com |
|---|---|
| To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | [PATCH] Check dead heap items before marking them unused |
| Date: | 2026-06-25 08:21:11 |
| Message-ID: | CACK3murhAZUQYCqFdN7aZ_zTE8nSOyTK-g4DGj-1EpRrxaNKUQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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.
The attached patch turns the Assert into a runtime check performed before
the visibility checks and before entering the critical section, so failure is
reported as a normal ERROR rather than becoming PANIC.
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.
Cheers,
Feng Wu
From b66a54896493c741a1ee734e05d18aadd92989b2 Mon Sep 17 00:00:00 2001
From: Feng Wu <wufengwufengwufeng(at)gmail(dot)com>
Date: Thu, 25 Jun 2026 15:42:26 +0800
Subject: [PATCH] Check dead heap items before marking them unused
lazy_vacuum_heap_page() relies on an Assert() to verify that
each offset from the dead-items store is LP_DEAD with no
storage before calling ItemIdSetUnused(). In production
builds this check is compiled out.
If the invariant is ever broken by a future code change,
stale dead-items tracking, or page corruption, the function
would mark unexpected items LP_UNUSED without detecting the
problem.
Replace the Assert with an elog(ERROR) that applies in all
builds. Perform the check before the visibility checks and
before entering the critical section, so failure is reported
as a normal ERROR.
Author: Feng Wu <wufengwufengwufeng(at)gmail(dot)com>
---
src/backend/access/heap/vacuumlazy.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c
b/src/backend/access/heap/vacuumlazy.c
index 39395aed..56d605fb 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -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));
+ }
+
/*
* Before marking dead items unused, check whether the page will become
* all-visible once that change is applied. This lets us reap the tuples
@@ -2810,14 +2826,10 @@ lazy_vacuum_heap_page(LVRelState *vacrel,
BlockNumber blkno, Buffer buffer,
for (int i = 0; i < num_offsets; i++)
{
- ItemId itemid;
- OffsetNumber toff = deadoffsets[i];
-
- itemid = PageGetItemId(page, toff);
+ ItemId itemid = PageGetItemId(page, deadoffsets[i]);
- Assert(ItemIdIsDead(itemid) && !ItemIdHasStorage(itemid));
ItemIdSetUnused(itemid);
- unused[nunused++] = toff;
+ unused[nunused++] = deadoffsets[i];
}
Assert(nunused > 0);
--
2.50.1 (Apple Git-155)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Shlok Kyal | 2026-06-25 08:47:44 | Re: Support EXCEPT for ALL SEQUENCES publications |
| Previous Message | Vitaly Davydov | 2026-06-25 08:12:07 | Re: DDL deparse |