Re: Inaccurate error message when set fdw batch_size to 0

On 2021/07/26 13:56, Bharath Rupireddy wrote:
> On Thu, Jul 15, 2021 at 7:54 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>
>> On Mon, Jul 12, 2021 at 10:11 PM Bharath Rupireddy
>> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>>
>>> 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.
>>
>> PSA v7 patch with the above change.
>
> PSA v8 patch rebased on to latest master.

Thanks for updating the patch!

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

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

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

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION