From 0a97d861ad1f0b7112612fc6bcc9536477e4a236 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Fri, 9 Jul 2021 02:40:31 +0000 Subject: [PATCH v6] 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 | 9 ++++++--- doc/src/sgml/sources.sgml | 11 +++++++++++ src/backend/access/transam/parallel.c | 2 +- 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(+), 15 deletions(-) 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..52804b1ae8 100644 --- a/doc/src/sgml/sources.sgml +++ b/doc/src/sgml/sources.sgml @@ -880,6 +880,17 @@ 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. + + + diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 3550ef13ba..983ddb0da5 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -170,7 +170,7 @@ 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. */ + /* Number of parallel workers should be greater than zero. */ 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 38baaefcda..e55942e9c8 100644 --- a/src/backend/partitioning/partbounds.c +++ b/src/backend/partitioning/partbounds.c @@ -4761,11 +4761,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 61974baa78..8fff1c7730 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -203,7 +203,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