Thanks for the review! On Tue, Jun 24, 2025 at 12:12 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman > <melanieplage...@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
From 93d73a4f9d499d58b27d5aed6f4c15fed3b79e45 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@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 <melanieplage...@gmail.com> Reviewed-by: Nazir Bilal Yavuz <byavu...@gmail.com> Reviewed-by: Masahiko Sawada <sawada.m...@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
