Re: Simplify VM counters in vacuum code

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