On Sat, Mar 7, 2026 at 12:25 AM Robert Haas <[email protected]> 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. >
Understood, the patch looks good to me, thanks. Regards, Amul
