Re: Simplify VM counters in vacuum code

Hi,

On Tue, 24 Jun 2025 at 07:13, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>

Thank you for working on this!

> On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman
> <melanieplageman(at)gmail(dot)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