On Wed, Jul 21, 2021 at 05:02:29PM +0900, Kyotaro Horiguchi wrote: > The difference is your suggestion is making the function output the > message within. I guess that the reason for the original proposal is > different style of message is required in several places.
That's one step toward having a maximum number of frontend tools to use the central logging APIs of src/common/. > 1. Some "bare" options (that is not preceded by a hyphen option) like > PID of pg_ctl kill doesn't fit the format. \pset parameters of > pg_ctl is the same. Yep. I was reviewing this one, but I have finished by removing it. The argument 2 just below also came into my mind. > 2. pg_ctl, pg_upgrade use their own error reporting mechanisms. Yeah, for this reason I don't think that it is a good idea to switch those areas to use the parsing of option_utils.c. Perhaps we should consider switching pg_upgrade to have a better logging infra, but there are also reasons behind what we have now. pg_ctl is out of scope as it needs to cover WIN32 event logging. > 3. parameters that take real numbers doesn't fit the scheme specifying > range borders. For example boundary values may or may not be included > in the range. This concerns only pgbench, which I'd be fine to let as-is. > 4. Most of the errors are PG_LOG_ERROR, but a few ones are > PG_LOG_FATAL. I would take it that pgbench is inconsistent with the rest. Note that pg_dump uses fatal(), but that's just a wrapper to pg_log_error(). > loglevel specifies the loglevel to use to emit error messages. If it > is the newly added item PG_LOG_OMIT, the function never emits an error > message. Addition to that, the return type is changed to an enum which > indicates what trouble the given string has. The caller can print > arbitrary error messages consulting the value. (killproc in pg_ctl.c) I am not much a fan of that. If we do so, what's the point in having a dependency to logging.c anyway in option_utils.c? This OMIT option only exists to bypass the specific logging needs where this gets added. That does not seem a design adapted to me in the long term, neither am I a fan of specific error codes for a code path that's just going to be used to parse command options. > I added two more similar functions option_parse_long/double. The > former is a clone of _int. The latter doesn't perform explicit range > checks for the reason described above. These have a limited impact, so I would limit things to int32 for now. > Maybe we need to make pg_upgrade use the common-logging features > instead of its own, but it is not included in this patch. Maybe. That would be good in the long term, though its case is very particular. > The attached patch needs more polish but should be enough to tell what > I have in my mind. This breaks some of the TAP tests of pgbench and pg_dump, at short sight. The checks for the port value in pg_receivewal and pg_recvlogical is strange to have. We don't care about that in any other tools. The number of checks for --jobs and workers could be made more consistent across the board, but I have let that out for now. Hacking on that, I am finishing with the attached. It is less ambitious, still very useful as it removes a dozen of custom error messages in favor of the two ones introduced in option_utils.c. On top of that this reduces a bit the code: 15 files changed, 156 insertions(+), 169 deletions(-) What do you think? -- Michael
From 3aaf98a9e53d90ca2ac8c2734ae77487aa03843f Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 21 Jul 2021 20:42:13 +0900
Subject: [PATCH v4] Introduce and use routine for parsing of int32 options
---
src/include/fe_utils/option_utils.h | 3 ++
src/fe_utils/option_utils.c | 39 ++++++++++++++
src/bin/pg_amcheck/pg_amcheck.c | 8 ++-
src/bin/pg_basebackup/pg_basebackup.c | 17 +++---
src/bin/pg_basebackup/pg_receivewal.c | 23 +++------
src/bin/pg_basebackup/pg_recvlogical.c | 25 ++++-----
src/bin/pg_checksums/Makefile | 3 ++
src/bin/pg_checksums/pg_checksums.c | 9 ++--
src/bin/pg_dump/pg_dump.c | 41 +++++++--------
src/bin/pg_dump/pg_restore.c | 31 +++++------
src/bin/pg_dump/t/001_basic.pl | 8 +--
src/bin/pgbench/pgbench.c | 60 +++++++---------------
src/bin/pgbench/t/002_pgbench_no_server.pl | 18 +++----
src/bin/scripts/reindexdb.c | 9 ++--
src/bin/scripts/vacuumdb.c | 31 ++++-------
15 files changed, 156 insertions(+), 169 deletions(-)
diff --git a/src/include/fe_utils/option_utils.h b/src/include/fe_utils/option_utils.h
index d653cb94e3..e999d56ec0 100644
--- a/src/include/fe_utils/option_utils.h
+++ b/src/include/fe_utils/option_utils.h
@@ -19,5 +19,8 @@ typedef void (*help_handler) (const char *progname);
extern void handle_help_version_opts(int argc, char *argv[],
const char *fixed_progname,
help_handler hlp);
+extern bool option_parse_int(const char *optarg, const char *optname,
+ int min_range, int max_range,
+ int *result);
#endif /* OPTION_UTILS_H */
diff --git a/src/fe_utils/option_utils.c b/src/fe_utils/option_utils.c
index e19a495dba..4f0bc7abc5 100644
--- a/src/fe_utils/option_utils.c
+++ b/src/fe_utils/option_utils.c
@@ -12,6 +12,8 @@
#include "postgres_fe.h"
+#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/option_utils.h"
/*
@@ -36,3 +38,40 @@ handle_help_version_opts(int argc, char *argv[],
}
}
}
+
+/*
+ * option_parse_int
+ *
+ * Parse an integer for a given option. Returns true if the parsing
+ * could be done with optionally *result holding the parsed value, and
+ * false on failure.
+ */
+bool
+option_parse_int(const char *optarg, const char *optname,
+ int min_range, int max_range,
+ int *result)
+{
+ char *endptr;
+ int val;
+
+ errno = 0;
+ val = strtoint(optarg, &endptr, 10);
+
+ if (*endptr)
+ {
+ pg_log_error("invalid value \"%s\" for option \"%s\"",
+ optarg, optname);
+ return false;
+ }
+
+ if (errno == ERANGE || val < min_range || val > max_range)
+ {
+ pg_log_error("%s must be in range %d..%d",
+ optname, min_range, max_range);
+ return false;
+ }
+
+ if (result)
+ *result = val;
+ return true;
+}
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..ee8aa71bdf 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -12,6 +12,7 @@
*/
#include "postgres_fe.h"
+#include <limits.h>
#include <time.h>
#include "catalog/pg_am_d.h"
@@ -326,12 +327,9 @@ main(int argc, char *argv[])
append_btree_pattern(&opts.exclude, optarg, encoding);
break;
case 'j':
- opts.jobs = atoi(optarg);
- if (opts.jobs < 1)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX,
+ &opts.jobs))
exit(1);
- }
break;
case 'p':
port = pg_strdup(optarg);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf498..9ea98481d8 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -31,6 +31,7 @@
#include "common/file_utils.h"
#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/recovery_gen.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
@@ -2371,12 +2372,9 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
- {
- pg_log_error("invalid compression level \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
+ &compresslevel))
exit(1);
- }
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2409,12 +2407,11 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-s/--status-interval", 0,
+ INT_MAX / 1000,
+ &standby_message_timeout))
exit(1);
- }
+ standby_message_timeout *= 1000;
break;
case 'v':
verbose++;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index c1334fad35..4474273daf 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <signal.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -22,6 +23,7 @@
#include "access/xlog_internal.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "receivelog.h"
@@ -532,11 +534,6 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
- {
- pg_log_error("invalid port number \"%s\"", optarg);
- exit(1);
- }
dbport = pg_strdup(optarg);
break;
case 'U':
@@ -549,12 +546,11 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-s/--status-interval", 0,
+ INT_MAX / 1000,
+ &standby_message_timeout))
exit(1);
- }
+ standby_message_timeout *= 1000;
break;
case 'S':
replication_slot = pg_strdup(optarg);
@@ -574,12 +570,9 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
- {
- pg_log_error("invalid compression level \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
+ &compresslevel))
exit(1);
- }
break;
/* action */
case 1:
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 76bd153fac..1d59bf3744 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -13,6 +13,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <sys/stat.h>
#include <unistd.h>
#ifdef HAVE_SYS_SELECT_H
@@ -23,6 +24,7 @@
#include "common/fe_memutils.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -739,12 +741,11 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
- {
- pg_log_error("invalid fsync interval \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-F/--fsync-interval", 0,
+ INT_MAX / 1000,
+ &fsync_interval))
exit(1);
- }
+ fsync_interval *= 1000;
break;
case 'n':
noloop = 1;
@@ -763,11 +764,6 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
- {
- pg_log_error("invalid port number \"%s\"", optarg);
- exit(1);
- }
dbport = pg_strdup(optarg);
break;
case 'U':
@@ -820,12 +816,11 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-s/--status-interval", 0,
+ INT_MAX / 1000,
+ &standby_message_timeout))
exit(1);
- }
+ standby_message_timeout *= 1000;
break;
case 'S':
replication_slot = pg_strdup(optarg);
diff --git a/src/bin/pg_checksums/Makefile b/src/bin/pg_checksums/Makefile
index ba62406105..4d1f0fc3a0 100644
--- a/src/bin/pg_checksums/Makefile
+++ b/src/bin/pg_checksums/Makefile
@@ -15,6 +15,9 @@ subdir = src/bin/pg_checksums
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+# We need libpq only because fe_utils does.
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
+
OBJS = \
$(WIN32RES) \
pg_checksums.o
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3c326906e2..e833c5d75e 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +25,7 @@
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -518,11 +520,10 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
case 'f':
- if (atoi(optarg) == 0)
- {
- pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
+ if (!option_parse_int(optarg, "-f/--filenode", 0,
+ INT_MAX,
+ NULL))
exit(1);
- }
only_filenode = pstrdup(optarg);
break;
case 'N':
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 34b91bb226..c8eff0aab0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -56,6 +56,7 @@
#include "catalog/pg_type_d.h"
#include "common/connect.h"
#include "dumputils.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq/libpq-fs.h"
@@ -487,7 +488,19 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ /*
+ * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS
+ * (= 64 usually) parallel jobs because that's the maximum
+ * limit for the WaitForMultipleObjects() call.
+ */
+ if (!option_parse_int(optarg, "-j/--jobs", 0,
+#ifdef WIN32
+ MAXIMUM_WAIT_OBJECTS,
+#else
+ INT_MAX,
+#endif
+ &numWorkers))
+ exit_nicely(1);
break;
case 'n': /* include schema(s) */
@@ -550,12 +563,9 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
- {
- pg_log_error("compression level must be in range 0..9");
+ if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
+ &compressLevel))
exit_nicely(1);
- }
break;
case 0:
@@ -588,12 +598,9 @@ main(int argc, char **argv)
case 8:
have_extra_float_digits = true;
- extra_float_digits = atoi(optarg);
- if (extra_float_digits < -15 || extra_float_digits > 3)
- {
- pg_log_error("extra_float_digits must be in range -15..3");
+ if (!option_parse_int(optarg, "--extra-float-digits", -15, 3,
+ &extra_float_digits))
exit_nicely(1);
- }
break;
case 9: /* inserts */
@@ -720,18 +727,6 @@ main(int argc, char **argv)
if (!plainText)
dopt.outputCreateDB = 1;
- /*
- * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
- * parallel jobs because that's the maximum limit for the
- * WaitForMultipleObjects() call.
- */
- if (numWorkers <= 0
-#ifdef WIN32
- || numWorkers > MAXIMUM_WAIT_OBJECTS
-#endif
- )
- fatal("invalid number of parallel jobs");
-
/* Parallel backup only in the directory archive format so far */
if (archiveFormat != archDirectory && numWorkers > 1)
fatal("parallel backup only supported by the directory format");
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 589b4aed53..5ac5344be3 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -46,6 +46,7 @@
#endif
#include "dumputils.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "parallel.h"
#include "pg_backup_utils.h"
@@ -181,7 +182,19 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ /*
+ * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS
+ * (= 64 usually) parallel jobs because that's the maximum
+ * limit for the WaitForMultipleObjects() call.
+ */
+ if (!option_parse_int(optarg, "-j/--jobs", 0,
+#ifdef WIN32
+ MAXIMUM_WAIT_OBJECTS,
+#else
+ INT_MAX,
+#endif
+ &numWorkers))
+ exit(1);
break;
case 'l': /* Dump the TOC summary */
@@ -344,22 +357,6 @@ main(int argc, char **argv)
exit_nicely(1);
}
- if (numWorkers <= 0)
- {
- pg_log_error("invalid number of parallel jobs");
- exit(1);
- }
-
- /* See comments in pg_dump.c */
-#ifdef WIN32
- if (numWorkers > MAXIMUM_WAIT_OBJECTS)
- {
- pg_log_error("maximum number of parallel jobs is %d",
- MAXIMUM_WAIT_OBJECTS);
- exit(1);
- }
-#endif
-
/* Can't do single-txn mode with multiple connections */
if (opts->single_txn && numWorkers > 1)
{
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index 9f12ca6c51..ab09587d67 100644
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -103,7 +103,7 @@ command_fails_like(
command_fails_like(
[ 'pg_dump', '-j', '-1' ],
- qr/\Qpg_dump: error: invalid number of parallel jobs\E/,
+ qr/\Qpg_dump: error: -j\/--jobs must be in range 0..2147483647\E/,
'pg_dump: invalid number of parallel jobs');
command_fails_like(
@@ -113,7 +113,7 @@ command_fails_like(
command_fails_like(
[ 'pg_restore', '-j', '-1', '-f -' ],
- qr/\Qpg_restore: error: invalid number of parallel jobs\E/,
+ qr/\Qpg_restore: error: -j\/--jobs must be in range 0..2147483647\E/,
'pg_restore: invalid number of parallel jobs');
command_fails_like(
@@ -123,12 +123,12 @@ command_fails_like(
command_fails_like(
[ 'pg_dump', '-Z', '-1' ],
- qr/\Qpg_dump: error: compression level must be in range 0..9\E/,
+ qr/\Qpg_dump: error: -Z\/--compress must be in range 0..9\E/,
'pg_dump: compression level must be in range 0..9');
command_fails_like(
[ 'pg_dump', '--extra-float-digits', '-16' ],
- qr/\Qpg_dump: error: extra_float_digits must be in range -15..3\E/,
+ qr/\Qpg_dump: error: --extra-float-digits must be in range -15..3\E/,
'pg_dump: extra_float_digits must be in range -15..3');
command_fails_like(
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..c51ebb8e31 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -63,6 +63,7 @@
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -5887,10 +5888,9 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
+ if (!option_parse_int(optarg, "-c/--clients", 1, INT_MAX,
+ &nclients))
{
- pg_log_fatal("invalid number of clients: \"%s\"", optarg);
exit(1);
}
#ifdef HAVE_GETRLIMIT
@@ -5914,10 +5914,9 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX,
+ &nthreads))
{
- pg_log_fatal("invalid number of threads: \"%s\"", optarg);
exit(1);
}
#ifndef ENABLE_THREAD_SAFETY
@@ -5938,30 +5937,21 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
- {
- pg_log_fatal("invalid scaling factor: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-s/--scale", 1, INT_MAX,
+ &scale))
exit(1);
- }
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
- {
- pg_log_fatal("invalid number of transactions: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-t/--transactions", 1, INT_MAX,
+ &nxacts))
exit(1);
- }
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
- {
- pg_log_fatal("invalid duration: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-T/--time", 1, INT_MAX,
+ &duration))
exit(1);
- }
break;
case 'U':
username = pg_strdup(optarg);
@@ -6019,12 +6009,9 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
- {
- pg_log_fatal("invalid fillfactor: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-F/--fillfactor", 10, 100,
+ &fillfactor))
exit(1);
- }
break;
case 'M':
benchmarking_option_set = true;
@@ -6039,12 +6026,9 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
- {
- pg_log_fatal("invalid thread progress delay: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "-P/--progress", 1, INT_MAX,
+ &progress))
exit(1);
- }
break;
case 'R':
{
@@ -6098,12 +6082,9 @@ main(int argc, char **argv)
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
- {
- pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "--aggregate-interval", 1, INT_MAX,
+ &agg_interval))
exit(1);
- }
break;
case 6: /* progress-timestamp */
progress_timestamp = true;
@@ -6135,12 +6116,9 @@ main(int argc, char **argv)
break;
case 11: /* partitions */
initialization_option_set = true;
- partitions = atoi(optarg);
- if (partitions < 0)
- {
- pg_log_fatal("invalid number of partitions: \"%s\"", optarg);
+ if (!option_parse_int(optarg, "--partitions", 1, INT_MAX,
+ &partitions))
exit(1);
- }
break;
case 12: /* partition-method */
initialization_option_set = true;
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index 346a2667fc..8fc3a5158d 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -91,17 +91,17 @@ my @options = (
[qr{weight spec.* out of range .*: -1}]
],
[ 'too many scripts', '-S ' x 129, [qr{at most 128 SQL scripts}] ],
- [ 'bad #clients', '-c three', [qr{invalid number of clients: "three"}] ],
+ [ 'bad #clients', '-c three', [qr{invalid value "three" for option "-c/--clients"}] ],
[
- 'bad #threads', '-j eleven', [qr{invalid number of threads: "eleven"}]
+ 'bad #threads', '-j eleven', [qr{invalid value "eleven" for option "-j/--jobs"}]
],
- [ 'bad scale', '-i -s two', [qr{invalid scaling factor: "two"}] ],
+ [ 'bad scale', '-i -s two', [qr{invalid value "two" for option "-s/--scale"}] ],
[
'invalid #transactions',
'-t zil',
- [qr{invalid number of transactions: "zil"}]
+ [qr{invalid value "zil" for option "-t/--transactions"}]
],
- [ 'invalid duration', '-T ten', [qr{invalid duration: "ten"}] ],
+ [ 'invalid duration', '-T ten', [qr{invalid value "ten" for option "-T/--time"}] ],
[
'-t XOR -T',
'-N -l --aggregate-interval=5 --log-prefix=notused -t 1000 -T 1',
@@ -113,11 +113,11 @@ my @options = (
[qr{specify either }]
],
[ 'bad variable', '--define foobla', [qr{invalid variable definition}] ],
- [ 'invalid fillfactor', '-F 1', [qr{invalid fillfactor}] ],
+ [ 'invalid fillfactor', '-F 1', [qr{-F/--fillfactor must be in range}] ],
[ 'invalid query mode', '-M no-such-mode', [qr{invalid query mode}] ],
[
'invalid progress', '--progress=0',
- [qr{invalid thread progress delay}]
+ [qr{-P/--progress must be in range}]
],
[ 'invalid rate', '--rate=0.0', [qr{invalid rate limit}] ],
[ 'invalid latency', '--latency-limit=0.0', [qr{invalid latency limit}] ],
@@ -127,7 +127,7 @@ my @options = (
],
[
'invalid aggregate interval', '--aggregate-interval=-3',
- [qr{invalid .* seconds for}]
+ [qr{--aggregate-interval must be in range}]
],
[
'weight zero',
@@ -171,7 +171,7 @@ my @options = (
[
'bad partition number',
'-i --partitions -1',
- [qr{invalid number of partitions: "-1"}]
+ [qr{--partitions must be in range}]
],
[
'partition method without partitioning',
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc0681538a..a0b0250c49 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -11,6 +11,8 @@
#include "postgres_fe.h"
+#include <limits.h>
+
#include "catalog/pg_class_d.h"
#include "common.h"
#include "common/connect.h"
@@ -151,12 +153,9 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX,
+ &concurrentCons))
exit(1);
- }
break;
case 'v':
verbose = true;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index a85919c5c1..f40bd14857 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -12,8 +12,9 @@
#include "postgres_fe.h"
-#include "catalog/pg_class_d.h"
+#include <limits.h>
+#include "catalog/pg_class_d.h"
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
@@ -192,20 +193,14 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX,
+ &concurrentCons))
exit(1);
- }
break;
case 'P':
- 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");
+ if (!option_parse_int(optarg, "-P/--parallel", 0, INT_MAX,
+ &vacopts.parallel_workers))
exit(1);
- }
break;
case 2:
maintenance_db = pg_strdup(optarg);
@@ -220,20 +215,14 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
- {
- pg_log_error("minimum transaction ID age must be at least 1");
+ if (!option_parse_int(optarg, "--min-xid-age", 1, INT_MAX,
+ &vacopts.min_xid_age))
exit(1);
- }
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
- {
- pg_log_error("minimum multixact ID age must be at least 1");
+ if (!option_parse_int(optarg, "--min-mxid-age", 1, INT_MAX,
+ &vacopts.min_mxid_age))
exit(1);
- }
break;
case 8:
vacopts.no_index_cleanup = true;
--
2.32.0
signature.asc
Description: PGP signature
