Simplify VM counters in vacuum code
Hi,
In dc6acfd910b8, I added some counters to track and log in
autovacuum/vacuum output the number of pages newly set
all-visible/frozen. Taking another look at the code recently, I
realized the conditions for setting the counters could be simplified
because of what we know to be true about the state of the heap page
and VM at the time we are doing the counting.
Further explanation is in the attached patch. This code is only in 18/master.
- Melanie
Attachments:
v1-Simplify-vacuum-VM-update-logging-counters.patchtext/x-patch; charset=US-ASCII; name=v1-Simplify-vacuum-VM-update-logging-counters.patchDownload
From 6cbbdd359ae4de835bbd77369b598885e8a279b2 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 18 Jun 2025 16:12:15 -0400
Subject: [PATCH v1 02/14] Simplify vacuum VM update logging counters
We can simplify the VM counters added in dc6acfd910b8 to
lazy_vacuum_heap_page() and lazy_scan_new_or_empty().
We won't invoke lazy_vacuum_heap_page() unless there are dead line
pointers, so we know the page can't be all-visible.
In lazy_scan_new_or_empty(), we only update the VM if the page-level
hint PD_ALL_VISIBLE is clear, and the VM bit cannot be set if the page
level bit is clear because a subsequent page update would fail to clear
the visibility map bit.
Simplify the logic for determining which log counters to increment based
on this knowledge.
---
src/backend/access/heap/vacuumlazy.c | 32 +++++++++++-----------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 09416450af9..c8da2f835c4 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1900,17 +1900,12 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
VISIBILITYMAP_ALL_FROZEN);
END_CRIT_SECTION();
- /*
- * If the page wasn't already set all-visible and/or all-frozen in
- * the VM, count it as newly set for logging.
- */
- if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
- {
- vacrel->vm_new_visible_pages++;
- vacrel->vm_new_visible_frozen_pages++;
- }
- else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)
- vacrel->vm_new_frozen_pages++;
+ /* VM bits cannot have been set if PD_ALL_VISIBLE was clear */
+ Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
+ (void) old_vmbits; /* Silence compiler */
+ /* Count the newly all-frozen pages for logging. */
+ vacrel->vm_new_visible_pages++;
+ vacrel->vm_new_visible_frozen_pages++;
}
freespace = PageGetHeapFreeSpace(page);
@@ -2930,20 +2925,17 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
vmbuffer, visibility_cutoff_xid,
flags);
- /*
- * If the page wasn't already set all-visible and/or all-frozen in the
- * VM, count it as newly set for logging.
- */
- if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
+ /* We know the page should not have been all-visible */
+ Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
+ (void) old_vmbits; /* Silence compiler */
+
+ /* Count the newly set VM page for logging */
+ if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0)
{
vacrel->vm_new_visible_pages++;
if (all_frozen)
vacrel->vm_new_visible_frozen_pages++;
}
-
- else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
- all_frozen)
- vacrel->vm_new_frozen_pages++;
}
/* Revert to the previous phase information for error traceback */
--
2.34.1
On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
Hi,
In dc6acfd910b8, I added some counters to track and log in
autovacuum/vacuum output the number of pages newly set
all-visible/frozen. Taking another look at the code recently, I
realized the conditions for setting the counters could be simplified
because of what we know to be true about the state of the heap page
and VM at the time we are doing the counting.
Thank you for the patch! I could not understand the following change:
+ /* We know the page should not have been all-visible */
+ Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
+ (void) old_vmbits; /* Silence compiler */
+
+ /* Count the newly set VM page for logging */
+ if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0)
{
vacrel->vm_new_visible_pages++;
if (all_frozen)
vacrel->vm_new_visible_frozen_pages++;
}The flags is initialized as:
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
so the new if-condition is always true.
Further explanation is in the attached patch. This code is only in 18/master.
The patch removes if statements and adds some assertions, which seems
to be a refactoring to me rather than a fix. I think we need to
consider whether it's really okay to apply it to v18.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Hi,
On Tue, 24 Jun 2025 at 07:13, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Thank you for working on this!
On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:Hi,
In dc6acfd910b8, I added some counters to track and log in
autovacuum/vacuum output the number of pages newly set
all-visible/frozen. Taking another look at the code recently, I
realized the conditions for setting the counters could be simplified
because of what we know to be true about the state of the heap page
and VM at the time we are doing the counting.Thank you for the patch! I could not understand the following change:
+ /* We know the page should not have been all-visible */ + Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0); + (void) old_vmbits; /* Silence compiler */ + + /* Count the newly set VM page for logging */ + if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0) { vacrel->vm_new_visible_pages++; if (all_frozen) vacrel->vm_new_visible_frozen_pages++; }The flags is initialized as:
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
so the new if-condition is always true.
I think we do not need to check visibility of the page here, as we
already know that page was not all-visible due to LP_DEAD items. We
can simply increment the vacrel->vm_new_visible_pages and check
whether the page is frozen.
--
Regards,
Nazir Bilal Yavuz
Microsoft
Thanks for the review!
On Tue, Jun 24, 2025 at 12:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:The flags is initialized as:
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
so the new if-condition is always true.
Yep, this was a mistake. I pulled this patch out of a larger set in
which I moved setting these counters outside of the
heap_page_is_all_visible() == true branch. Attached v2 fixes this.
The patch removes if statements and adds some assertions, which seems
to be a refactoring to me rather than a fix. I think we need to
consider whether it's really okay to apply it to v18.
The reason I consider it a fix is that the if statement is confusing
-- it makes the reader think it is possible that the VM page was
already all-visible/frozen. In the other cases where we set the VM
counters, that is true. But in the case of lazy_vacuum_heap_page(), it
would not be correct for the page to have been all-visible because it
contained LP_DEAD items. And in the case of an empty page where
PD_ALL_VISIBLE was clear, the VM bits cannot have been set (because
the page bit must be set if the VM bit is set).
We could remove the asserts, as we rely on other code to enforce these
invariants. So, here the asserts would only really be protecting from
code changes that make it so the counters are no longer correctly
counting newly all-visible pages -- which isn't critical to get right.
- Melanie
Attachments:
v2-0001-Simplify-vacuum-VM-update-logging-counters.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Simplify-vacuum-VM-update-logging-counters.patchDownload
From 93d73a4f9d499d58b27d5aed6f4c15fed3b79e45 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 18 Jun 2025 16:12:15 -0400
Subject: [PATCH v2] Simplify vacuum VM update logging counters
We can simplify the VM counters added in dc6acfd910b8 to
lazy_vacuum_heap_page() and lazy_scan_new_or_empty().
We won't invoke lazy_vacuum_heap_page() unless there are dead line
pointers, so we know the page can't be all-visible.
In lazy_scan_new_or_empty(), we only update the VM if the page-level
hint PD_ALL_VISIBLE is clear, and the VM bit cannot be set if the page
level bit is clear because a subsequent page update would fail to clear
the visibility map bit.
Simplify the logic for determining which log counters to increment based
on this knowledge.
Author: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/flat/CAAKRu_a9w_n2mwY%3DG4LjfWTvRTJtjbfvnYAKi4WjO8QXHHrA0g%40mail.gmail.com
---
src/backend/access/heap/vacuumlazy.c | 37 ++++++++++------------------
1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 09416450af9..1a8326b7750 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1900,17 +1900,12 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
VISIBILITYMAP_ALL_FROZEN);
END_CRIT_SECTION();
- /*
- * If the page wasn't already set all-visible and/or all-frozen in
- * the VM, count it as newly set for logging.
- */
- if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
- {
- vacrel->vm_new_visible_pages++;
- vacrel->vm_new_visible_frozen_pages++;
- }
- else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)
- vacrel->vm_new_frozen_pages++;
+ /* VM bits cannot have been set if PD_ALL_VISIBLE was clear */
+ Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
+ (void) old_vmbits; /* Silence compiler */
+ /* Count the newly all-frozen pages for logging. */
+ vacrel->vm_new_visible_pages++;
+ vacrel->vm_new_visible_frozen_pages++;
}
freespace = PageGetHeapFreeSpace(page);
@@ -2930,20 +2925,14 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
vmbuffer, visibility_cutoff_xid,
flags);
- /*
- * If the page wasn't already set all-visible and/or all-frozen in the
- * VM, count it as newly set for logging.
- */
- if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
- {
- vacrel->vm_new_visible_pages++;
- if (all_frozen)
- vacrel->vm_new_visible_frozen_pages++;
- }
+ /* We know the page should not have been all-visible */
+ Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
+ (void) old_vmbits; /* Silence compiler */
- else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
- all_frozen)
- vacrel->vm_new_frozen_pages++;
+ /* Count the newly set VM page for logging */
+ vacrel->vm_new_visible_pages++;
+ if (all_frozen)
+ vacrel->vm_new_visible_frozen_pages++;
}
/* Revert to the previous phase information for error traceback */
--
2.34.1
On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
Thank you for the patch! I could not understand the following change:
+ /* We know the page should not have been all-visible */ + Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0); + (void) old_vmbits; /* Silence compiler */ + + /* Count the newly set VM page for logging */ + if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0) { vacrel->vm_new_visible_pages++; if (all_frozen) vacrel->vm_new_visible_frozen_pages++; }The flags is initialized as:
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
so the new if-condition is always true.
Yep, I fixed that in the v2 patch I just sent.
I think we do not need to check visibility of the page here, as we
already know that page was not all-visible due to LP_DEAD items. We
can simply increment the vacrel->vm_new_visible_pages and check
whether the page is frozen.
My idea with the assert was sort of to codify the expectation that the
page couldn't have been all-visible because of the dead items. But
perhaps that is obvious. But you are right that the if statement is
not needed. Perhaps I ought to remove the asserts as they may be more
confusing than helpful.
- Melanie
On Tue, Jun 24, 2025 at 9:17 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
I think we do not need to check visibility of the page here, as we
already know that page was not all-visible due to LP_DEAD items. We
can simply increment the vacrel->vm_new_visible_pages and check
whether the page is frozen.My idea with the assert was sort of to codify the expectation that the
page couldn't have been all-visible because of the dead items. But
perhaps that is obvious. But you are right that the if statement is
not needed. Perhaps I ought to remove the asserts as they may be more
confusing than helpful.
Thinking about this more, I think it is better without the asserts.
I've done this in attached v3.
- Melanie
Attachments:
v3-0001-Simplify-vacuum-VM-update-logging-counters.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Simplify-vacuum-VM-update-logging-counters.patchDownload
From d4386febdb8eaae9c49d78d2d9815de6f653d1c6 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 18 Jun 2025 16:12:15 -0400
Subject: [PATCH v3] Simplify vacuum VM update logging counters
We can simplify the VM counters added in dc6acfd910b8 to
lazy_vacuum_heap_page() and lazy_scan_new_or_empty().
We won't invoke lazy_vacuum_heap_page() unless there are dead line
pointers, so we know the page can't be all-visible.
In lazy_scan_new_or_empty(), we only update the VM if the page-level
hint PD_ALL_VISIBLE is clear, and the VM bit cannot be set if the page
level bit is clear because a subsequent page update would fail to clear
the visibility map bit.
Simplify the logic for determining which log counters to increment based
on this knowledge.
Author: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/flat/CAAKRu_a9w_n2mwY%3DG4LjfWTvRTJtjbfvnYAKi4WjO8QXHHrA0g%40mail.gmail.com
---
src/backend/access/heap/vacuumlazy.c | 53 +++++++++-------------------
1 file changed, 16 insertions(+), 37 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 09416450af9..9912ec52030 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1872,8 +1872,6 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
*/
if (!PageIsAllVisible(page))
{
- uint8 old_vmbits;
-
START_CRIT_SECTION();
/* mark buffer dirty before writing a WAL record */
@@ -1893,24 +1891,16 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
log_newpage_buffer(buf, true);
PageSetAllVisible(page);
- old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
- InvalidXLogRecPtr,
- vmbuffer, InvalidTransactionId,
- VISIBILITYMAP_ALL_VISIBLE |
- VISIBILITYMAP_ALL_FROZEN);
+ visibilitymap_set(vacrel->rel, blkno, buf,
+ InvalidXLogRecPtr,
+ vmbuffer, InvalidTransactionId,
+ VISIBILITYMAP_ALL_VISIBLE |
+ VISIBILITYMAP_ALL_FROZEN);
END_CRIT_SECTION();
- /*
- * If the page wasn't already set all-visible and/or all-frozen in
- * the VM, count it as newly set for logging.
- */
- if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
- {
- vacrel->vm_new_visible_pages++;
- vacrel->vm_new_visible_frozen_pages++;
- }
- else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)
- vacrel->vm_new_frozen_pages++;
+ /* Count the newly all-frozen pages for logging. */
+ vacrel->vm_new_visible_pages++;
+ vacrel->vm_new_visible_frozen_pages++;
}
freespace = PageGetHeapFreeSpace(page);
@@ -2915,7 +2905,6 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
if (heap_page_is_all_visible(vacrel, buffer, &visibility_cutoff_xid,
&all_frozen))
{
- uint8 old_vmbits;
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
if (all_frozen)
@@ -2925,25 +2914,15 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
}
PageSetAllVisible(page);
- old_vmbits = visibilitymap_set(vacrel->rel, blkno, buffer,
- InvalidXLogRecPtr,
- vmbuffer, visibility_cutoff_xid,
- flags);
+ visibilitymap_set(vacrel->rel, blkno, buffer,
+ InvalidXLogRecPtr,
+ vmbuffer, visibility_cutoff_xid,
+ flags);
- /*
- * If the page wasn't already set all-visible and/or all-frozen in the
- * VM, count it as newly set for logging.
- */
- if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0)
- {
- vacrel->vm_new_visible_pages++;
- if (all_frozen)
- vacrel->vm_new_visible_frozen_pages++;
- }
-
- else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 &&
- all_frozen)
- vacrel->vm_new_frozen_pages++;
+ /* Count the newly set VM page for logging */
+ vacrel->vm_new_visible_pages++;
+ if (all_frozen)
+ vacrel->vm_new_visible_frozen_pages++;
}
/* Revert to the previous phase information for error traceback */
--
2.34.1
Hi,
On Tue, 24 Jun 2025 at 18:12, Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Jun 24, 2025 at 9:17 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
I think we do not need to check visibility of the page here, as we
already know that page was not all-visible due to LP_DEAD items. We
can simply increment the vacrel->vm_new_visible_pages and check
whether the page is frozen.My idea with the assert was sort of to codify the expectation that the
page couldn't have been all-visible because of the dead items. But
perhaps that is obvious. But you are right that the if statement is
not needed. Perhaps I ought to remove the asserts as they may be more
confusing than helpful.Thinking about this more, I think it is better without the asserts.
I've done this in attached v3.
I liked this version more. I agree that the asserts were causing some confusion.
nitpick:
+ /* Count the newly all-frozen pages for logging. */
AFAIK, we do not use periods in the one line comments. Other than
that, the patch looks good to me.
--
Regards,
Nazir Bilal Yavuz
Microsoft
On Wed, Jun 25, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
I liked this version more. I agree that the asserts were causing some confusion.
Thanks for the review!
Sawada-san, do you have any objection to this being merged in
master/18? I know you had said changing the if statements to asserts
did not feel like a bug fix. However, now that the asserts have been
removed, do you still feel this way?
I feel simplifying the counters is a clarity improvement and would
prefer we have it before branching, but I would hold off if you still
felt this way even with the asserts removed.
(I had originally misunderstood and didn't realize that the page bit
must be set if the VM bit is set, so that is why I had a guard in the
empty page case. And for lazy_vacuum_heap_page(), I was being overly
cautious at the expense of clarity.)
nitpick:
+ /* Count the newly all-frozen pages for logging. */AFAIK, we do not use periods in the one line comments. Other than
that, the patch looks good to me.
Will fix. Thanks
- Melanie
On Thu, Jun 26, 2025 at 10:01 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Wed, Jun 25, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
I liked this version more. I agree that the asserts were causing some confusion.
Thanks for the review!
Sawada-san, do you have any objection to this being merged in
master/18? I know you had said changing the if statements to asserts
did not feel like a bug fix. However, now that the asserts have been
removed, do you still feel this way?I feel simplifying the counters is a clarity improvement and would
prefer we have it before branching, but I would hold off if you still
felt this way even with the asserts removed.(I had originally misunderstood and didn't realize that the page bit
must be set if the VM bit is set, so that is why I had a guard in the
empty page case. And for lazy_vacuum_heap_page(), I was being overly
cautious at the expense of clarity.)
Thank you for updating the patch! The changes in v3 appear
straightforward; the patch eliminates unnecessary codes that were
introduced in the original commit due to some misunderstandings. And I
agreed with your answer[1]/messages/by-id/CAAKRu_ZFGLXCWkLZv2BT4SLbu=3zHPkfx5EeR+Rupe=xCxLaPA@mail.gmail.com to my question. So it looks okay to me to
push it to master/18.
Regards,
[1]: /messages/by-id/CAAKRu_ZFGLXCWkLZv2BT4SLbu=3zHPkfx5EeR+Rupe=xCxLaPA@mail.gmail.com
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Thu, Jun 26, 2025 at 10:25 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Thank you for updating the patch! The changes in v3 appear
straightforward; the patch eliminates unnecessary codes that were
introduced in the original commit due to some misunderstandings. And I
agreed with your answer[1] to my question. So it looks okay to me to
push it to master/18.
Thanks so much for the reply. I've now pushed this.
- Melanie