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

Thanks for the review!

Attached is an updated v6.

v6 has some updates and corrections. It has two remaining TODOs in the
code: one is around what value to initialize the ring_size to in
VacuumParams, the other is around whether or not parallel vacuum index
workers should in fact stick with the default buffer access strategy
sizes.

I also separated vacuumdb into its own commit.

I also have addressed Justin's review feedback.

On Sat, Mar 18, 2023 at 2:30 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> > Subject: [PATCH v4 3/3] add vacuum[db] option to specify ring_size and guc
>
> > +        Specifies the ring buffer size to be used for a given invocation of
> > +        <command>VACUUM</command> or instance of autovacuum. This size is
> > +        converted to a number of shared buffers which will be reused as part of
>
> I'd say "specifies the size of shared_buffers to be reused as .."

I've included "shared_buffers" in the description.

> > +        a <literal>Buffer Access Strategy</literal>. <literal>0</literal> will
> > +        disable use of a <literal>Buffer Access Strategy</literal>.
> > +        <literal>-1</literal> will set the size to a default of <literal>256
> > +        kB</literal>. The maximum ring buffer size is <literal>16 GB</literal>.
> > +        Though you may set <varname>vacuum_buffer_usage_limit</varname> below
> > +        <literal>128 kB</literal>, it will be clamped to <literal>128
> > +        kB</literal> at runtime. The default value is <literal>-1</literal>.
> > +        This parameter can be set at any time.
>
> GUC docs usually also say something like
> "If this value is specified without units, it is taken as .."

I had updated this in v5 with slightly different wording, but I now am
using the wording you suggested (which does appear standard in the rest
of the docs).

>
> > +      is used to calculate a number of shared buffers which will be reused as
>
> *the* number?

updated.

>
> > +      <command>VACUUM</command>. The analyze stage and parallel vacuum workers
> > +      do not use this size.
>
> I think what you mean is that vacuum's heap scan stage uses the
> strategy, but the index scan/cleanup phases doesn't?

Yes, non-parallel index vacuum and cleanup will use whatever value you
specify but parallel workers make their own buffer access strategy
object. I've updated the docs to indicate that they will use the default
size for this.


>
> > +        The size in kB of the ring buffer used for vacuuming. This size is used
> > +        to calculate a number of shared buffers which will be reused as part of
>
> *the* number

fixed.

> > +++ b/doc/src/sgml/ref/vacuumdb.sgml
>
> The docs here duplicate the sql-vacuum docs.  It seems better to refer
> to the vacuum page for details, like --parallel does.

Good idea.

>
> Unrelated: it would be nice if the client-side options were documented
> separately from the server-side options.  Especially due to --jobs and
> --parallel.

Yes, that would be helpful.

> > +                     if (!parse_int(vac_buffer_size, &result, GUC_UNIT_KB, NULL))
> > +                     {
> > +                             ereport(ERROR,
> > +                                             (errcode(ERRCODE_SYNTAX_ERROR),
> > +                                                     errmsg("buffer_usage_limit for a vacuum must be between -1
and%d. %s is invalid.", 
> > +                                                                     MAX_BAS_RING_SIZE_KB, vac_buffer_size),
> > +                                                     parser_errposition(pstate, opt->location)));
> > +                     }
> > +
> > +                     /* check for out-of-bounds */
> > +                     if (result < -1 || result > MAX_BAS_RING_SIZE_KB)
> > +                     {
> > +                             ereport(ERROR,
> > +                                             (errcode(ERRCODE_SYNTAX_ERROR),
> > +                                                     errmsg("buffer_usage_limit for a vacuum must be between -1
and%d", 
> > +                                                                     MAX_BAS_RING_SIZE_KB),
> > +                                                     parser_errposition(pstate, opt->location)));
> > +                     }
>
> I think these checks could be collapsed into a single ereport().
>
> if !parse_int() || (result < -1 || result > MAX_BAS_RINGSIZE_KB):
>         ereport(ERROR,
>                 errcode(ERRCODE_SYNTAX_ERROR),
>                 errmsg("buffer_usage_limit for a vacuum must be an integer between -1 and %d",
>                         MAX_BAS_RING_SIZE_KB),
>
> There was a recent, similar, and unrelated suggestion here:
> https://www.postgresql.org/message-id/20230314.135859.260879647537075548.horikyota.ntt%40gmail.com

So, these have been updated/improved in v5. I still didn't combine them.
I see what you are saying about combining them (and I checked the link
you shared), but in this case, having them separate allows me to provide
info using the hintmsg passed to parse_int() about why it failed during
parse_int -- which could be something not related to range. So, I think
it makes sense to keep them separate.

> > +#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
>
> I think it's confusing to say "and use shared buffers", since
> "strategies" also use shared_buffers.  It seems better to remove those 4
> words.

Got it. I've gone ahead and done that.

> > @@ -550,6 +563,13 @@ vacuum_one_database(ConnParams *cparams,
> >               pg_fatal("cannot use the \"%s\" option on server versions older than PostgreSQL %s",
> >                                "--parallel", "13");
> >
> > +     // TODO: this is a problem: if the user specifies this option with -1 in a
> > +     // version before 16, it will not produce an error message. it also won't
> > +     // do anything, but that still doesn't seem right.
>
> Actually, that seems fine to me.  If someone installs v16 vacuumdb, they
> can run it against old servers and specify the option as -1 without it
> failing with an error.  I don't know if anyone will find that useful,
> but it doesn't seem unreasonable.

I sort of skirted around this by removing any validation from vacuumdb
(present in v5 and still the case in v6). Now, the parameter is a string
and I check if it is non-NULL when the version is < 16. However, this
will no longer have the property that someone can use v16 vacuumdb and
pass buffer-usage-limit and have it not fail. I think that is okay,
though, since they might be confused thinking it was doing something.

> I still think adding something to the glossary would be good.
>
> Buffer Access Strategy:
> A circular/ring buffer used for reading or writing data pages from/to
> the operating system.  Ring buffers are used for sequential scans of
> large tables, VACUUM, COPY FROM, CREATE TABLE AS SELECT, ALTER TABLE,
> and CLUSTER.  By using only a limited portion of >shared_buffers<, the
> ring buffer avoids avoids evicting large amounts of data whenever a
> backend performs bulk I/O operations.  Use of a ring buffer also forces
> the backend to write out its own dirty pages, rather than leaving them
> behind to be cleaned up by other backends.

Yes, I have taken some ideas from here and added a separate commit
before all the others adding Buffer Access Strategy to the
documentation.

> If there's a larger section added than a glossary entry, the text could
> be promoted from src/backend/storage/buffer/README to doc/.

This is a good idea. I think we provided enough information in the
glossary (as far as users would care) if it weren't for the new
buffer_usage_limit guc, which probably merits more explanation about how
it interacts with buffer access strategies. Since it is only used for
vacuum now, do you think such a thing would belong in VACUUM-related
documentation? Like somewhere in [1]?

On Wed, Mar 15, 2023 at 9:03 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> On Wed, Mar 15, 2023 at 8:14 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > 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.
>
> Oh, that's a cool idea. I will think on it.

So, I thought about doing a version of this by adding a helper which did
the allocation of the BufferAccessStrategy object given a number of
buffers that could be called by both GetAccessStrategy() and
GetAccessStrategyWithSize(). I decided not to because I wanted to emit a
debug message if the size of the ring was clamped lower or higher than
the user would expect -- but only do this in GetAccessStrategyWithSize()
since there is no user expectation in the use of GetAccessStrategy().

- Melanie

[1] https://www.postgresql.org/docs/devel/routine-vacuuming.html