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

On Mon, Jan 27, 2025 at 12:52 PM Masahiko Sawada <sawada.mshk@gmail.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.

- Melanie