RelationGetNumberOfBlocks called before vacuum_get_cutoffs
Commit 052026c9b9 made heap_vacuum_rel call RelationGetNumberOfBlocks
before it calls vacuum_get_cutoffs -- it swapped the order. This is
wrong, as explained by an intact comment above the call to
vacuum_get_cutoffs.
In short, there is now a brief window during which the relation can be
extended that'll allow heap pages with tuple headers < VACUUM's
OldestXmin to be created, which are overlooked by that same VACUUM
(they're beyond the same VACUUM's rel_pages). As a result of all this,
VACUUM might advance pg_class.relfrozenxid to a later/younger value
than a remaining/unfrozen XID value from one of these unscanned heap
pages.
--
Peter Geoghegan
On Sun, Jun 1, 2025 at 12:07 PM Peter Geoghegan <pg@bowt.ie> wrote:
Commit 052026c9b9 made heap_vacuum_rel call RelationGetNumberOfBlocks
before it calls vacuum_get_cutoffs -- it swapped the order. This is
wrong, as explained by an intact comment above the call to
vacuum_get_cutoffs.In short, there is now a brief window during which the relation can be
extended that'll allow heap pages with tuple headers < VACUUM's
OldestXmin to be created, which are overlooked by that same VACUUM
(they're beyond the same VACUUM's rel_pages). As a result of all this,
VACUUM might advance pg_class.relfrozenxid to a later/younger value
than a remaining/unfrozen XID value from one of these unscanned heap
pages.
Thanks for the report. That was a dumb mistake. There was no reason
for me to move the line up as you can see in the diff -- it must have
been unintentional. I'll push the fix tomorrow.
I started to feel like I ought to write a TAP test, but I'm hesitant
to add a whole new TAP test for a case when a comment should have been
sufficient deterrent.
- Melanie
On Sun, Jun 1, 2025 at 1:51 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
Thanks for the report. That was a dumb mistake. There was no reason
for me to move the line up as you can see in the diff -- it must have
been unintentional.
I have to imagine that you moved rel_pages initialization back so that
it took place next to the initialization of other, similar BlockNumber
fields from LVRelState. IIRC I wrote a comment about this issue at
least in part because I understood the temptation to do that.
I'll push the fix tomorrow.
Cool.
--
Peter Geoghegan
On Sun, Jun 1, 2025 at 2:23 PM Peter Geoghegan <pg@bowt.ie> wrote:
I have to imagine that you moved rel_pages initialization back so that
it took place next to the initialization of other, similar BlockNumber
fields from LVRelState. IIRC I wrote a comment about this issue at
least in part because I understood the temptation to do that.
That sounds right.
I'll push the fix tomorrow.
Cool.
Attached what I plan to push shortly.
- Melanie
Attachments:
Correct-heap-vacuum-boundary-state-setup-ordering.patchtext/x-patch; charset=US-ASCII; name=Correct-heap-vacuum-boundary-state-setup-ordering.patchDownload
From 731dc1ef1285e167d877992eab64ebd644599926 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 2 Jun 2025 10:08:30 -0400
Subject: [PATCH v1] Correct heap vacuum boundary state setup ordering
052026c9b9 mistakenly reordered setup steps in heap_vacuum_rel(),
incorrectly moving RelationGetNumberOfBlocks() before
vacuum_get_cutoffs(). OldestXmin must be determined before
RelationGetNumberOfBlocks() calculates the number of blocks in the
relation that will be vacuumed. Otherwise tuples older than OldestXmin
may be inserted into the end of the relation into blocks that are not
vacuumed. If additional tuples newer than those inserted into unscanned
blocks but older than OldestXmin are inserted into free space earlier in
the relation, the result could be advancing pg_class.relfrozenxid to a
newer value than an unfrozen XID in one of the unscanned heap pages.
Assigning an incorrect relfrozenxid can lead to data loss, so it is
imperative that it correctly reflect the oldest unfrozen xid.
Reported-by: Peter Geoghegan <pg@bowt.ie>
Author: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzntqvVEdbbpqG5JqSZGuLWmy4PBfUO-OswfivKchr2gvw%40mail.gmail.com
---
src/backend/access/heap/vacuumlazy.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 708674d8fcf..09416450af9 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -757,7 +757,6 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
vacrel->vm_new_visible_pages = 0;
vacrel->vm_new_visible_frozen_pages = 0;
vacrel->vm_new_frozen_pages = 0;
- vacrel->rel_pages = orig_rel_pages = RelationGetNumberOfBlocks(rel);
/*
* Get cutoffs that determine which deleted tuples are considered DEAD,
@@ -776,7 +775,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
* to increase the number of dead tuples it can prune away.)
*/
vacrel->aggressive = vacuum_get_cutoffs(rel, params, &vacrel->cutoffs);
+ vacrel->rel_pages = orig_rel_pages = RelationGetNumberOfBlocks(rel);
vacrel->vistest = GlobalVisTestFor(rel);
+
/* Initialize state used to track oldest extant XID/MXID */
vacrel->NewRelfrozenXid = vacrel->cutoffs.OldestXmin;
vacrel->NewRelminMxid = vacrel->cutoffs.OldestMxact;
--
2.34.1
On Mon, Jun 2, 2025 at 10:27 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
Attached what I plan to push shortly.
Looks good to me.
--
Peter Geoghegan