Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode

On Wed, Apr 5, 2023 at 5:15 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2023-04-05 16:17:20 -0400, Melanie Plageman wrote:
> > On Wed, Apr 5, 2023 at 1:05 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Perhaps the best solution for autovac vs interactive vacuum issue would be to
> > > move the allocation of the bstrategy to ExecVacuum()?
> >
> > So, I started looking into allocating the bstrategy in ExecVacuum().
> >
> > While doing so, I was trying to understand if the "sanity checking" in
> > vacuum() could possibly apply to autovacuum, and I don't really see how.
> >
> > AFAICT, autovacuum does not ever set VACOPT_DISABLE_PAGE_SKIPPING or
> > VACOPT_FULL or VACOPT_ONLY_DATABASE_STATS.
> >
> > We could move those sanity checks up into ExecVacuum().
>
> Would make sense.
>
> ISTM that eventually most of what currently happens in vacuum() should be in
> ExecVacuum(). There's a lot of stuff that can't happen for autovacuum. So it
> just seems to make more sense to move those parts to ExecVacuum().

I've done that in the attached wip patch. It is perhaps too much of a
change, I dunno.

> > I also noticed that we make the vac_context in vacuum() which says it is
> > for "cross-transaction storage". We use it for the buffer access
> > strategy and for the newrels relation list created in vacuum(). Then we
> > delete it at the end of vacuum().
>
> > Autovacuum workers already make a similar kind of memory context called
> > AutovacMemCxt in do_autovacuum() which the comment says is for the list
> > of relations to vacuum/analyze across transactions.
>
> AutovacMemCxt seems to be a bit longer lived / cover more than the context
> created in vacuum(). It's where all the hash tables etc live that
> do_autovacuum() uses to determine what to vacuum.
>
> Note that do_autovacuum() also creates:
>
> /*
> * create a memory context to act as fake PortalContext, so that the
> * contexts created in the vacuum code are cleaned up for each table.
> */
> PortalContext = AllocSetContextCreate(AutovacMemCxt,
> "Autovacuum Portal",
> ALLOCSET_DEFAULT_SIZES);
>
> which is then what vacuum() creates the "Vacuum" context in.

Yea, I realized that when writing the patch.

> > What if we made ExecVacuum() make its own memory context and both it and
> > do_autovacuum() pass that memory context (along with the buffer access
> > strategy they make) to vacuum(), which then uses the memory context in
> > the same way it does now?
>
> Maybe? It's not clear to me why it'd be a win.

Less that it is a win and more that we need access to that memory
context when allocating the buffer access strategy, so we would have had
to make it in ExecVacuum(). And if we have already made it, we would
need to pass it in to vacuum() for it to use.

> > It simplifies the buffer usage limit patchset and also seems a bit more
> > clear than what is there now?
>
> I don't really see what it'd make simpler? The context in vacuum() is used for
> just that vacuum - we couldn't just use AutovacMemCxt, as that'd live much
> longer (for all the tables a autovac worker processes).

Autovacuum already made the BufferAccessStrategy in the AutovacMemCxt,
so this is the same behavior. I simply made autovacuum_do_vac_analyze()
make the per table vacuum memory context and pass that to vacuum(). So
we have the same amount of memory context granularity as before.

Attached patchset has some kind of isolation test failure due to a hard
deadlock that I haven't figured out yet. I thought it was something with
the "in_vacuum" static variable and having VACUUM or ANALYZE called when
already in VACUUM or ANALYZE, but that variable is the same as in
master.

I've mostly shared it because I want to know if this approach is worth
pursuing or not.

Also, while working on it, I noticed that I made a mistake in the code
that was committed in 4830f102 and didn't remember that we should still
make a Buffer Access Strategy in the case of VACUUM (FULL, ANALYZE).

Changing this:

if (params->options & (VACOPT_ONLY_DATABASE_STATS | VACOPT_FULL)) == 0)

to this:

if ((params.options & VACOPT_ONLY_DATABASE_STATS) == 0 ||
(params.options & VACOPT_FULL && (params.options & VACOPT_ANALYZE) == 0)

should fix it.

- Melanie