From 492126340f3bb3ab3a89ff04a8b8f4b6fcdb767b Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 26 May 2021 11:50:42 +0530 Subject: [PATCH v5] 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 + + Avoid Using <quote>non-negative</quote> Word in Error Messages + + + Do not use non-negative word in error messages as it looks + ambiguous. Instead, use foo must be an integer value greater than zero or + foo must be an integer value greater than or equal to zero + if option foo expects an integer value. Use + foo must be a numeric value greater than zero or + foo must be a numeric value greater than or equal to zero + if option foo expects a numeric value + + + 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 069a861aab..3add651559 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 workers for vacuum must be greater than or equal to zero"); + pg_log_error("parallel workers for vacuum 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