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

Thanks for the reviews and for trying the patch!

On Wed, Mar 15, 2023 at 4:31 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Mar 11, 2023 at 11:55 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > > On Tue, Feb 28, 2023 at 3:16 AM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > > On Thu, Jan 12, 2023 at 6:06 AM Andres Freund <andres@anarazel.de> wrote:
> > > > >
> > > > > On 2023-01-11 17:26:19 -0700, David G. Johnston wrote:
> > > > > > Should we just add "ring_buffers" to the existing "shared_buffers" and
> > > > > > "temp_buffers" settings?
> > > > >
> > > > > The different types of ring buffers have different sizes, for good reasons. So
> > > > > I don't see that working well. I also think it'd be more often useful to
> > > > > control this on a statement basis - if you have a parallel import tool that
> > > > > starts NCPU COPYs you'd want a smaller buffer than a single threaded COPY. Of
> > > > > course each session can change the ring buffer settings, but still.
> > > >
> > > > How about having GUCs for each ring buffer (bulk_read_ring_buffers,
> > > > bulk_write_ring_buffers, vacuum_ring_buffers - ah, 3 more new GUCs)?
> > > > These options can help especially when statement level controls aren't
> > > > easy to add (COPY, CREATE TABLE AS/CTAS, REFRESH MAT VIEW/RMV)? If
> > > > needed users can also set them at the system level. For instance, one
> > > > can set bulk_write_ring_buffers to other than 16MB or -1 to disable
> > > > the ring buffer to use shared_buffers and run a bunch of bulk write
> > > > queries.
> >
> > In attached v3, I've changed the name of the guc from buffer_usage_limit
> > to vacuum_buffer_usage_limit, since it is only used for vacuum and
> > autovacuum.
> >
> > I haven't added the other suggested strategy gucs, as those could easily
> > be done in a future patchset.
> >
> > I've also changed GetAccessStrategyExt() to GetAccessStrategyWithSize()
> > -- similar to initArrayResultWithSize().
> >
> > And I've added tab completion for BUFFER_USAGE_LIMIT so that it is
> > easier to try out my patch.
> >
> > Most of the TODOs in the code are related to the question of how
> > autovacuum uses the guc vacuum_buffer_usage_limit. autovacuum creates
> > the buffer access strategy ring in do_autovacuum() before looping
> > through and vacuuming tables. It passes this strategy object on to
> > vacuum(). Since we reuse the same strategy object for all tables in a
> > given invocation of do_autovacuum(), only failsafe autovacuum would
> > change buffer access strategies. This is probably okay, but it does mean
> > that the table-level VacuumParams variable, ring_size, means something
> > different for autovacuum than vacuum. Autovacuum workers will always
> > have set it to -1. We won't ever reach code in vacuum() which relies on
> > VacuumParams->ring_size as long as autovacuum workers pass a non-NULL
> > BufferAccessStrategy object to vacuum(), though.
>
> I've not reviewed the patchset in depth yet but I got assertion
> failure and SEGV when using the buffer_usage_limit parameter.
>
> postgres(1:471180)=# vacuum (buffer_usage_limit 10000000000) ;
> 2023-03-15 17:10:02.947 JST [471180] ERROR:  buffer_usage_limit for a
> vacuum must be between -1 and 16777216. 10000000000 is invalid. at
> character 9
>
> The message show the max value is 16777216, but when I set it, I got
> an assertion failure:
>
> postgres(1:470992)=# vacuum (buffer_usage_limit 16777216) ;
> TRAP: failed Assert("ring_size < MAX_BAS_RING_SIZE_KB"), File:
> "freelist.c", Line: 606, PID: 470992
>
> Then when I used 1 byte lower value, 16777215, I got a SEGV:
>
> postgres(1:471180)=# vacuum (buffer_usage_limit 16777215) ;
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> The connection to the server was lost. Attempting reset: 2023-03-15
> 17:10:59.404 JST [471159] LOG:  server process (PID 471180) was
> terminated by signal 11: Segmentation fault
>
> Finally, when I used a more lower value, 16777100, I got a memory
> allocation error:
>
> postgres(1:471361)=# vacuum (buffer_usage_limit 16777100) ;
> 2023-03-15 17:12:17.853 JST [471361] ERROR:  invalid memory alloc
> request size 18446744073709551572
>
> Probably vacuum_buffer_usage_limit also has the same issue.

Oh dear--it seems I had an integer overflow when calculating the number
of buffers using the specified buffer size in the macro:

 #define bufsize_limit_to_nbuffers(bufsize) (bufsize * 1024 / BLCKSZ)

In the attached v4, I've changed that to:

  static inline int
  bufsize_limit_to_nbuffers(int bufsize_limit_kb)
  {
    int blcksz_kb = BLCKSZ / 1024;

    Assert(blcksz_kb > 0);

    return bufsize_limit_kb / blcksz_kb;
  }

This should address Justin's suggestions and Ants' concern about the
macro as well.

Also, I was missing the = in the Assert(ring_size <= MAX_BAS_RING_SIZE)
I've fixed that as well, so it should work for you to specify up to 16777216.

> Also, should we support a table option for vacuum_buffer_usage_limit as well?

Hmm. Since this is meant more for balancing resource usage globally, it
doesn't make as much sense as a table option to me. But, I could be
convinced.

On Sat, Mar 11, 2023 at 2:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Sat, Mar 11, 2023 at 09:55:33AM -0500, Melanie Plageman wrote:
> > Subject: [PATCH v3 2/3] use shared buffers when failsafe active
> >
> > +             /*
> > +              * Assume the caller who allocated the memory for the
> > +              * BufferAccessStrategy object will free it.
> > +              */
> > +             vacrel->bstrategy = NULL;
>
> This comment could use elaboration:
>
> ** VACUUM normally restricts itself to a small ring buffer; but in
> failsafe mode, in order to process tables as quickly as possible, allow
> the leaving behind large number of dirty buffers.

I've added a comment in attached v4 which is a bit different than Justin's
suggestion but still more verbose than the previous comment.

> > Subject: [PATCH v3 3/3] add vacuum option to specify ring_size and guc
> > +             {"vacuum_buffer_usage_limit", PGC_USERSET, RESOURCES_MEM,
> > +                     gettext_noop("Sets the buffer pool size for operations employing a buffer access strategy."),
>
> The description should mention vacuum, if that's the scope of the GUC's
> behavior.

I've updated this in v4.

> > +#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.

So, on the topic of "other documentation", I have, at least, added docs
for the vacuum_buffer_usage_limit guc and the BUFFER_USAGE option to
VACUUM and the buffer-usage-limit parameter to vacuumdb.

> This patch should add support in vacuumdb.c.  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".

So, v4 adds support for buffer-usage-limit to vacuumdb. There are a few
issues. The main one is that no other vacuumdb option takes a size as a
parameter. I couldn't actually find any other client with a parameter
specified as a size.

My VACUUM option code is using the GUC size parsing code from
parse_int() -- including the unit flag GUC_UNIT_KB. Now that vacuumdb
also needs to parse sizes, I think we'll need to lift the parse_int()
code and the unit_conversion struct and
unit_conversion_memory_unit_conversion_table out of guc.c and put it
somewhere that it can be accessed for more than guc parsing (e.g. option
parsing).

For vacuumdb in this version, I just specified buffer-usage-limit is
only in kB and thus can only be specified as an int.

If we had something like pg_parse_size() in common, would this make
sense? It would be a little bit of work to figure out what to do about
the flags, etc.

Another issue is the server-side guc
#define MAX_BAS_RING_SIZE_KB (16 * 1024 * 1024)
I just redefined it in vacuumdb code. I'm not sure what the preferred
method for dealing with this is.

I know this validation would get done server-side if I just passed the
user-specified option through, but all of the other vacuumdb options
appear to be doing min/max boundary validation on the client side.

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.

> > > > +#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.

So, I was thinking it would be good to have some documentation in
general about Buffer Access Strategies (i.e. not just for vacuum). It
would have been nice to have something to reference from the pg_stat_io
docs that describe what buffer access strategies are.

> > > 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 */

I've added a little something to the comment above the VacuumParams
struct.

> > 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".

I've squished in the code for adding the parameter to vacuumdb in a
single commit with the guc and vacuum option, but I will separate it out
after some of the basics get sorted.

- Melanie