Adding vacuum test case of setting the VM when heap page is unmodified
Hi,
While working on a patch to set the VM in the same WAL record as
pruning and freezing [1]/messages/by-id/CAAKRu_bvkCZ+xBzBjujJMkA5STU+b6AtrUUTjcvAH=ZnnpTtzA@mail.gmail.com, 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]: /messages/by-id/CAAKRu_bvkCZ+xBzBjujJMkA5STU+b6AtrUUTjcvAH=ZnnpTtzA@mail.gmail.com
Attachments:
v1-0001-Test-vacuum-setting-VM-for-unmodified-heap-buffer.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Test-vacuum-setting-VM-for-unmodified-heap-buffer.patchDownload
From fa16946d3ab986542436efa57248abaf11f77fc0 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 10 Dec 2025 12:34:58 -0500
Subject: [PATCH v1] Test vacuum setting VM for unmodified heap buffer
When setting the VM during phase I vacuum, it is possible that the heap
buffer requires no modification. This can happen if the VM was truncated
or destroyed. Because we still add the heap buffer to the WAL chain (we
use it to update the freespace map in recovery), currently, we require
that the heap buffer be marked dirty anyway. We could pass
REGBUF_NO_CHANGES when registering the buffer, but that adds fiddly
extra logic for an uncommon case.
Either way, we had no test coverage for this, so it's best to add it.
---
.../pg_visibility/expected/pg_visibility.out | 17 +++++++++++++++++
contrib/pg_visibility/sql/pg_visibility.sql | 13 +++++++++++++
2 files changed, 30 insertions(+)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index 09fa5933a35..3608f801eee 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -204,6 +204,23 @@ select pg_truncate_visibility_map('test_partition');
(1 row)
+-- test the case where vacuum phase I does not need to modify the heap buffer
+-- and only needs to set the VM
+create table test_vac_unmodified_heap(a int);
+insert into test_vac_unmodified_heap values (1);
+vacuum (freeze) test_vac_unmodified_heap;
+-- the checkpoint cleans the buffer dirtied by freezing the sole tuple
+checkpoint;
+-- truncating the VM ensures that the next vacuum will need to set it
+select pg_truncate_visibility_map('test_vac_unmodified_heap');
+ pg_truncate_visibility_map
+----------------------------
+
+(1 row)
+
+-- vacuum sets the VM but does not need to set PD_ALL_VISIBLE so no heap page
+-- modification
+vacuum test_vac_unmodified_heap;
-- test copy freeze
create table copyfreeze (a int, b char(1500));
-- load all rows via COPY FREEZE and ensure that all pages are set all-visible
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
index 5af06ec5b76..6af7c179df0 100644
--- a/contrib/pg_visibility/sql/pg_visibility.sql
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -94,6 +94,19 @@ select count(*) > 0 from pg_visibility_map_summary('test_partition');
select * from pg_check_frozen('test_partition'); -- hopefully none
select pg_truncate_visibility_map('test_partition');
+-- test the case where vacuum phase I does not need to modify the heap buffer
+-- and only needs to set the VM
+create table test_vac_unmodified_heap(a int);
+insert into test_vac_unmodified_heap values (1);
+vacuum (freeze) test_vac_unmodified_heap;
+-- the checkpoint cleans the buffer dirtied by freezing the sole tuple
+checkpoint;
+-- truncating the VM ensures that the next vacuum will need to set it
+select pg_truncate_visibility_map('test_vac_unmodified_heap');
+-- vacuum sets the VM but does not need to set PD_ALL_VISIBLE so no heap page
+-- modification
+vacuum test_vac_unmodified_heap;
+
-- test copy freeze
create table copyfreeze (a int, b char(1500));
--
2.43.0
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/
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]/messages/by-id/CAAKRu_ayWLg=WDGZZfSPWf0KjPM8u=LBb0D6XaEWyx2_YFFwAQ@mail.gmail.com.
- Melanie
[1]: /messages/by-id/CAAKRu_ayWLg=WDGZZfSPWf0KjPM8u=LBb0D6XaEWyx2_YFFwAQ@mail.gmail.com
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 trueas 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/
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() caseif ((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] /messages/by-id/CAAKRu_ayWLg=WDGZZfSPWf0KjPM8u=LBb0D6XaEWyx2_YFFwAQ@mail.gmail.com
Hi!
I did run a test and this indeed triggers assertion if somebody writes
something like [0]https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html#2209. Code at [0]https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html#2209 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
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]/messages/by-id/CAAKRu_Yt6EH5aFSJBm-k7PrNM4bTt56fTRbyU7gqYXe4cW+F9g@mail.gmail.com
(which you are also participating in).
I proposed the test separately since it seems independently valuable
but 0001 in [1]/messages/by-id/CAAKRu_Yt6EH5aFSJBm-k7PrNM4bTt56fTRbyU7gqYXe4cW+F9g@mail.gmail.com contains the same test as the one in this thread
actually.
- Melanie
[1]: /messages/by-id/CAAKRu_Yt6EH5aFSJBm-k7PrNM4bTt56fTRbyU7gqYXe4cW+F9g@mail.gmail.com