Re: Adding vacuum test case of setting the VM when heap page is unmodified

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.