Обсуждение: ERROR messages in VACUUM's PARALLEL option

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

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;
c) If we did want to change this, is it too late for v16?

For a), I know that's much older code, so perhaps it's not worth
messing around with the ERROR messages for this. For b), this seems
like a fairly minor detail given that VACUUM commands are fairly
simple. It shouldn't be too hard for a user to see what we're talking
about.

I've attached a patch to adjust this.

David

[1] https://postgr.es/m/20230411.102335.1643720544536884844.horikyota.ntt@gmail.com

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)

            regards, tom lane

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

(On Wed, 12 Apr 2023 at 01:58, 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:

Thanks for chipping in on this. Can you confirm if you think this
should apply to VACUUM options?  We're not talking GUCs here.

I think there might be some precedents creeping in here for the legacy
VACUUM syntax.  Roughly the current ERROR messages are using upper
case. e.g:

"PROCESS_TOAST required with VACUUM FULL"
"VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL"
"ANALYZE option must be specified when a column list is provided"

It's quite possible the newer options copied what VACUUM FULL did, and
VACUUM FULL was probably upper case from before we had the newer
syntax with the options in parenthesis.

I did notice that defGetString() did lower case, but that not exactly
terrible. It seems worse to have ExecVacuum() use a random assortment
of casings when reporting errors in the specified options. Unless
someone thinks are should edit all of those to lower case, then I
think the best option is just to make BUFFER_USAGE_LIMIT follow the
existing precedent of upper casing.

David

David Rowley <dgrowleyml@gmail.com> writes:
> Thanks for chipping in on this. Can you confirm if you think this
> should apply to VACUUM options?  We're not talking GUCs here.

My druthers would be to treat them similarly to GUCs.
I recognize that I might be in the minority, and that doing
so would entail touching a lot of existing messages in this
area.  Nonetheless, I think this area is not being consistent
with our wider conventions, which is why for example defGetString
is out of step with these messages.

            regards, tom lane

At Tue, 11 Apr 2023 18:16:40 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> David Rowley <dgrowleyml@gmail.com> writes:
> > Thanks for chipping in on this. Can you confirm if you think this
> > should apply to VACUUM options?  We're not talking GUCs here.
> 
> My druthers would be to treat them similarly to GUCs.

IMHO I like this direction.

> I recognize that I might be in the minority, and that doing
> so would entail touching a lot of existing messages in this
> area.  Nonetheless, I think this area is not being consistent
> with our wider conventions, which is why for example defGetString
> is out of step with these messages.

On the other hand, the documentation write optinos in uppercase
[1]. It is why I did not push hard for the normalization.

[1] https://www.postgresql.org/docs/devel/sql-vacuum.html

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center