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

Thank you to all reviewers!

This email is in answer to the three reviews
done since my last version. Attached is v2 and inline below is replies
to all the review comments.

The main difference between this version and the previous version is
that I've added a guc, buffer_usage_limit and the VACUUM option
BUFFER_USAGE_LIMIT is now to be specified in size (like kB, MB, etc).

I currently only use the guc value for VACUUM, but it is meant to be
used for all buffer access strategies and is configurable at the session
level.

I would prefer that we had the option of resizing the buffer access
strategy object per table being autovacuumed. Since autovacuum reloads
the config file between tables, this would be quite possible.

I started implementing this, but stopped because the code is not really
in a good state for that.

In fact, I'm not very happy with my implementation at all because I
think given the current structure of vacuum() and vacuum_rel(), it will
potentially make the code more confusing.

I don't like how autovacuum and vacuum use vacuum_rel() and vacuum()
differently (autovacuum always calls vacuum() with a list containing a
single relation). And vacuum() takes buffer access strategy as a
parameter, supposedly so that autovacuum can change the buffer access
strategy object per call, but it doesn't do that. And then vacuum() and
vacuum_rel() go and access VacuumParams at various places with no rhyme
or reason -- seemingly just based on the random availability of other
objects whose state they would like to check on. So, IMO, in adding a
"buffers" parameter to VacuumParams, I am asking for confusion in
autovacuum code with table-level VacuumParams containing an value for
buffers that isn't used.

On Mon, Feb 27, 2023 at 4:21 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-02-22 16:32:53 -0500, Melanie Plageman wrote:
> > The first patch in the set is to free the BufferAccessStrategy object
> > that is made in do_autovacuum() -- I don't see when the memory context
> > it is allocated in is destroyed, so it seems like it might be a leak?
>
> The backend shuts down just after, so that's not a real issue. Not that it'd
> hurt to fix it.

I've dropped that patch from the set.

> > > I can see that making sense, particularly if we were to later extend this
> > > to
> > > other users of ringbuffers. E.g. COPYs us of the ringbuffer makes loading
> > > of
> > > data > 16MB but also << s_b vastly slower, but it can still be very
> > > important
> > > to use if there's lots of parallel processes loading data.
> > >
> > > Maybe BUFFER_USAGE_LIMIT, with a value from -1 to N, with -1 indicating the
> > > default value, 0 preventing use of a buffer access strategy, and 1..N
> > > indicating the size in blocks?
>
> > I have found the implementation you suggested very hard to use.
> > The attached fourth patch in the set implements it the way you suggest.
> > I had to figure out what number to set the BUFER_USAGE_LIMIT to -- and,
> > since I don't specify shared buffers in units of nbuffer, it's pretty
> > annoying to have to figure out a valid number.
>
> I think we should be able to parse it in a similar way to how we parse
> shared_buffers. You could even implement this as a GUC that is then set by
> VACUUM (similar to how VACUUM FREEZE is implemented).

in the attached v2, I've used parse_int() to do this.

> > I think that it would be better to have it be either a percentage of
> > shared buffers or a size in units of bytes/kb/mb like that of shared
> > buffers.
>
> I don't think a percentage of shared buffers works particularly well - you
> very quickly run into the ringbuffer becoming impractically big.

It is now a size.

> > > Would we want to set an upper limit lower than implied by the memory limit
> > > for
> > > the BufferAccessStrategy allocation?
> > >
> > >
> > So, I was wondering what you thought about NBuffers / 8 (the current
> > limit). Does it make sense?
>
> That seems *way* too big. Imagine how large allocations we'd end up with a
> shared_buffers size of a few TB.
>
> I'd probably make it a hard error at 1GB and a silent cap at NBuffers / 2 or
> such.

Well, as I mentioned NBuffers / 8 is the current GetAccessStrategy()
cap.

In the attached patchset, I have introduced a hard cap of 16GB which is
enforced for the VACUUM option and for the buffer_usage_limit guc. I
kept the "silent cap" at NBuffers / 8 but am open to changing it to
NBuffers / 2 if we think it is okay for its silent cap to be different
than GetAccessStrategy()'s cap.

> > @@ -547,7 +547,7 @@ bt_check_every_level(Relation rel, Relation heaprel, bool heapkeyspace,
> > state->targetcontext = AllocSetContextCreate(CurrentMemoryContext,
> > "amcheck context",
> > ALLOCSET_DEFAULT_SIZES);
> > - state->checkstrategy = GetAccessStrategy(BAS_BULKREAD);
> > + state->checkstrategy = GetAccessStrategy(BAS_BULKREAD, -1);
> >
> > /* Get true root block from meta-page */
> > metapage = palloc_btree_page(state, BTREE_METAPAGE);
>
> Changing this everywhere seems pretty annoying, particularly because I suspect
> a bunch of extensions also use GetAccessStrategy(). How about a
> GetAccessStrategyExt(), GetAccessStrategyCustomSize() or such?

Yes, I don't know what I was thinking. Changed it to
GetAccessStrategyExt() -- though now I am thinking I don't like Ext and
want to change it.

> > BufferAccessStrategy
> > -GetAccessStrategy(BufferAccessStrategyType btype)
> > +GetAccessStrategy(BufferAccessStrategyType btype, int buffers)
> > {
> > BufferAccessStrategy strategy;
> > int ring_size;
> > + const char *strategy_name = btype_get_name(btype);
>
> Shouldn't be executed when we don't need it.

I got rid of it for now.

> > + if (btype != BAS_VACUUM)
> > + {
> > + if (buffers == 0)
> > + elog(ERROR, "Use of shared buffers unsupported for buffer access strategy: %s. nbuffers must be -1.",
> > + strategy_name);
> > +
> > + if (buffers > 0)
> > + elog(ERROR, "Specification of ring size in buffers unsupported for buffer access strategy: %s. nbuffers must be -1.",
> > + strategy_name);
> > + }
> > +
> > + // TODO: DEBUG logging message for dev?
> > + if (buffers == 0)
> > + btype = BAS_NORMAL;
>
> GetAccessStrategy() often can be executed hundreds of thousands of times a
> second, so I'm very sceptical that adding log messages to it useful.

So, in the case of vacuum and autovacuum, I don't see how
GetAccessStrategyExt() could be called hundreds of thousands of times a
second. It is not even called for each table being vacuumed -- it is
only called before vacuuming a list of tables.

On Tue, Feb 28, 2023 at 3:16 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> On Thu, Jan 12, 2023 at 6:06 AM Andres Freund <andres(at)anarazel(dot)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.

So, I've rebelled a bit and implemented a single guc,
buffer_usage_limit, in the attached patchset. Users can set it at the
session or system level or they can specify BUFFER_USAGE_LIMIT to
vacuum. It is the same size for all operations. By default all of this
would be the same as it is now.

The attached patchset does not use the guc for any operations except
VACUUM, though. I will add on another patch if people still feel
strongly that we cannot have a single guc. If the other operations use
this guc, I think we could get much of the same flexibility as having
multiple gucs by just being able to set it at the session level (or
having command options).

> Although I'm not quite opposing the idea of statement level controls
> (like the VACUUM one proposed here), it is better to make these ring
> buffer sizes configurable across the system to help with the other
> similar cases e.g., a CTAS or RMV can help subsequent reads from
> shared buffers if ring buffer is skipped.

Yes, I've done both.

On Tue, Feb 28, 2023 at 3:52 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Thu, Feb 23, 2023 at 3:03 AM Melanie Plageman
> 0001:
> 1. I don't quite understand the need for this 0001 patch. Firstly,
> bstrategy allocated once per autovacuum worker in AutovacMemCxt which
> goes away with the process. Secondly, the worker exits after
> do_autovacuum() with which memory context is gone. I think this patch
> is unnecessary unless I'm missing something.

I've dropped this one.

> 0002:
> 1. Don't we need to remove vac_strategy for analyze.c as well? It's
> pretty-meaningless there than vacuum.c as we're passing bstrategy to
> all required functions.

So, it is a bit harder to remove it from analyze because acquire_func
func doesn't take the buffer access strategy as a parameter and
acquire_sample_rows uses the vac_context global variable to pass to
table_scan_analyze_next_block().

We could change acquire_func, but it looks like FDW uses it, so I'm not
sure. It would be more for consistency than as a performance win, as I
imagine analyze is less of a problem than vacuum (i.e. it is probably
reading fewer blocks and probably not dirtying them [unless it does
on-access pruning?]).

I haven't done this in the attached set.

> 0004:
> 1. I think no multiple sentences in a single error message. How about
> "of %d, changing it to %d"?
> + elog(WARNING, "buffer_usage_limit %d is below the
> minimum buffer_usage_limit of %d. setting it to %d",

I've removed this message, but if I put back a message about clamping, I
will remember this note.

> 2. Typically, postgres error messages start with lowercase letters,
> hints and detail messages start with uppercase letters.
> + if (buffers == 0)
> + elog(ERROR, "Use of shared buffers unsupported for buffer
> access strategy: %s. nbuffers must be -1.",
> + strategy_name);
> +
> + if (buffers > 0)
> + elog(ERROR, "Specification of ring size in buffers
> unsupported for buffer access strategy: %s. nbuffers must be -1.",
> + strategy_name);

Thanks! I've removed some of the error messages for now, but, for the
ones I kept, I tthink they are consistent now with this pattern.

> 3. A function for this seems unnecessary, especially when a static
> array would do the needful, something like forkNames[].
> +static const char *
> +btype_get_name(BufferAccessStrategyType btype)
> +{
> + switch (btype)
> + {

I've removed it for now.

>
> 4. Why are these assumptions needed? Can't we simplify by doing
> validations on the new buffers parameter only when the btype is
> BAS_VACUUM?
> + if (buffers == 0)
> + elog(ERROR, "Use of shared buffers unsupported for buffer
> access strategy: %s. nbuffers must be -1.",
> + strategy_name);
>
> + // TODO: DEBUG logging message for dev?
> + if (buffers == 0)
> + btype = BAS_NORMAL;

So, I've moved validation to the vacuum option parsing for the vacuum
option and am using the guc infrastructure to check min and max for the
guc value.

> 5. Is this change needed for this patch?
> default:
> elog(ERROR, "unrecognized buffer access strategy: %d",
> - (int) btype);
> - return NULL; /* keep compiler quiet */
> + (int) btype);
> +
> + pg_unreachable();

The pg_unreachable() is removed, as I've left GetAccessStrategy()
untouched.

- Melanie