Re: Adding vacuum test case of setting the VM when heap page is unmodified - Mailing list pgsql-hackers

On Tue, 16 Dec 2025 at 22:17, 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.
>
> 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
>
>


Hi!

I did run a test and this indeed triggers assertion if somebody writes
something like [0]. Code at [0] works (although never testes) only
because is passed
InvalidXLogRecPtr as recptr to visibilitymap_set. Maybe it is worth to
add comment nearby  this

```
/*
* Avoid relying on all_visible_according_to_vm as a proxy for the
* page-level PD_ALL_VISIBLE bit being set, since it might have become
* stale -- even when all_visible is set
*/

```
To explain why is it OK to make conditional MarkBufferDirty?


[0] https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html#2209

--
Best regards,
Kirill Reshke