Re: Inaccurate error message when set fdw batch_size to 0

On Mon, Jul 26, 2021 at 9:37 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> +  <formalpara>
> +    <title>non-negative</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>
> +  </formalpara>
>
> This description looks a bit redundant. And IMO it's better to also document how "non-negative" is ambiguous. So what
aboutthe following description, instead? I replaced this description with the following. Patch attached. I also
uppercasedthe first character "n" of "non-negative" at the title for the sake of consistency with other items. 
>
> +  <formalpara>
> +    <title>Non-negative</title>
> +   <para>
> +    Avoid <quote>non-negative</quote> as it is ambiguous
> +    about whether it accepts zero.  It's better to use
> +    <quote>greater than zero</quote> or
> +    <quote>greater than or equal to zero</quote>.
> +   </para>
> +  </formalpara>

LGTM.

> -       /* Number of workers should be non-negative. */
> +       /* Number of parallel workers should be greater than zero. */
>         Assert(nworkers >= 0);
>
> This should be "greater than or equal to zero", instead? Anyway since this is comment not an error message, and also
thereare still other comments using "non-negative", I don't think we need to change only this comment for now. So I
excludedthis change from the patch. Maybe we can get rid of all "non-negative" from comments and documents later *if*
necessary.

+1 to not change any code comments.

> -                                errmsg("repeat count size must be a non-negative integer")));
> +                                errmsg("repeat count size must be greater than or equal to zero")));
>
> -                                errmsg("number of workers must be a non-negative integer")));
> +                                errmsg("number of workers must be greater than or equal to zero")));
>
> Isn't it better to replace "be greater" with "be an integer value greater"? I applied this to the patch.

+1.

Thanks for the v8 patch, it LGTM.

Regards,
Bharath Rupireddy.