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

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