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

1.
> + BlockNumber vm_page_freezes; /* # pages newly set all-frozen in VM */
> + BlockNumber vm_page_visibles; /* # pages newly set all-visible in the VM */

I believe renaming the variables to ‘vm_freeze_pages’ and
‘vm_visible_pages’ would enhance readability and ensure consistency
with the surrounding variables in the structure.

2.
> /* # pages newly set all-frozen in VM */
> /* # pages newly set all-visible in the VM */

The comment ‘# pages newly set all-frozen in VM’ is missing the word ‘the’.

3.
> + old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
> + InvalidXLogRecPtr,
> + vmbuffer, InvalidTransactionId,
> + VISIBILITYMAP_ALL_VISIBLE |
> + VISIBILITYMAP_ALL_FROZEN);

Could we pass ‘VISIBILITYMAP_VALID_BITS’ instead of
‘VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN’? I understand
this might be beyond the scope of this patch, but I noticed it during
reviewing and I’m curious.

4.
> - * any I/O.
> + * any I/O. Returns the visibility map status before setting the bits.
> */
> -void
> +uint8
> visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
> XLogRecPtr recptr, Buffer vmBuf, TransactionId cutoff_xid,
> uint8 flags)

Instead of returning old_vmbits can we reuse the flags parameter to
return old_vmbits, making it an in-out parameter.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft

On Fri, Nov 1, 2024 at 4:00 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> 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