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

