Re: Simplify VM counters in vacuum code

Hi,

On Tue, 24 Jun 2025 at 18:12, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Tue, Jun 24, 2025 at 9:17 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> >
> > > 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.
>
> Thinking about this more, I think it is better without the asserts.
> I've done this in attached v3.

I liked this version more. I agree that the asserts were causing some confusion.

nitpick:
+            /* Count the newly all-frozen pages for logging. */

AFAIK, we do not use periods in the one line comments. Other than
that, the patch looks good to me.

--
Regards,
Nazir Bilal Yavuz
Microsoft