On Thu, May 20, 2021 at 1:44 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
>
> At Wed, 19 May 2021 21:48:56 +0530, Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote in
> > On Wed, May 19, 2021 at 5:20 PM Fujii Masao <masao.fu...@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
From 3dea08a9f6a0a39ea48ea2db67791d46db7997a9 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 20 May 2021 14:23:03 +0530
Subject: [PATCH v4] Disambiguate error messages that use "non-negative"
Using "non-negative" word in the error message looks ambiguous
since value 0 can't be considered as either a positive or negative
integer and some users might think "positive" is the same as
"non-negative". So, be clear with the following messages:
if (foo <= 0) then use "foo must be greater than zero" and
if (foo < 0) then use "foo must be greater than or equal to zero"
Also, added a note in the Error Message Style Guide to help writing
the new messages.
Authors: Hou Zhijie, Bharath Rupireddy.
---
contrib/postgres_fdw/option.c | 8 ++++----
doc/src/sgml/sources.sgml | 14 ++++++++++++++
src/backend/access/transam/parallel.c | 1 -
src/backend/partitioning/partbounds.c | 4 ++--
src/backend/utils/adt/tsquery_op.c | 2 +-
src/bin/scripts/vacuumdb.c | 2 +-
src/test/modules/test_shm_mq/test.c | 10 +++++-----
src/test/regress/expected/hash_part.out | 4 ++--
8 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 672b55a808..a0d771a16b 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -118,7 +118,7 @@ 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 positive numeric value */
double val;
char *endp;
@@ -126,7 +126,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
if (*endp || val < 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s requires a non-negative numeric value",
+ errmsg("\"%s\" must be a numeric value 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 an integer value greater than zero",
def->defname)));
}
else if (strcmp(def->defname, "batch_size") == 0)
@@ -153,7 +153,7 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
if (batch_size <= 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- 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..b9c632a349 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -880,6 +880,20 @@ BETTER: unrecognized node type: 42
</para>
</simplesect>
+ <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. Use
+ <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
+ </para>
+ </simplesect>
+
</sect1>
<sect1 id="source-conventions">
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 3550ef13ba..cb978845fe 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -170,7 +170,6 @@ CreateParallelContext(const char *library_name, const char *function_name,
/* It is unsafe to create a parallel context if not in parallel mode. */
Assert(IsInParallelMode());
- /* Number of workers should be non-negative. */
Assert(nworkers >= 0);
/* We might be running in a short-lived memory context. */
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7925fcce3b..ac4ae65189 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -4698,11 +4698,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/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7901c41f16..3434ce2793 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -200,7 +200,7 @@ main(int argc, char *argv[])
vacopts.parallel_workers = atoi(optarg);
if (vacopts.parallel_workers < 0)
{
- pg_log_error("parallel vacuum degree must be a non-negative integer");
+ pg_log_error("parallel vacuum degree must be an integer value greater than or equal to zero");
exit(1);
}
break;
diff --git a/src/test/modules/test_shm_mq/test.c b/src/test/modules/test_shm_mq/test.c
index e5e32abac2..8867ba4e6b 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 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 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
--
2.25.1
