Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode - Mailing list pgsql-hackers

On Wed, Apr 5, 2023 at 9:15 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Thu, 6 Apr 2023 at 12:42, Melanie Plageman <melanieplageman@gmail.com> wrote:
> > 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().
>
> I'm still playing catch up to the moving of the pre-checks from
> vacuum() to ExecVacuum().  I'm now wondering...
>
> Is it intended that VACUUM t1,t2; now share the same strategy?
> Currently, in master, we'll allocate a new strategy for t2 after
> vacuuming t1. Does this not mean we'll now leave fewer t1 pages in
> shared_buffers because the reuse of the strategy will force them out
> with t2 pages?  I understand there's nothing particularly invalid
> about that, but it is a change in behaviour that the patch seems to be
> making with very little consideration as to if it's better or worse.

I'm pretty sure that in master we also reuse the strategy since we make
it above this loop in vacuum() (and pass it in)

        /*
        * Loop to process each selected relation.
        */
        foreach(cur, relations)
        {
            VacuumRelation *vrel = lfirst_node(VacuumRelation, cur);
            if (params->options & VACOPT_VACUUM)
            {
                if (!vacuum_rel(vrel->oid, vrel->relation, params,
false, bstrategy))
                    continue;
            }

On Wed, Apr 5, 2023 at 8:41 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> 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?

David had already pushed a fix, so the patchset had merge conflicts.
Attached v13 should work.

- Melanie