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

On Wed, Apr 5, 2023 at 1:05 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2023-04-04 13:53:15 -0400, Melanie Plageman wrote:
> > > 8. I don't quite follow this comment:
> > >
> > > /*
> > > * TODO: should this be 0 so that we are sure that vacuum() never
> > > * allocates a new bstrategy for us, even if we pass in NULL for that
> > > * parameter? maybe could change how failsafe NULLs out bstrategy if
> > > * so?
> > > */
> > >
> > > Can you explain under what circumstances would vacuum() allocate a
> > > bstrategy when do_autovacuum() would not? Is this a case of a config
> > > reload where someone changes vacuum_buffer_usage_limit from 0 to
> > > something non-zero? If so, perhaps do_autovacuum() needs to detect
> > > this and allocate a strategy rather than having vacuum() do it once
> > > per table (wastefully).
>
> Hm. I don't much like that we use a single strategy for multiple tables
> today. That way even tiny tables never end up in shared_buffers. But that's
> really a discussion for a different thread. However, if were to use a
> per-table bstrategy, it'd be a lot easier to react to changes of the config.
>
>
> I doubt it's worth adding complications to the code for changing the size of
> the ringbuffer during an ongoing vacuum scan, at least for 16. Reacting to
> enabling/disbling the ringbuffer alltogether seems a bit more important, but
> still not crucial compared to making it configurable at all.
>
> I think it'd be OK to add a comment saying something like "XXX: In the future
> we might want to react to configuration changes of the ring buffer size during
> a vacuum" or such.
>
> WRT to the TODO specifically: Yes, passing in 0 seems to make sense. I don't
> see a reason not to do so? But perhaps there's a better solution:
>
> 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().

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.

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?

It simplifies the buffer usage limit patchset and also seems a bit more
clear than what is there now?

- Melanie