Hello Tatsuo-san,
Thanks for the review. I have registered it to Aug Commit fest.
https://commitfest.postgresql.org/action/patch_view?id=1532
I'm not sure of the variable name "is_non_init_parameter_set". I would
suggest "benchmarking_option_set"?
Ok, I will replace the variable name as you suggested.
Also, to be consistent, maybe it should check that no initialization-specific
option are set when benchmarking.
Good suggesition. Here is the v2 patch.
I applied it without problem and tested it.
* It seems that -c is ignored, the atoi() line has been removed.
* Option -q is initialization specific, but not detected as such like the
other, although there is a specific detection later. I think that it would
be better to use the systematic approach, and to remove the specific
check.
* I would name the second boolean "initialization_option_set", as it is
describe like that in the documentation.
* I would suggest the following error messages:
"some options cannot be used in initialization (-i) mode\n" and
"some options cannot be used in benchmarking mode\n".
Although these messages are rough, I think that they are enough and avoid
running something unexpected, which is your purpose.
Find attached a patch which adds these changes to your current version.
--
Fabien.
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 67d7960..2f7d80e 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -2521,7 +2521,7 @@ main(int argc, char **argv)
bool scale_given = false;
bool benchmarking_option_set = false;
- bool initializing_option_set = false;
+ bool initialization_option_set = false;
CState *state; /* status of clients */
TState *threads; /* array of thread */
@@ -2610,6 +2610,7 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
+ nclients = atoi(optarg);
if (nclients <= 0 || nclients > MAXCLIENTS)
{
fprintf(stderr, "invalid number of clients: %d\n", nclients);
@@ -2695,6 +2696,7 @@ main(int argc, char **argv)
use_log = true;
break;
case 'q':
+ initialization_option_set = true;
use_quiet = true;
break;
case 'f':
@@ -2722,7 +2724,7 @@ main(int argc, char **argv)
}
break;
case 'F':
- initializing_option_set = true;
+ initialization_option_set = true;
fillfactor = atoi(optarg);
if ((fillfactor < 10) || (fillfactor > 100))
{
@@ -2776,14 +2778,14 @@ main(int argc, char **argv)
case 0:
/* This covers long options which take no argument. */
if (foreign_keys || unlogged_tables)
- initializing_option_set = true;
+ initialization_option_set = true;
break;
case 2: /* tablespace */
- initializing_option_set = true;
+ initialization_option_set = true;
tablespace = pg_strdup(optarg);
break;
case 3: /* index-tablespace */
- initializing_option_set = true;
+ initialization_option_set = true;
index_tablespace = pg_strdup(optarg);
break;
case 4:
@@ -2835,7 +2837,7 @@ main(int argc, char **argv)
{
if (benchmarking_option_set)
{
- fprintf(stderr, "some parameters cannot be used in initializing mode\n");
+ fprintf(stderr, "some options cannot be used in initialization (-i) mode\n");
exit(1);
}
@@ -2844,9 +2846,9 @@ main(int argc, char **argv)
}
else
{
- if (initializing_option_set)
+ if (initialization_option_set)
{
- fprintf(stderr, "some parameters cannot be used in benchmarking mode\n");
+ fprintf(stderr, "some options cannot be used in benchmarking mode\n");
exit(1);
}
}
@@ -2868,13 +2870,6 @@ main(int argc, char **argv)
exit(1);
}
- /* -q may be used only with -i */
- if (use_quiet && !is_init_mode)
- {
- fprintf(stderr, "quiet-logging is allowed only in initialization mode (-i)\n");
- exit(1);
- }
-
/* --sampling-rate may must not be used with --aggregate-interval */
if (sample_rate > 0.0 && agg_interval > 0)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
