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

Greetings,

Andres Freund