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+2-2
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