On 06/03/2026 20:55, Robert Haas wrote:
On Tue, Mar 3, 2026 at 11:50 PM Amul Sul <[email protected]> wrote:
I think the fix will be to correct the wal summary entry that records
an incorrect truncation limit for the VM fork. Attached are the
patches: 0001 is a refactoring patch that moves the necessary macro
definitions from visibilitymap.c to visibilitymap.h to correctly
calculate the VM fork limit recorded in the wal summary file, and 0002
provides the actual fix.
I don't like exposing those macros to the rest of the system, because
they're not named very generically.
Also, I don't think that using HEAPBLK_TO_MAPBLOCK() directly is
correct. That macro returns the visibility map page that contains the
VM bit for the indicated heap block number, but that's not what we
need here. For example, if we truncate the heap to a length of 1,
HEAPBLK_TO_MAPBLOCK() will return 0, but if we set the limit block for
the VM to zero, that means the VM was truncated away entirely, which
in this scenario wouldn't be the case. So, instead of computing too
large a value for the VM's limit block, I think your patch would cause
us to compute too small a value for the VM's limit block.
The question we need to answer here is: if we entire remove all heap
blocks >= N, then for what value of M do we remove all visibility map
blocks >= M? The answer is that if the the N (the heap block limit) is
a multiple of HEAPBLOCKS_PER_PAGE, then it's just
N/HEAPBLOCKS_PER_PAGE. Otherwise, it's one more, because we need an
extra VM page as soon as it's necessary to use at least one bit on
that page. So, basically, we need to divide by HEAPBLOCKS_PER_PAGE and
round up.
So here's my attempt at a patch. Instead of exposing
HEAPBLK_TO_MAPBLOCK() etc., I added a new helper function,
visibilitymap_truncation_length. It's just a wrapper around a new
macro HEAPBLK_TO_MAPBLOCK_LIMIT(), but I think keeping the exposed
symbols having visibilitymap in the name is a good idea. Also, I added
a test case, and I have verified that this test case passes with the
fix and fails without it.
For 'master', I wonder if we should change the 'xl_smgr_create' record
format to directly include the new sizes of all of the truncated forks.
It's always felt error-prone to me that the redo function recomputes
those. It's a little more complicated for the redo function, as it needs
to also clear out part of the last remaining page, but that information
could also be included in the WAL record directly.
- Heikki
