Re: Eagerly scan all-visible pages to amortize aggressive vacuum - Mailing list pgsql-hackers

On Tue, Dec 17, 2024 at 5:51 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> Thanks for taking a look!
>
> I've rebased and attached an updated v3 which also addresses review feedback.
>
> On Sun, Dec 15, 2024 at 1:05 AM Robert Treat <rob@xzilla.net> wrote:
> > On Fri, Dec 13, 2024 at 5:53 PM Melanie Plageman
> > <melanieplageman@gmail.com> wrote:
<snip>
>
> So, all vacuums are eager after the first eager vacuum (in this
> workload).

I have been thinking about this statement and a sense of awkwardness
that I felt about how this implementation came together which I think
Robert captured well with his statement "- It's a little weird that
you mostly treat eager vacuums as an intermediate position between
aggressive and normal, but then we decide whether we're eager in a
different place than we decide whether
we're aggressive."

It feels to me like eager vacuums are not so much a distinct thing,
but that, like how all vacuum do index cleanup unless told not to, all
vacuums are optimistic that they will convert avp to afp, only we bail
out quickly if the table is below vacuum_freeze_min_age and we throw
the limits out if it is above autovacuum_freeze_max_age, similar to
how we manage vacuum cost limit.

> > > + * The eagerness level of a vacuum determines how many all-visible but
> > > + * not all-frozen pages it eagerly scans.
> > > + *
> > > + * A normal vacuum (eagerness VAC_NORMAL) scans no all-visible pages (with the
> > > + * exception of those scanned due to SKIP_PAGES_THRESHOLD).
> > > + *
> > > + * An eager vacuum (eagerness VAC_EAGER) scans a number of pages up to a limit
> > > + * based on whether or not it is succeeding or failing. An eager vacuum is
> > > + * downgraded to a normal vacuum when it hits its success quota. An aggressive
> > > + * vacuum cannot be downgraded. No eagerness level is ever upgraded.
> > > + *
> >
> > At the risk of being overly nit-picky... eager vacuums scan their
> > subset of all-visible pages "up to a limit" based solely on the
> > success ratio. In the case of (excessive) failures, there is no limit
> > to the number of pages scanned, only a pause in the pages scanned
> > until the next region.
>
> Yes, this is a good point. I've changed it to specifically indicate
> the limit is based on successes. However, I feel like I should either
> not mention the success limit here or mention the regional limiting
> based on failures -- otherwise it is confusing. Can you think of a
> wording that would be good for this comment?
>

I don't feel like I came up with anything concise :-)

I think the gist is something like "a given vacuum will scan a number
of pages up to either a limit based on successes, after which it is
downgraded to a normal vacuum, or a subset of pages based on failures
across different regions within the heap." but even that feels like we
are relying on people already understanding what is going on rather
than defining/explaining it.

> > > + * An aggressive vacuum (eagerness EAGER_FULL) must scan all all-visible but
> > > + * not all-frozen pages.
> > > + */
> >
> > I think the above should be VAC_AGGRESSIVE vs EAGER_FULL, no?
>
> Yep, thanks! Fixed.
>
> > >       vacrel->skipwithvm = skipwithvm;
> > >
> > > +     heap_vacuum_set_up_eagerness(vacrel, aggressive);
> > > +
> > >       if (verbose)
> > > -     {
> > > -             if (vacrel->aggressive)
> > > -                     ereport(INFO,
> > > -                                     (errmsg("aggressively vacuuming \"%s.%s.%s\"",
> > > -                                                     vacrel->dbname, vacrel->relnamespace,
> > > -                                                     vacrel->relname)));
> > > -             else
> > > -                     ereport(INFO,
> > > -                                     (errmsg("vacuuming \"%s.%s.%s\"",
> > > -                                                     vacrel->dbname, vacrel->relnamespace,
> > > -                                                     vacrel->relname)));
> > > -     }
> > > +             ereport(INFO,
> > > +                             (errmsg("%s of \"%s.%s.%s\"",
> > > +                                             vac_eagerness_description(vacrel->eagerness),
> > > +                                             vacrel->dbname, vacrel->relnamespace,
> > > +                                             vacrel->relname)));
> > >
> > >       /*
> > >        * Allocate dead_items memory using dead_items_alloc.  This handles
> >
> > One thing I am wondering about is that since we actually modify
> > vacrel->eagerness during the "success downgrade" cycle, a single
> > vacuum run could potentially produce messages with both eager vacuum
> > and normal vacuum language. I don't think that'd be a problem in the
> > above spot, but wondering if it might be elsewhere (maybe in
> > pg_stat_activity?).
>
> Great point. It's actually already an issue in the vacuum log output.
> In the new patch, I save the eagerness level at the beginning and use
> it in the logging output. Any future users of the eagerness level will
> have to figure out if they care about the original eagerness level or
> the current one.
>

In a general sense, you want to know if your vacuums are converting
avp to afp since it can explain i/o usage. In this version of the
patch, we know it is turned on based on VacEagerness, but it'd be nice
to know if it got turned off; ie. basically if we end up in
if (vacrel->eager_pages.remaining_successes == 0) maybe drop a note in the logs.

In a world where all vacuums are eager vacuums, I think you still want
the latter messaging, but I think the former would end up being noted
based on the outcome of criteria in vacuum_get_cutoffs() (primarily if
we are over the freeze limit).

Robert Treat
https://xzilla.net