Re: Count and log pages set all-frozen by vacuum

Thanks for the review!

On Thu, Oct 31, 2024 at 2:13 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
>
> Some small suggestions:
>
> + vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
> + InvalidXLogRecPtr,
> + vmbuffer, InvalidTransactionId,
> + VISIBILITYMAP_ALL_VISIBLE |
> + VISIBILITYMAP_ALL_FROZEN);
>
> How about using "old_vmbits" or something along the same lines to make
> it clear that this value has the bits before visibilitymap_set()?

I've changed it like this.

> ---
> + /* If it wasn't all-frozen before, count it */
> + if (!(vmbits & VISIBILITYMAP_ALL_FROZEN))
>
> To keep consistent with surrounding codes in lazyvacuum.c, I think we
> can write "if ((vmbits & VISIBILITYMAP_ALL_FROZEN) == 0)" instead.

Ah, yes. I considered this. I find the == 0 way more confusing, but I
didn't want to change the other occurrences because maybe other people
like them. But, you're quite right, I should be consistent. I've
changed them to match the current style.

The attached patch set also counts pages set all-visible. I've given
the LVRelState member for it the unfortunate name "vm_page_visibles."
It matches the part of speech of vm_page_freezes. I like that it
conveys that it is the number of pages set all-visible not the number
of pages that are all-visible. Also I didn't want to include the word
"all" since vm_page_freezes doesn't have the word "all". However, it
sounds rather awkward. Suggestions welcome.

- Melanie