Horton

Thread

  1. Adding vacuum test case of setting the VM when heap page is unmodified

    Melanie Plageman <melanieplageman@gmail.com> — 2025-12-10T17:51:01Z

    Hi,

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.

Currently, we require the heap buffer to be marked dirty even if it is
unmodified because we add it to the WAL chain and do not pass
REGBUF_NO_CHANGES. (And we require adding it to the WAL chain because
we update the freespace map using the heap buffer in recovery). The VM
being gone is an uncommon case, so I don't think it makes sense to add
special logic to pass REGBUF_NO_CHANGES. However, I do think we should
have a test for this case.

I added the test to pg_visibility tests because I use
pg_truncate_visibility_map(). That seemed better than adding a new
test module or something. It doesn't test the extension functionality
specifically, but it seems like other tests in pg_visibility.sql also
exercise core code. Let me know if this interpretation is off-base.

- Melanie

[1] https://www.postgresql.org/message-id/CAAKRu_bvkCZ%2BxBzBjujJMkA5STU%2Bb6AtrUUTjcvAH%3DZnnpTtzA%40mail.gmail.com

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

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-12-16T16:39:12Z

    Hi Melanie,

On Wed, Dec 10, 2025 at 11:21 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:

> Hi,
>
> 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);


> Currently, we require the heap buffer to be marked dirty even if it is
> unmodified because we add it to the WAL chain and do not pass
> REGBUF_NO_CHANGES. (And we require adding it to the WAL chain because
> we update the freespace map using the heap buffer in recovery). The VM
> being gone is an uncommon case, so I don't think it makes sense to add
> special logic to pass REGBUF_NO_CHANGES. However, I do think we should
> have a test for this case.
>

makes sense, i think this below comment supports your final decision
of not optimizing it.

* NB: If the heap page is all-visible but the VM bit is not set, we
* don't need to dirty the heap page.  However, if checksums are
* enabled, we do need to make sure that the heap page is dirtied
* before passing it to visibilitymap_set(), because it may be logged.
* Given that this situation should only happen in rare cases after a
* crash, it is not worth optimizing.
*/

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

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

    Melanie Plageman <melanieplageman@gmail.com> — 2025-12-16T17:16:55Z

    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

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

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

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-17T19:29:36Z

    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

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

    Melanie Plageman <melanieplageman@gmail.com> — 2025-12-18T00:35:53Z

    On Wed, Dec 17, 2025 at 2:29 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> 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?

I actually propose we never do that (because the buffer should always
be dirty anyway, and it isn't too expensive to mark an already dirty
buffer dirty again). I've proposed a refactoring of this code, which
includes comments about this expectation in 0001 on this thread [1]
(which you are also participating in).

I proposed the test separately since it seems independently valuable
but 0001 in [1] contains the same test as the one in this thread
actually.

- Melanie

[1] https://www.postgresql.org/message-id/CAAKRu_Yt6EH5aFSJBm-k7PrNM4bTt56fTRbyU7gqYXe4cW%2BF9g%40mail.gmail.com