Thanks for the review! On Thu, Oct 31, 2024 at 2:13 PM Masahiko Sawada <sawada.m...@gmail.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
v2-0002-Make-visibilitymap_set-return-previous-state-of-v.patch
Description: Binary data
v2-0001-Rename-LVRelState-frozen_pages.patch
Description: Binary data
v2-0003-Count-pages-set-all-visible-and-all-frozen-in-VM-.patch
Description: Binary data
