Thread: Inaccurate error message when set fdw batch_size to 0
Hi,
When testing the fdw batch insert, I found a possible issue.
If I set the batch_size to 0 , it will throw an error:
---------------------
CREATE FOREIGN TABLE test(a int, b varchar)
SERVER testserver
OPTIONS (table_name 'testlocal', batch_size '0');
ERROR: fetch_size requires a non-negative integer value
---------------------
The error message here seems not accurate, because
I can see from the code batch_size should be positive ( > 0).
So, is it better to change the error message to “fetch_size requires a positive integer value” ?
I also found fetch_size has the similar issue, attaching a patch to fix this.
Best regards,
houzj
Attachment
On Sat, May 8, 2021 at 9:09 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > > The error message here seems not accurate, because > > I can see from the code batch_size should be positive ( > 0). > > So, is it better to change the error message to “fetch_size requires a positive integer value” ? > > I also found fetch_size has the similar issue, attaching a patch to fix this. Yes, it should be a positive integer, so your change makes sense. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
From: houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> > So, is it better to change the error message to “fetch_size requires a positive integer value” ? > I also found fetch_size has the similar issue, attaching a patch to fix this. I have a faint memory that I fixed them after receiving the same feedback from someone else, strange... Anyway, thanks. Regards Takayuki Tsunakawa
On 2021/05/10 10:26, tsunakawa.takay@fujitsu.com wrote: > From: houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> >> So, is it better to change the error message to “fetch_size requires a positive integer value” ? >> I also found fetch_size has the similar issue, attaching a patch to fix this. > > I have a faint memory that I fixed them after receiving the same feedback from someone else, strange... Anyway, thanks. +1 for the change of the error messages. One question is; should we back-patch the change of the error message about fetch_size to back branches? Since this is minor thing, is it enough to apply the change only to the master? Even if we should do the back-patch, we would need to wait until upcoming minor release is done before doing that. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> +1 for the change of the error messages.
Yeah, this error message seems outright buggy. However, it's a minor
matter. Also, some people think "positive" is the same thing as
"non-negative", so maybe we need less ambiguous wording?
> One question is; should we back-patch the change of the error message about
> fetch_size to back branches? Since this is minor thing, is it enough to apply
> the change only to the master? Even if we should do the back-patch,
> we would need to wait until upcoming minor release is done before doing that.
+1 for back-patch, but not till after the releases are out. Right now
is no time for inessential changes ...
regards, tom lane
On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Fujii Masao <masao.fujii@oss.nttdata.com> writes: > > +1 for the change of the error messages. > > Yeah, this error message seems outright buggy. However, it's a minor > matter. Also, some people think "positive" is the same thing as > "non-negative", so maybe we need less ambiguous wording? Since value 0 can't be considered as either a positive or negative integer, I think we can do as following(roughly): if (value < 0) "requires a zero or positive integer value" if (value <= 0) "requires a positive integer value" I'm not sure whether we should consider changing these messages: remainder for hash partition must be a non-negative integer parallel vacuum degree must be a non-negative integer repeat count size must be a non-negative integer number of workers must be a non-negative integer %s requires a non-negative numeric value distance in phrase operator should be non-negative and less than %d With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, this error message seems outright buggy. However, it's a minor
>> matter. Also, some people think "positive" is the same thing as
>> "non-negative", so maybe we need less ambiguous wording?
> Since value 0 can't be considered as either a positive or negative
> integer, I think we can do as following(roughly):
> if (value < 0) "requires a zero or positive integer value"
> if (value <= 0) "requires a positive integer value"
I was thinking of avoiding the passive voice and writing
"foo must be greater than zero"
which removes all doubt. It's not necessary to keep the "integer"
aspect of the existing text, because if someone had supplied a
non-integer value, that would not have gotten this far anyway.
> I'm not sure whether we should consider changing these messages:
> remainder for hash partition must be a non-negative integer
> parallel vacuum degree must be a non-negative integer
> repeat count size must be a non-negative integer
> number of workers must be a non-negative integer
> %s requires a non-negative numeric value
> distance in phrase operator should be non-negative and less than %d
I think for consistency it'd be good to change 'em all. I'm almost
tempted to put this matter into our message style guide too.
regards, tom lane
On Mon, May 10, 2021 at 10:09:40AM -0400, Tom Lane wrote: > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: >> if (value < 0) "requires a zero or positive integer value" >> if (value <= 0) "requires a positive integer value" > > I was thinking of avoiding the passive voice and writing > > "foo must be greater than zero" Sounds like a good idea to me. >> I'm not sure whether we should consider changing these messages: >> remainder for hash partition must be a non-negative integer >> parallel vacuum degree must be a non-negative integer >> repeat count size must be a non-negative integer >> number of workers must be a non-negative integer >> %s requires a non-negative numeric value >> distance in phrase operator should be non-negative and less than %d > > I think for consistency it'd be good to change 'em all. I'm almost > tempted to put this matter into our message style guide too. +1. -- Michael
Attachment
On Mon, May 10, 2021 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Yeah, this error message seems outright buggy. However, it's a minor > >> matter. Also, some people think "positive" is the same thing as > >> "non-negative", so maybe we need less ambiguous wording? > > > Since value 0 can't be considered as either a positive or negative > > integer, I think we can do as following(roughly): > > > if (value < 0) "requires a zero or positive integer value" > > if (value <= 0) "requires a positive integer value" > > I was thinking of avoiding the passive voice and writing > > "foo must be greater than zero" +1 for "foo must be greater than zero" if (foo <= 0) kind of errors. But, we also have some values for which zero is accepted, see below error messages. How about the error message "foo must be greater than or equal to zero"? remainder for hash partition must be a non-negative integer parallel vacuum degree must be a non-negative integer repeat count size must be a non-negative integer number of workers must be a non-negative integer distance in phrase operator should be non-negative and less than %d > which removes all doubt. It's not necessary to keep the "integer" > aspect of the existing text, because if someone had supplied a > non-integer value, that would not have gotten this far anyway. This led me to have a look at two postgres_fdw options: fetch_size and batch_size, whether they accept positive non-integers like '123.456', '789.123' and some unsound strings such as '100$%$#$#', '9,223,372,'. It looks like yes, the truncated values 123. 789, 100, 9 (respectively) are accepted. This is because the way strtol is used to fetch the integers from string. I'm not sure if that's intentional. fetch_size = strtol(defGetString(def), NULL, 10); batch_size = strtol(defGetString(def), NULL, 10); I know that fetch_size and batch_size are "number of rows", so no sensible users may specify values with fractional part or non integer characters, but still we can fix this with the endptr parameter of the strtol. Note that for the options fdw_startup_cost and fdw_tuple_cost it's already fixed. I'm thinking of starting a separate thread to discuss this, if this thread is not the right place. > > I'm not sure whether we should consider changing these messages: > > remainder for hash partition must be a non-negative integer > > parallel vacuum degree must be a non-negative integer > > repeat count size must be a non-negative integer > > number of workers must be a non-negative integer > > %s requires a non-negative numeric value > > distance in phrase operator should be non-negative and less than %d > > I think for consistency it'd be good to change 'em all. +1. > I'm almost tempted to put this matter into our message style guide too. +1. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Tue, May 11, 2021 at 11:28 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Mon, May 10, 2021 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > > On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> Yeah, this error message seems outright buggy. However, it's a minor > > >> matter. Also, some people think "positive" is the same thing as > > >> "non-negative", so maybe we need less ambiguous wording? > > > > > Since value 0 can't be considered as either a positive or negative > > > integer, I think we can do as following(roughly): > > > > > if (value < 0) "requires a zero or positive integer value" > > > if (value <= 0) "requires a positive integer value" > > > > I was thinking of avoiding the passive voice and writing > > > > "foo must be greater than zero" > > +1 for "foo must be greater than zero" if (foo <= 0) kind of errors. > But, we also have some values for which zero is accepted, see below > error messages. How about the error message "foo must be greater than > or equal to zero"? > +1 for your proposed message for the cases where we have a check if (foo < 0). Tom, Michael, do you see any problem with the proposed message? We would like to make a similar change at another place [1] so wanted to be consistent. [1] - https://www.postgresql.org/message-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj%3DQ%40mail.gmail.com -- With Regards, Amit Kapila.
On Mon, May 17, 2021 at 4:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, May 11, 2021 at 11:28 AM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Mon, May 10, 2021 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > > Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: > > > > On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > >> Yeah, this error message seems outright buggy. However, it's a minor > > > >> matter. Also, some people think "positive" is the same thing as > > > >> "non-negative", so maybe we need less ambiguous wording? > > > > > > > Since value 0 can't be considered as either a positive or negative > > > > integer, I think we can do as following(roughly): > > > > > > > if (value < 0) "requires a zero or positive integer value" > > > > if (value <= 0) "requires a positive integer value" > > > > > > I was thinking of avoiding the passive voice and writing > > > > > > "foo must be greater than zero" > > > > +1 for "foo must be greater than zero" if (foo <= 0) kind of errors. > > But, we also have some values for which zero is accepted, see below > > error messages. How about the error message "foo must be greater than > > or equal to zero"? > > > > +1 for your proposed message for the cases where we have a check if > (foo < 0). Tom, Michael, do you see any problem with the proposed > message? We would like to make a similar change at another place [1] > so wanted to be consistent. > > [1] - https://www.postgresql.org/message-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj%3DQ%40mail.gmail.com Thanks all for your inputs. PSA v2 patch that uses the new convention. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Attachment
On 2021/05/19 20:01, Bharath Rupireddy wrote: > On Mon, May 17, 2021 at 4:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> On Tue, May 11, 2021 at 11:28 AM Bharath Rupireddy >> <bharath.rupireddyforpostgres@gmail.com> wrote: >>> >>> On Mon, May 10, 2021 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> >>>> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes: >>>>> On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>>>> Yeah, this error message seems outright buggy. However, it's a minor >>>>>> matter. Also, some people think "positive" is the same thing as >>>>>> "non-negative", so maybe we need less ambiguous wording? >>>> >>>>> Since value 0 can't be considered as either a positive or negative >>>>> integer, I think we can do as following(roughly): >>>> >>>>> if (value < 0) "requires a zero or positive integer value" >>>>> if (value <= 0) "requires a positive integer value" >>>> >>>> I was thinking of avoiding the passive voice and writing >>>> >>>> "foo must be greater than zero" >>> >>> +1 for "foo must be greater than zero" if (foo <= 0) kind of errors. >>> But, we also have some values for which zero is accepted, see below >>> error messages. How about the error message "foo must be greater than >>> or equal to zero"? >>> >> >> +1 for your proposed message for the cases where we have a check if >> (foo < 0). Tom, Michael, do you see any problem with the proposed >> message? We would like to make a similar change at another place [1] >> so wanted to be consistent. >> >> [1] - https://www.postgresql.org/message-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj%3DQ%40mail.gmail.com > > Thanks all for your inputs. PSA v2 patch that uses the new convention. Thanks for the patch! ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative numeric value", + errmsg("%s must be greater than or equal to zero", def->defname))); } else if (strcmp(def->defname, "extensions") == 0) @@ -142,7 +142,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS) if (fetch_size <= 0) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s requires a non-negative integer value", + errmsg("%s must be greater than zero", I'm fine to convert "non-negative" word to "greater than" or "greater than or equal to" in the messages. But this change also seems to get rid of the information about the data type of the option from the message. I'm not sure if this is an improvement. Probably isn't it better to convert "requires a non-negative integer value" to "must be an integer value greater than zero"? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Wed, May 19, 2021 at 5:20 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> - errmsg("%s requires a non-negative numeric value",
> + errmsg("%s must be greater than or equal to zero",
> def->defname)));
> }
> else if (strcmp(def->defname, "extensions") == 0)
> @@ -142,7 +142,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
> if (fetch_size <= 0)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> - errmsg("%s requires a non-negative integer value",
> + errmsg("%s must be greater than zero",
>
> I'm fine to convert "non-negative" word to "greater than" or "greater than
> or equal to" in the messages. But this change also seems to get rid of
> the information about the data type of the option from the message.
> I'm not sure if this is an improvement. Probably isn't it better to
> convert "requires a non-negative integer value" to "must be an integer value
> greater than zero"?
Thanks for the comments. Done that way. PSA v3 patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachment
At Wed, 19 May 2021 21:48:56 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> On Wed, May 19, 2021 at 5:20 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > I'm fine to convert "non-negative" word to "greater than" or "greater than
> > or equal to" in the messages. But this change also seems to get rid of
> > the information about the data type of the option from the message.
> > I'm not sure if this is an improvement. Probably isn't it better to
> > convert "requires a non-negative integer value" to "must be an integer value
> > greater than zero"?
>
> Thanks for the comments. Done that way. PSA v3 patch.
--- a/src/backend/utils/adt/tsquery_op.c
+++ b/src/backend/utils/adt/tsquery_op.c
@@ -121,7 +121,7 @@ tsquery_phrase_distance(PG_FUNCTION_ARGS)
if (distance < 0 || distance > MAXENTRYPOS)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("distance in phrase operator should be non-negative and less than %d",
+ errmsg("distance in phrase operator must be an integer value greater than or equal to zero and less
than%d",
MAXENTRYPOS)));
Though it is not due to this patch, but the message looks wrong. The condition is suggesting:
"distance in phrase operator must be an integer value greater than or equal to zero and less than or equal to %d"
I'm not sure readers can read it without biting their tongue. How
about something like the following instead?
"distance in phrase operator must be an integer value between zero and
%d inclusive."
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, May 20, 2021 at 1:44 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 19 May 2021 21:48:56 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> > On Wed, May 19, 2021 at 5:20 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > > I'm fine to convert "non-negative" word to "greater than" or "greater than
> > > or equal to" in the messages. But this change also seems to get rid of
> > > the information about the data type of the option from the message.
> > > I'm not sure if this is an improvement. Probably isn't it better to
> > > convert "requires a non-negative integer value" to "must be an integer value
> > > greater than zero"?
> >
> > Thanks for the comments. Done that way. PSA v3 patch.
>
> --- a/src/backend/utils/adt/tsquery_op.c
> +++ b/src/backend/utils/adt/tsquery_op.c
> @@ -121,7 +121,7 @@ tsquery_phrase_distance(PG_FUNCTION_ARGS)
> if (distance < 0 || distance > MAXENTRYPOS)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("distance in phrase operator should be non-negative and less than %d",
> + errmsg("distance in phrase operator must be an integer value greater than or equal
tozero and less than %d",
> MAXENTRYPOS)));
>
> Though it is not due to this patch, but the message looks wrong. The condition is suggesting:
>
> "distance in phrase operator must be an integer value greater than or equal to zero and less than or equal to %d"
>
> I'm not sure readers can read it without biting their tongue. How
> about something like the following instead?
>
> "distance in phrase operator must be an integer value between zero and
> %d inclusive."
Thanks. That looks better. PSA v4 patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, May 20, 2021 at 2:43 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > Thanks. That looks better. PSA v4 patch. Attaching v5 patch rebased on latest master. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Attachment
On 2021/05/26 15:22, Bharath Rupireddy wrote:
> On Thu, May 20, 2021 at 2:43 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> Thanks. That looks better. PSA v4 patch.
>
> Attaching v5 patch rebased on latest master.
The patch could not be applied cleanly because of recent commit d854720df6.
Could you rebase the patch?
- /* these must have a non-negative numeric value */
+ /* these must have a positive numeric value */
Isn't it better to replace this with "these must have a floating point value
greater than or equal to zero"?
- errmsg("%s requires a non-negative numeric value",
+ errmsg("\"%s\" must be a numeric value greater than or equal to
zero",
"numeric" should be "floating point"?
+ <quote>foo must be a numeric value greater than zero</quote> or
+ <quote>foo must be a numeric value greater than or equal to zero</quote>
+ if option <quote>foo</quote> expects a numeric value
Maybe this description about numeric value is redundant
because there is already the description about integer value?
- /* Number of workers should be non-negative. */
Isn't it better to replace this with "Number of workers should be greater than zero"
rather than removing the comment?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Jul 9, 2021 at 6:25 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> The patch could not be applied cleanly because of recent commit d854720df6.
> Could you rebase the patch?
Thanks. Done.
> - /* these must have a non-negative numeric value */
> + /* these must have a positive numeric value */
>
> Isn't it better to replace this with "these must have a floating point value
> greater than or equal to zero"?
Changed.
> - errmsg("%s requires a non-negative numeric value",
> + errmsg("\"%s\" must be a numeric value greater than or equal to
zero",
>
> "numeric" should be "floating point"?
Changed.
> + <quote>foo must be a numeric value greater than zero</quote> or
> + <quote>foo must be a numeric value greater than or equal to zero</quote>
> + if option <quote>foo</quote> expects a numeric value
>
> Maybe this description about numeric value is redundant
> because there is already the description about integer value?
Removed.
> - /* Number of workers should be non-negative. */
>
> Isn't it better to replace this with "Number of workers should be greater than zero"
> rather than removing the comment?
Changed.
PSA v6 patch.
Regards,
Bharath Rupireddy.
Attachment
On 2021/07/09 11:41, Bharath Rupireddy wrote:
> PSA v6 patch.
Thanks for updating the patch!
+ <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?
- 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?
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")));
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao <masao.fujii@oss.nttdata.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.
On Mon, Jul 12, 2021 at 10:11 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao <masao.fujii@oss.nttdata.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. Regards, Bharath Rupireddy.
Attachment
On Thu, Jul 15, 2021 at 7:54 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Mon, Jul 12, 2021 at 10:11 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao <masao.fujii@oss.nttdata.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. Regards, Bharath Rupireddy.
Attachment
On 2021/07/26 13:56, Bharath Rupireddy wrote:
> On Thu, Jul 15, 2021 at 7:54 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Mon, Jul 12, 2021 at 10:11 PM Bharath Rupireddy
>> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>>
>>> On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao <masao.fujii@oss.nttdata.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
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>
- /* 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.
- 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
Attachment
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.
On 2021/07/27 15:06, Bharath Rupireddy wrote: > Thanks for the v8 patch, it LGTM. Pushed. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION