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

On Tue, Mar 14, 2023 at 08:56:58PM -0400, Melanie Plageman wrote:
> On Sat, Mar 11, 2023 at 2:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

> > > @@ -586,6 +587,45 @@ GetAccessStrategy(BufferAccessStrategyType btype)
> > > +BufferAccessStrategy
> > > +GetAccessStrategyWithSize(BufferAccessStrategyType btype, int ring_size)
> >
> > Maybe it would make sense for GetAccessStrategy() to call
> > GetAccessStrategyWithSize().  Or maybe not.
> 
> You mean instead of having anyone call GetAccessStrategyWithSize()?
> We would need to change the signature of GetAccessStrategy() then -- and
> there are quite a few callers.

I mean to avoid code duplication, GetAccessStrategy() could "Select ring
size to use" and then call into GetAccessStrategyWithSize().  Maybe it's
counter to your intent or otherwise not worth it to save 8 LOC.

> > > +#vacuum_buffer_usage_limit = -1 # size of vacuum buffer access strategy ring.
> > > +                             # -1 to use default,
> > > +                             # 0 to disable vacuum buffer access strategy and use shared buffers
> > > +                             # > 0 to specify size
> >
> > If I'm not wrong, there's still no documentation about "ring buffers" or
> > postgres' "strategy".  Which seems important to do for this patch, along
> > with other documentation.
> 
> Yes, it is. I have been thinking about where in the docs to add it (the
> docs about buffer access strategies). Any ideas?

This patch could add something to the vacuum manpage and to the appendix.
And maybe references from the shared_buffers and pg_buffercache
manpages.

> > This patch should add support in vacuumdb.c.
> 
> Oh, I had totally forgotten about vacuumdb.

:)

> > And maybe a comment about adding support there, since it's annoying
> > when it the release notes one year say "support VACUUM (FOO)" and then
> > one year later say "support vacuumdb --foo".
> 
> I'm not sure I totally follow. Do you mean to add a comment to
> ExecVacuum() saying to add support to vacuumdb when adding a new option
> to vacuum?

Yeah, like:
/* Options added here should also be added to vacuumdb.c */

> In the past, did people forget to add support to vacuumdb for new vacuum
> options or did they forget to document that they did that or did they
> forgot to include that they did that in the release notes?

The first.  Maybe not often, it's not important whether it's in the
original patch, but it's odd if the vacuumdb option isn't added until
the following release, which then shows up as a separate "feature".

-- 
Justin