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

On Fri, 7 Apr 2023 at 05:20, Melanie Plageman <melanieplageman@gmail.com> wrote:
> > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml

> This still says that the default value is -1.

Oops, I had this staged but didn't commit and formed the patch with
"git diff master.."  instead of "git diff master", so missed a few
staged changes.

> > diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
> I noticed you seemed to have removed the reference to the GUC
> vacuum_buffer_usage_limit here. Was that intentional?
> We may not need to mention "falling back" as I did before, however, the
> GUC docs mention max/min values and such, which might be useful.

Unintentional. I removed it when removing the -1 stuff. It's useful to
keep something about the fallback, so I put that part back.

> > +     /* Allow specifying the default or disabling Buffer Access Strategy */
> > +     if (*newval == -1 || *newval == 0)
> > +             return true;
>
> This should not check for -1 since that isn't the default anymore.
> It should only need to check for 0 I think?

Thanks. That one was one of the staged fixes.

> > +     /* Value upper and lower hard limits are inclusive */
> > +     if (*newval >= MIN_BAS_VAC_RING_SIZE_KB &&
> > +             *newval <= MAX_BAS_VAC_RING_SIZE_KB)
> > +             return true;
> > +
> > +     /* Value does not fall within any allowable range */
> > +     GUC_check_errdetail("\"vacuum_buffer_usage_limit\" must be -1, 0 or between %d KB and %d KB",
> > +                                             MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB);
>
> Also remove -1 here.

And this one.

> >   * Primary entry point for manual VACUUM and ANALYZE commands
> >   *
> > @@ -114,6 +139,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
> >       bool            disable_page_skipping = false;
> >       bool            process_main = true;
> >       bool            process_toast = true;
> > +     /* by default use buffer access strategy with default size */
> > +     int                     ring_size = -1;
>
> We need to update this comment to something like, "use an invalid value
> for ring_size" so it is clear whether or not the BUFFER_USAGE_LIMIT was
> specified when making the access strategy later". Also, I think just
> removing the comment would be okay, because this is the normal behavior
> for initializing values, I think.

Yeah, I've moved the assignment away from the declaration and wrote
something along those lines.

> > @@ -240,6 +309,17 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
> >                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> >                                errmsg("VACUUM FULL cannot be performed in parallel")));
> >
> > +     /*
> > +      * BUFFER_USAGE_LIMIT does nothing for VACUUM (FULL) so just raise an
> > +      * ERROR for that case.  VACUUM (FULL, ANALYZE) does make use of it, so
> > +      * we'll permit that.
> > +      */
> > +     if ((params.options & VACOPT_FULL) && !(params.options & VACOPT_ANALYZE) &&
> > +             ring_size > -1)
> > +             ereport(ERROR,
> > +                             (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > +                              errmsg("BUFFER_USAGE_LIMIT cannot be specified for VACUUM FULL")));
> > +
> >       /*
> >        * Make sure VACOPT_ANALYZE is specified if any column lists are present.
> >        */
> > @@ -341,7 +421,20 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
> >
> >               MemoryContext old_context = MemoryContextSwitchTo(vac_context);
> >
> > -             bstrategy = GetAccessStrategy(BAS_VACUUM);
>
> Is it worth moving this assert up above when we do the "sanity checking"
> for VACUUM FULL with BUFFER_USAGE_LIMIT?

I didn't do this, but I did adjust that check to check ring_size != -1
and put that as the first condition. It's likely more rare to have
ring_size not set to -1, so probably should check that first.

> s/expliot/exploit

oops

> I might rephrase the last sentence(s). Since it overrides it, I think it
> is clear that if it is not specified, then the thing it overrides is
> used. Then you could phrase the whole thing like:
>
>  "If BUFFER_USAGE_LIMIT was specified by the VACUUM or ANALYZE command,
>  it overrides the value of VacuumBufferUsageLimit.  Either value may be
>  0, in which case GetAccessStrategyWithSize() will return NULL, which is
>  the expected behavior."

Fixed.

> > diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
> > index c1e911b1b3..1b5f779384 100644
> > --- a/src/backend/postmaster/autovacuum.c
> > +++ b/src/backend/postmaster/autovacuum.c
> > @@ -2287,11 +2287,21 @@ do_autovacuum(void)
> >       /*
> > -      * Create a buffer access strategy object for VACUUM to use.  We want to
> > -      * use the same one across all the vacuum operations we perform, since the
> > -      * point is for VACUUM not to blow out the shared cache.
> > +      * Optionally, create a buffer access strategy object for VACUUM to use.
> > +      * When creating one, we want the same one across all tables being
> > +      * vacuumed this helps prevent autovacuum from blowing out shared buffers.
>
> "When creating one" is a bit awkward. I would say something like "Use
> the same BufferAccessStrategy object for all tables VACUUMed by this
> worker to prevent autovacuum from blowing out shared buffers."

Adjusted

> > +     /* Figure out how many buffers ring_size_kb is */
> > +     ring_buffers = ring_size_kb / (BLCKSZ / 1024);
>
> Is there any BLCKSZ that could end up rounding down to 0 and resulting
> in a divide by 0?

I removed that Assert() as I found quite a number of other places in
our code that assume BLCKSZ / 1024 is never 0.

> So, I know we agreed to make it camel cased, but I couldn't help
> mentioning the discussion over in [1] in which Sawada-san says:

I didn't change anything here. I'm happy to follow any rules about
this once they're defined. What we have today is horribly
inconsistent.

> GUC name mentioned in comment is inconsistent with current GUC name.
>
> > +/*
> > + * Upper and lower hard limits for the buffer access strategy ring size
> > + * specified by the vacuum_buffer_usage_limit GUC and BUFFER_USAGE_LIMIT
> > + * option to VACUUM and ANALYZE.

I did adjust this. I wasn't sure it was incorrect as I mentioned "GUC"
as in, the user facing setting.

> Is it worth adding a VACUUM (BUFFER_USAGE_LIMIT 0) vac_option_tab test?

I think so.

I've attached v16.

David