Hi all, While looking at the code areas of $subject, I got surprised about a couple of things: - pgbench has its own parsing routines for int64 and double, with an option to skip errors. That's not surprising in itself, but, for strtodouble(), errorOK is always true, meaning that the error strings are dead. For strtoint64(), errorOK is false only when parsing a Variable, where a second error string is generated. I don't really think that we need to be that pedantic about the type of errors generated in those code paths when failing to parse a variable, so I'd like to propose a simplification of the code where we reuse the same error message as for double, cutting a bit the number of translatable strings. - pgbench and pg_verifybackup make use of pg_log_fatal() to report some failures in code paths dedicated to command-line options, which is inconsistent with all the other tools that use pg_log_error().
Please find attached a patch to clean up all those inconsistencies. Thoughts? -- Michael
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index f5ebd57a47..5f0469373f 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -261,7 +261,7 @@ main(int argc, char **argv)
/* Get backup directory name */
if (optind >= argc)
{
- pg_log_fatal("no backup directory specified");
+ pg_log_error("no backup directory specified");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
exit(1);
@@ -272,7 +272,7 @@ main(int argc, char **argv)
/* Complain if any arguments remain */
if (optind < argc)
{
- pg_log_fatal("too many command-line arguments (first is \"%s\")",
+ pg_log_error("too many command-line arguments (first is \"%s\")",
argv[optind]);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
diff --git a/src/bin/pgbench/exprscan.l b/src/bin/pgbench/exprscan.l
index 75432cedc6..b3322f01f7 100644
--- a/src/bin/pgbench/exprscan.l
+++ b/src/bin/pgbench/exprscan.l
@@ -205,19 +205,19 @@ notnull [Nn][Oo][Tt][Nn][Uu][Ll][Ll]
return MAXINT_PLUS_ONE_CONST;
}
{digit}+ {
- if (!strtoint64(yytext, true, &yylval->ival))
+ if (!strtoint64(yytext, &yylval->ival))
expr_yyerror_more(yyscanner, "bigint constant overflow",
strdup(yytext));
return INTEGER_CONST;
}
{digit}+(\.{digit}*)?([eE][-+]?{digit}+)? {
- if (!strtodouble(yytext, true, &yylval->dval))
+ if (!strtodouble(yytext, &yylval->dval))
expr_yyerror_more(yyscanner, "double constant overflow",
strdup(yytext));
return DOUBLE_CONST;
}
\.{digit}+([eE][-+]?{digit}+)? {
- if (!strtodouble(yytext, true, &yylval->dval))
+ if (!strtodouble(yytext, &yylval->dval))
expr_yyerror_more(yyscanner, "double constant overflow",
strdup(yytext));
return DOUBLE_CONST;
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index c51ebb8e31..87f3b9f3b6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -803,7 +803,7 @@ is_an_int(const char *str)
* If not errorOK, an error message is also printed out on errors.
*/
bool
-strtoint64(const char *str, bool errorOK, int64 *result)
+strtoint64(const char *str, int64 *result)
{
const char *ptr = str;
int64 tmp = 0;
@@ -862,19 +862,15 @@ strtoint64(const char *str, bool errorOK, int64 *result)
return true;
out_of_range:
- if (!errorOK)
- pg_log_error("value \"%s\" is out of range for type bigint", str);
return false;
invalid_syntax:
- if (!errorOK)
- pg_log_error("invalid input syntax for type bigint: \"%s\"", str);
return false;
}
/* convert string to double, detecting overflows/underflows */
bool
-strtodouble(const char *str, bool errorOK, double *dv)
+strtodouble(const char *str, double *dv)
{
char *end;
@@ -882,18 +878,9 @@ strtodouble(const char *str, bool errorOK, double *dv)
*dv = strtod(str, &end);
if (unlikely(errno != 0))
- {
- if (!errorOK)
- pg_log_error("value \"%s\" is out of range for type double", str);
return false;
- }
-
if (unlikely(end == str || *end != '\0'))
- {
- if (!errorOK)
- pg_log_error("invalid input syntax for type double: \"%s\"", str);
return false;
- }
return true;
}
@@ -1525,16 +1512,19 @@ makeVariableValue(Variable *var)
/* if it looks like an int, it must be an int without overflow */
int64 iv;
- if (!strtoint64(var->svalue, false, &iv))
+ if (!strtoint64(var->svalue, &iv))
+ {
+ pg_log_error("malformed variable \"%s\" value: \"%s\"",
+ var->name, var->svalue);
return false;
-
+ }
setIntValue(&var->value, iv);
}
else /* type should be double */
{
double dv;
- if (!strtodouble(var->svalue, true, &dv))
+ if (!strtodouble(var->svalue, &dv))
{
pg_log_error("malformed variable \"%s\" value: \"%s\"",
var->name, var->svalue);
@@ -5900,12 +5890,12 @@ main(int argc, char **argv)
if (getrlimit(RLIMIT_OFILE, &rlim) == -1)
#endif /* RLIMIT_NOFILE */
{
- pg_log_fatal("getrlimit failed: %m");
+ pg_log_error("getrlimit failed: %m");
exit(1);
}
if (rlim.rlim_cur < nclients + 3)
{
- pg_log_fatal("need at least %d open files, but system limit is %ld",
+ pg_log_error("need at least %d open files, but system limit is %ld",
nclients + 3, (long) rlim.rlim_cur);
pg_log_info("Reduce number of clients, or use limit/ulimit to increase the system limit.");
exit(1);
@@ -5922,7 +5912,7 @@ main(int argc, char **argv)
#ifndef ENABLE_THREAD_SAFETY
if (nthreads != 1)
{
- pg_log_fatal("threads are not supported on this platform; use -j1");
+ pg_log_error("threads are not supported on this platform; use -j1");
exit(1);
}
#endif /* !ENABLE_THREAD_SAFETY */
@@ -5998,7 +5988,7 @@ main(int argc, char **argv)
if ((p = strchr(optarg, '=')) == NULL || p == optarg || *(p + 1) == '\0')
{
- pg_log_fatal("invalid variable definition: \"%s\"", optarg);
+ pg_log_error("invalid variable definition: \"%s\"", optarg);
exit(1);
}
@@ -6020,7 +6010,7 @@ main(int argc, char **argv)
break;
if (querymode >= NUM_QUERYMODE)
{
- pg_log_fatal("invalid query mode (-M): \"%s\"", optarg);
+ pg_log_error("invalid query mode (-M): \"%s\"", optarg);
exit(1);
}
break;
@@ -6039,7 +6029,7 @@ main(int argc, char **argv)
if (throttle_value <= 0.0)
{
- pg_log_fatal("invalid rate limit: \"%s\"", optarg);
+ pg_log_error("invalid rate limit: \"%s\"", optarg);
exit(1);
}
/* Invert rate limit into per-transaction delay in usec */
@@ -6052,7 +6042,7 @@ main(int argc, char **argv)
if (limit_ms <= 0.0)
{
- pg_log_fatal("invalid latency limit: \"%s\"", optarg);
+ pg_log_error("invalid latency limit: \"%s\"", optarg);
exit(1);
}
benchmarking_option_set = true;
@@ -6076,7 +6066,7 @@ main(int argc, char **argv)
sample_rate = atof(optarg);
if (sample_rate <= 0.0 || sample_rate > 1.0)
{
- pg_log_fatal("invalid sampling rate: \"%s\"", optarg);
+ pg_log_error("invalid sampling rate: \"%s\"", optarg);
exit(1);
}
break;
@@ -6102,7 +6092,7 @@ main(int argc, char **argv)
benchmarking_option_set = true;
if (!set_random_seed(optarg))
{
- pg_log_fatal("error while setting random seed from --random-seed option");
+ pg_log_error("error while setting random seed from --random-seed option");
exit(1);
}
break;
@@ -6128,7 +6118,7 @@ main(int argc, char **argv)
partition_method = PART_HASH;
else
{
- pg_log_fatal("invalid partition method, expecting \"range\" or \"hash\", got: \"%s\"",
+ pg_log_error("invalid partition method, expecting \"range\" or \"hash\", got: \"%s\"",
optarg);
exit(1);
}
@@ -6163,7 +6153,7 @@ main(int argc, char **argv)
if (total_weight == 0 && !is_init_mode)
{
- pg_log_fatal("total script weight must not be zero");
+ pg_log_error("total script weight must not be zero");
exit(1);
}
@@ -6200,7 +6190,7 @@ main(int argc, char **argv)
if (optind < argc)
{
- pg_log_fatal("too many command-line arguments (first is \"%s\")",
+ pg_log_error("too many command-line arguments (first is \"%s\")",
argv[optind]);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -6210,13 +6200,13 @@ main(int argc, char **argv)
{
if (benchmarking_option_set)
{
- pg_log_fatal("some of the specified options cannot be used in initialization (-i) mode");
+ pg_log_error("some of the specified options cannot be used in initialization (-i) mode");
exit(1);
}
if (partitions == 0 && partition_method != PART_NONE)
{
- pg_log_fatal("--partition-method requires greater than zero --partitions");
+ pg_log_error("--partition-method requires greater than zero --partitions");
exit(1);
}
@@ -6255,14 +6245,14 @@ main(int argc, char **argv)
{
if (initialization_option_set)
{
- pg_log_fatal("some of the specified options cannot be used in benchmarking mode");
+ pg_log_error("some of the specified options cannot be used in benchmarking mode");
exit(1);
}
}
if (nxacts > 0 && duration > 0)
{
- pg_log_fatal("specify either a number of transactions (-t) or a duration (-T), not both");
+ pg_log_error("specify either a number of transactions (-t) or a duration (-T), not both");
exit(1);
}
@@ -6273,44 +6263,44 @@ main(int argc, char **argv)
/* --sampling-rate may be used only with -l */
if (sample_rate > 0.0 && !use_log)
{
- pg_log_fatal("log sampling (--sampling-rate) is allowed only when logging transactions (-l)");
+ pg_log_error("log sampling (--sampling-rate) is allowed only when logging transactions (-l)");
exit(1);
}
/* --sampling-rate may not be used with --aggregate-interval */
if (sample_rate > 0.0 && agg_interval > 0)
{
- pg_log_fatal("log sampling (--sampling-rate) and aggregation (--aggregate-interval) cannot be used at the same time");
+ pg_log_error("log sampling (--sampling-rate) and aggregation (--aggregate-interval) cannot be used at the same time");
exit(1);
}
if (agg_interval > 0 && !use_log)
{
- pg_log_fatal("log aggregation is allowed only when actually logging transactions");
+ pg_log_error("log aggregation is allowed only when actually logging transactions");
exit(1);
}
if (!use_log && logfile_prefix)
{
- pg_log_fatal("log file prefix (--log-prefix) is allowed only when logging transactions (-l)");
+ pg_log_error("log file prefix (--log-prefix) is allowed only when logging transactions (-l)");
exit(1);
}
if (duration > 0 && agg_interval > duration)
{
- pg_log_fatal("number of seconds for aggregation (%d) must not be higher than test duration (%d)", agg_interval, duration);
+ pg_log_error("number of seconds for aggregation (%d) must not be higher than test duration (%d)", agg_interval, duration);
exit(1);
}
if (duration > 0 && agg_interval > 0 && duration % agg_interval != 0)
{
- pg_log_fatal("duration (%d) must be a multiple of aggregation interval (%d)", duration, agg_interval);
+ pg_log_error("duration (%d) must be a multiple of aggregation interval (%d)", duration, agg_interval);
exit(1);
}
if (progress_timestamp && progress == 0)
{
- pg_log_fatal("--progress-timestamp is allowed only under --progress");
+ pg_log_error("--progress-timestamp is allowed only under --progress");
exit(1);
}
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index 6ce1c98649..82adde9b2c 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -161,7 +161,7 @@ extern void syntax_error(const char *source, int lineno, const char *line,
const char *cmd, const char *msg,
const char *more, int col) pg_attribute_noreturn();
-extern bool strtoint64(const char *str, bool errorOK, int64 *pi);
-extern bool strtodouble(const char *str, bool errorOK, double *pd);
+extern bool strtoint64(const char *str, int64 *pi);
+extern bool strtodouble(const char *str, double *pd);
#endif /* PGBENCH_H */
signature.asc
Description: PGP signature
