Thread

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

Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-12-17T17:06:31Z

On Tue, Dec 16, 2025 at 10:47 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:

> 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.
>

Makes sense, after this clarification, I have tested the patch,
LGTM.

-- 
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/