Inaccurate error message when set fdw batch_size to 0

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

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]/messages/by-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj=Q@mail.gmail.com
so wanted to be consistent.

[1]: /messages/by-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj=Q@mail.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] - /messages/by-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj=Q@mail.gmail.com

Thanks all for your inputs. PSA v2 patch that uses the new convention.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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] - /messages/by-id/CALj2ACWGB9oHCR5ygkc8u6_QDqecObf9j2MxtOgsjZMMKsLj=Q@mail.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

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

Thanks. That looks better. PSA v4 patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

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.

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.