Thread: 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

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

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