Re: Eagerly scan all-visible pages to amortize aggressive vacuum

On Mon, Jan 27, 2025 at 1:22 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Mon, Jan 27, 2025 at 12:52 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > Thank you for updating the patch. I was reviewing the v10 patch and
> > had some comments. I believe these comments are still valid for v11,
> > but please ignore them if outdated.
>
> Thanks so much for the review!
>
> > + if (TransactionIdIsNormal(vacrel->cutoffs.relfrozenxid) &&
> > + TransactionIdPrecedesOrEquals(vacrel->cutoffs.relfrozenxid,
> > +
> > vacrel->cutoffs.FreezeLimit))
> > + oldest_unfrozen_before_cutoff = true;
> > +
> > + if (!oldest_unfrozen_before_cutoff &&
> > + MultiXactIdIsValid(vacrel->cutoffs.relminmxid) &&
> > + MultiXactIdPrecedesOrEquals(vacrel->cutoffs.relminmxid,
> > +
> > vacrel->cutoffs.MultiXactCutoff))
> > + oldest_unfrozen_before_cutoff = true;
> >
> > Given that our freeze check strictly checks if an xid is older than
> > the cutoff (exclusive bound), I think we should check if the
> > relfrozenxid and relminmxid strictly precede the cutoff values.
>
> Makes sense. I've changed that.
>
> > ---
> > if (*was_eager_scanned)
> > vacrel->eager_scanned_pages++;
> >
> > How about incrementing this counter near the place where incrementing
> > scanned_pages (i.e., at the beginning of the loop of
> > heap_vac_scan_next_block())? It would make it easy to understand the
> > difference between eager_scanned_pages and scanned_pages.
>
> Right. That makes sense. I've changed that in the attached v12.
>
> > ---
> > + * No vm_page_frozen output parameter (like what is passed to
> > + * lazy_scan_prune()) is passed here because empty pages are always frozen and
> > + * thus could never be eagerly scanned.
> >
> > The last part "thus could never be eagerly scanned" confused me a bit;
> > IIUC we count all pages that are scanned because of this new eager
> > scan feature as "eagerly scanned pages". We increment
> > eager_scanned_pages counter even if the page is either new or empty.
> > This fact seems to contradict the sentence "empty pages could never be
> > eagerly scanned".
>
> Ah, so what I mean by this is that the first time an empty page is
> vacuumed, it is set all-visible and all-frozen in the VM. We only
> eagerly scan pages that are _only_ all-visible (not also all-frozen).
> So, no empty pages will ever be eligible for eager scanning. I've
> updated the comment, because I agree it was confusing. See what you
> think now.

Thank you for updating the patch! These updates look good to me.

BTW I realized that we need to update tab-complete.c too to support
the tab-completion for vacuum_max_eager_freeze_failure_rate.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com