On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavu...@gmail.com> wrote:
>
> > Thank you for the patch! I could not understand the following change:
> >
> > + /* We know the page should not have been all-visible */
> > + Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
> > + (void) old_vmbits; /* Silence compiler */
> > +
> > + /* Count the newly set VM page for logging */
> > + if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0)
> > {
> > vacrel->vm_new_visible_pages++;
> > if (all_frozen)
> > vacrel->vm_new_visible_frozen_pages++;
> > }
> >
> > The flags is initialized as:
> >
> > uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
> >
> > so the new if-condition is always true.
Yep, I fixed that in the v2 patch I just sent.
> I think we do not need to check visibility of the page here, as we
> already know that page was not all-visible due to LP_DEAD items. We
> can simply increment the vacrel->vm_new_visible_pages and check
> whether the page is frozen.
My idea with the assert was sort of to codify the expectation that the
page couldn't have been all-visible because of the dead items. But
perhaps that is obvious. But you are right that the if statement is
not needed. Perhaps I ought to remove the asserts as they may be more
confusing than helpful.
- Melanie
