Re: pgsql: Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option

On Tue, 11 Apr 2023 at 15:02, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> > postgres=# vacuum (buffer_usage_limit);
> > ERROR:  buffer_usage_limit requires a valid value
>
> The error message doesn't really make much sense to me.  In the same
> context, most of the code seems to use ("%s requires a parameter",
> buffer_usage_limit) without "valid".

The only other option I see that requires a value in this context is
"parallel", and what you say is not true for that, so I'm not sure I
follow what you're referring to with "most of the code". Can you quote
the code you mean?

> vacuum.c:347
> >                errmsg("BUFFER_USAGE_LIMIT cannot be specified for VACUUM FULL")));
>
> It seems like that the vacuum options are usually spelled in uppercase
> at least for vacuum and analyze. In any case, shouldn't we need to
> unify them in a certain area?  (For example, command options for
> CREATE SUBSCRIPTION is shown in lowercase in the documentation. But,
> I'm not exactly sure what we should do about it.)

I wouldn't object to making things more consistent in this area with
regards to the casing of the options in the ERROR messages.  However,
it doesn't really seem like there is much consistency to follow that
this new code is breaking.

For example, both of these are not upper casing the option name:

ereport(ERROR,
        (errcode(ERRCODE_SYNTAX_ERROR),
        errmsg("parallel option requires a value between 0 and %d",
        MAX_PARALLEL_WORKER_LIMIT),
        parser_errposition(pstate, opt->location)));

ereport(ERROR,
        (errcode(ERRCODE_SYNTAX_ERROR),
        errmsg("parallel workers for vacuum must be between 0 and %d",
        MAX_PARALLEL_WORKER_LIMIT),
        parser_errposition(pstate, opt->location)));

But if you wanted to change that, you'll need to raise a thread on
-hackers as that code has been there for a while.

(actually, it seems pointless to have two error messages for that any
not just one)

I do agree that it's not very good that I pushed the lowercase version
of buffer_usage_limit and the same in uppercase for the VACUUM FULL
conflict.  I'll hold back from fixing that until we figure out the
other stuff.

David