Re: Inaccurate error message when set fdw batch_size to 0

On Mon, Jul 26, 2021 at 9:37 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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 about the following description, instead? I replaced this description with the following. Patch attached. I also uppercased the 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 there are still other comments using "non-negative", I don't think we need to change only this comment for now. So I excluded this 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.