Re: Simplify VM counters in vacuum code

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