On 2021/07/26 13:56, Bharath Rupireddy wrote:
On Thu, Jul 15, 2021 at 7:54 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
On Mon, Jul 12, 2021 at 10:11 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao <masao.fu...@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 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
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 4593cbc540..c574ca2cf3 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -119,7 +119,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
else if (strcmp(def->defname, "fdw_startup_cost") == 0 ||
strcmp(def->defname, "fdw_tuple_cost") == 0)
{
- /* these must have a non-negative numeric value */
+ /*
+ * These must have a floating point value greater than
or equal to
+ * zero.
+ */
char *value;
double real_val;
bool is_parsed;
@@ -136,7 +139,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
if (real_val < 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"%s\" requires a
non-negative floating point value",
+ errmsg("\"%s\" must be a
floating point value greater than or equal to zero",
def->defname)));
}
else if (strcmp(def->defname, "extensions") == 0)
@@ -163,7 +166,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
if (int_val <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"%s\" requires a
non-negative integer value",
+ errmsg("\"%s\" must be an
integer value greater than zero",
def->defname)));
}
else if (strcmp(def->defname, "password_required") == 0)
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index 3f2c40b750..e6ae02f2af 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -828,6 +828,16 @@ BETTER: unrecognized node type: 42
</para>
</formalpara>
+ <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>
+
</simplesect>
<simplesect>
diff --git a/src/backend/partitioning/partbounds.c
b/src/backend/partitioning/partbounds.c
index 12599da9a8..25018b1a8b 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -4760,11 +4760,11 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
if (modulus <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("modulus for hash partition must be a
positive integer")));
+ errmsg("modulus for hash partition must be an
integer value greater than zero")));
if (remainder < 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("remainder for hash partition must be a
non-negative integer")));
+ errmsg("remainder for hash partition must be
an integer value greater than or equal to zero")));
if (remainder >= modulus)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/utils/adt/tsquery_op.c
b/src/backend/utils/adt/tsquery_op.c
index 0575b55272..8211e6c9bc 100644
--- 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 between zero and %d inclusive",
MAXENTRYPOS)));
if (a->size == 0)
{
diff --git a/src/test/modules/test_shm_mq/test.c
b/src/test/modules/test_shm_mq/test.c
index e5e32abac2..2d8d695f97 100644
--- a/src/test/modules/test_shm_mq/test.c
+++ b/src/test/modules/test_shm_mq/test.c
@@ -57,17 +57,17 @@ test_shm_mq(PG_FUNCTION_ARGS)
if (loop_count < 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("repeat count size must be a
non-negative integer")));
+ errmsg("repeat count size must be an integer
value greater than or equal to zero")));
/*
* Since this test sends data using the blocking interfaces, it cannot
* send data to itself. Therefore, a minimum of 1 worker is required.
Of
* course, a negative worker count is nonsensical.
*/
- 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")));
/* Set up dynamic shared memory segment and background workers. */
test_shm_mq_setup(queue_size, nworkers, &seg, &outqh, &inqh);
@@ -149,7 +149,7 @@ test_shm_mq_pipelined(PG_FUNCTION_ARGS)
if (loop_count < 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("repeat count size must be a
non-negative integer")));
+ errmsg("repeat count size must be an integer
value greater than or equal to zero")));
/*
* Using the nonblocking interfaces, we can even send data to ourselves,
@@ -158,7 +158,7 @@ test_shm_mq_pipelined(PG_FUNCTION_ARGS)
if (nworkers < 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("number of workers must be a
non-negative integer")));
+ errmsg("number of workers must be an integer
value greater than or equal to zero")));
/* Set up dynamic shared memory segment and background workers. */
test_shm_mq_setup(queue_size, nworkers, &seg, &outqh, &inqh);
diff --git a/src/test/regress/expected/hash_part.out
b/src/test/regress/expected/hash_part.out
index b79c2b44c1..ac3aabee02 100644
--- a/src/test/regress/expected/hash_part.out
+++ b/src/test/regress/expected/hash_part.out
@@ -19,10 +19,10 @@ SELECT satisfies_hash_partition('mchash1'::regclass, 4, 0,
NULL);
ERROR: "mchash1" is not a hash partitioned table
-- invalid modulus
SELECT satisfies_hash_partition('mchash'::regclass, 0, 0, NULL);
-ERROR: modulus for hash partition must be a positive integer
+ERROR: modulus for hash partition must be an integer value greater than zero
-- remainder too small
SELECT satisfies_hash_partition('mchash'::regclass, 1, -1, NULL);
-ERROR: remainder for hash partition must be a non-negative integer
+ERROR: remainder for hash partition must be an integer value greater than or
equal to zero
-- remainder too large
SELECT satisfies_hash_partition('mchash'::regclass, 1, 1, NULL);
ERROR: remainder for hash partition must be less than modulus
