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

On Thu, Feb 23, 2023 at 3:03 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> Hi,
>
> So, I attached a rough implementation of both the autovacuum failsafe
> reverts to shared buffers and the vacuum option (no tests or docs or
> anything).

Thanks for the patches. I have some comments.

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.

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.

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

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

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)
+ {

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;

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();

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com