Re: Inaccurate error message when set fdw batch_size to 0

On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> + <simplesect>
> + <title>Avoid Using <quote>non-negative</quote> Word in Error Messages</title>
> +
> + <para>
> + Do not use <quote>non-negative</quote> word in error messages as it looks
> + ambiguous. Instead, use <quote>foo must be an integer value greater than zero</quote>
> + or <quote>foo must be an integer value greater than or equal to zero</quote>
> + if option <quote>foo</quote> expects an integer value.
> + </para>
> + </simplesect>
>
> It seems suitable to put this guide under "Tricky Words to Avoid"
> rather than adding it as separate section. Thought?

+1. I will change.

> - if (nworkers < 1)
> + if (nworkers <= 0)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("number of workers must be a positive integer")));
> + errmsg("number of workers must be an integer value greater than zero")));
>
> You replaced "positve" with "greater than zero". So the error message
> style guide should mention not only "non-negative" but also "positive"
> (probably also "negative") keyword?

The main focus of the patch is to replace the ambiguous "non-negative"
work in the error message. Let's keep it to that. However, I changed
below two messages too to keep them in sync with nearby messages.
Also, there seems to be an ambiguity in treating 0 as a positive or
negative integer, I thought it makes sense to replace them. But, if
others don't agree, I'm happy to revert.

- errmsg("modulus for hash partition must be a positive integer")));
+ errmsg("modulus for hash partition must be an integer value greater
than zero")));
- errmsg("number of workers must be a positive integer")));
+ errmsg("number of workers must be an integer value greater than zero")));

> If this is true, there are still many messages using "positive" or "negative"
> keyword as follows. We should also fix them at all? Of course,
> which would increase the change too big unnecessarily, I'm afraid, though..
>
> src/backend/storage/ipc/signalfuncs.c: errmsg("\"timeout\" must not be negative")));
> src/backend/commands/functioncmds.c: errmsg("COST must be positive")));

You are right. The change is going to be an unnecessary one. So, let's
not do that.

Regards,
Bharath Rupireddy.