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

Thanks for the review, Nitin! Attached v3 addresses your review feedback.

On Sat, Nov 2, 2024 at 7:05 AM Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
> 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.

This is a good idea. I've gone with vm_new_frozen_pages and
vm_new_visible_pages.

I also renamed tuple_freeze_pages to new_frozen_tuple_pages in
response to a review of this patch set on another thread [1] which
said that tuple_freeze_pages didn't make it clear enough that these
were only pages with _newly_ frozen tuples.

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

Thanks. I have fixed this.

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

That wouldn't be incorrect today. However, if we were to add more
visibility map states, that could silently break in the future. Even
with just the two states, I find it more clear to pass them separately
to make it quite clear that we are intentionally setting the page both
all-visible and all-frozen.

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

I see what you are saying. I thought about it quite a bit, and while I
don't think returning the old VM bits is a perfect API, I don't much
like making flags an in-out parameter either. It would make sense if
visibilitymap_set() were overwriting the vmbits and so flags is
new_vmbits as an input parameter and old_vmbits as an output
parameter. But, as it is, flags is the additional flags we want to set
in excess of what is set already (not the final state of the VM bits
we want to achieve). Though the datatypes match, it feels like a
different sort of thing then the state of the vmbits itself.

As such, I've left it as a return value. Alternative API suggestions
are welcome.

- Melanie

[1] https://www.postgresql.org/message-id/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu