Re: ERROR messages in VACUUM's PARALLEL option - Mailing list pgsql-hackers

On Tue, Apr 11, 2023 at 4:00 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> BUFFER_USAGE_LIMIT option.
>
> 1) buffer_usage_limit in the ERROR messages should be consistently in uppercase.

I did notice that all the other VACUUM options don't do this:

postgres=# vacuum (process_toast 'boom') foo;
ERROR:  process_toast requires a Boolean value
postgres=# vacuum (full 2) foo;
ERROR:  full requires a Boolean value
postgres=# vacuum (verbose 2) foo;
ERROR:  verbose requires a Boolean value

Though, I actually prefer uppercase. They are all documented in
uppercase, so I don't see why they would all be lowercase in the error
messages. Additionally, for BUFFER_USAGE_LIMIT, I find the uppercase
helpful to differentiate it from the GUC vacuum_buffer_usage_limit.

> 2) defGetString() already checks for opt->args == NULL and raises an
> ERROR when it is.
>
> I suspect that Melanie might have followed the lead of the PARALLEL
> option when she was working on adding the BUFFER_USAGE_LIMIT patch.

Yes, I mostly imitated parallel since it was the other VACUUM option
with a valid range (as opposed to a boolean or enum of acceptable
values).

> What I'm wondering now is:
>
> a) Is it worth changing this for the PARALLEL option too? and;
> b) I see we lose the parse position indicator, and;

I'm not too worried about the parse position indicator, as we don't get
it for the majority of the errors about valid values. And, as you say
later in your email, the VACUUM options are pretty simple so it should
be easy for the user to figure out without a parse position indicator.

postgres=# vacuum (SKIP_LOCKED 2) foo;
ERROR:  skip_locked requires a Boolean value

While trying different combinations, I noticed that defGetInt32 produces
a somewhat unsatisfactory error message when I provide an argument which
would overflow an int32

postgres=# vacuum (parallel 3333333333333333333333) foo;
ERROR:  parallel requires an integer value

In defGetInt32(), the def->arg nodeTag is T_Float, which is why we get
this error message. Perhaps it is worth checking somewhere in the stack
for integer overflow (instead of assuming it is a float) and giving a
more helpful message like the one from parse_int()?

    if (val > INT_MAX || val < INT_MIN)
    {
        if (hintmsg)
            *hintmsg = gettext_noop("Value exceeds integer range.");
        return false;
    }

Probably not a trivial task and not one for 16, however.

I will say that I prefer the new error message introduced by this
patch's usage of defGetInt32() when an argument is not specified to the
previous error message.

postgres=# vacuum (parallel) foo;
ERROR:  parallel requires an integer value

On Tue, Apr 11, 2023 at 9:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Over in [1], Horiguchisan mentioned a few things about VACUUM's new
> > BUFFER_USAGE_LIMIT option.
>
> > 1) buffer_usage_limit in the ERROR messages should be consistently in uppercase.
>
> FWIW, I think this is exactly backward, and so is whatever code you
> based this on.  Our usual habit is to write GUC names and suchlike
> in lower case in error messages.  Add double quotes if you feel you
> want to set them off from the surrounding text.  Here's a typical
> example of longstanding style:
>
> regression=# set max_parallel_workers_per_gather to -1;
> ERROR:  -1 is outside the valid range for parameter "max_parallel_workers_per_gather" (0 .. 1024)

It seems it also is true for VACUUM options currently, but you didn't
mention command options. Do you also feel these should be lowercase?

- Melanie