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

On Wed, Apr 5, 2023 at 6:55 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Wed, Apr 5, 2023 at 5:15 PM Andres Freund <andres@anarazel.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@anarazel.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.

Figured out how to fix the issue, though I'm not sure I understand how
the issue can occur.
use_own_xacts seems like it will always be true for autovacuum when it
calls vacuum() and ExecVacuum() only calls vacuum() once, so I thought
that I could make use_own_xacts a parameter to vacuum() and push up its
calculation for VACUUM and ANALYZE into ExecVacuum().
This caused a deadlock, so there must be a way that in_vacuum is false
but vacuum() is called in a nested context.
Anyway, recalculating it every time in vacuum() fixes it.

Attached is a v12 of the whole vacuum_buffer_usage_limit patch set which
includes a commit to fix the bug in master and a commit to move relevant
code from vacuum() up into ExecVacuum().

The logic I suggested earlier for fixing the bug was...not right.
Attached fix should be right?

- Melanie