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

Hi,

On 2023-02-22 16:32:53 -0500, Melanie Plageman wrote:
> I was wondering about the status of the autovacuum wraparound failsafe
> test suggested in [1]. I don't see it registered for the March's
> commitfest. I'll probably review it since it will be useful for this
> patchset.

It's pretty hard to make it work reliably. I was suggesting somewhere that we
ought to add a EMERGENCY parameter to manual VACUUMs to allow testing that
path a tad more easily.


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


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


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


> @@ -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?


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


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

Greetings,

Andres Freund