Thanks for the review!
On Tue, Dec 16, 2025 at 11:39 AM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:
>
>> While working on a patch to set the VM in the same WAL record as
>> pruning and freezing [1], I discovered we have no test coverage of the
>> case where vacuum phase I sets the VM but no modifications are made to
>> the heap buffer (not even setting PD_ALL_VISIBLE). This can only
>> happen when the VM was somehow removed or destroyed.
>
> +1 for adding the test, but IIUC PD_ALL_VISIBLE is being set in this
> case during the "vacuum test_vac_unmodified_heap;" because
> VM bit is not set (as we truncated VM) and presult.all_visible is true as well ,
> so it goes in if (!all_visible_according_to_vm && presult.all_visible), where its
> doing these, this was the flow i observed while trying to understand the
> patch by running the given test, please correct me if I'm wrong.
>
> PageSetAllVisible(page);
> MarkBufferDirty(buf);
> old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
> InvalidXLogRecPtr,
> vmbuffer, presult.vm_conflict_horizon,
> flags);
You're right. In the current code, it will correctly mark the buffer
dirty -- even if PD_ALL_VISIBLE was already set. I'm suggesting we add
the test to guard against someone trying to optimize this case and not
set PD_ALL_VISIBLE and mark the buffer dirty if PD_ALL_VISIBLE is
already set and the heap page requires no modification.
While writing another patch, I did try this optimization and didn't
see any test failures. After a conversation off-list with Andres, he
reminded me that buffers always must be marked dirty before
registering them with XLogRegisterBuffer() (unless REGBUF_NO_CHANGES
is passed) or an assert will be tripped. That is how I realized we
didn't have coverage of the case where the heap buffer doesn't need to
be modified.
Adding to the confusion, in the fourth branch of the if statement for
setting the VM in lazy_scan_prune():
else if (all_visible_according_to_vm && presult.all_frozen &&
!VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer))
we conditionally mark the heap buffer dirty
if (!PageIsAllVisible(page))
{
PageSetAllVisible(page);
MarkBufferDirty(buf);
}
This doesn't trip the assert because we heap buffer is always already
marked dirty when we enter this branch. However, I think that it is a
coincidence that this works out and was not intended by the author.
And because the heap buffer is always dirty anyway, we don't save
anything with this if statement and only create confusion.
As such, I've proposed in that thread that we refactor this code to a
single visibilitymap_set() case
if ((presult.all_visible && !all_visible_according_to_vm) ||
(presult.all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer)))
which unconditionally sets PD_ALL_VISIBLE and marks the heap buffer dirty.
That patch is 0001 in the v27 posted here [1].
- Melanie
[1] https://www.postgresql.org/message-id/CAAKRu_ayWLg%3DWDGZZfSPWf0KjPM8u%3DLBb0D6XaEWyx2_YFFwAQ%40mail.gmail.com
