Enhancing pgbench parameter checking
Hi,
It is pretty annoying that pgbench does not check parameter which
should not be used with -i. I often type like:
pgbench -c 10 -T 300 -S -i test
and accidentally initialize pgbench database. This is pretty
uncomfortable if the database is huge since initializing huge database
takes long time. Why don't we check the case? Included is the patch to
enhance the behavior of pgbench in this regard IMO. Here is a sample
session after patching:
$ ./pgbench -c 10 -T 300 -S -i test
some parameters cannot be used in initialize mode
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
Attachments:
pgbench.patchtext/x-patch; charset=us-asciiDownload
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index c0e5e24..d7a3f57 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -2520,6 +2520,8 @@ main(int argc, char **argv)
char *filename = NULL;
bool scale_given = false;
+ bool is_non_init_param_set = false;
+
CState *state; /* status of clients */
TState *threads; /* array of thread */
@@ -2599,12 +2601,14 @@ main(int argc, char **argv)
break;
case 'S':
ttype = 1;
+ is_non_init_param_set = true;
break;
case 'N':
ttype = 2;
+ is_non_init_param_set = true;
break;
case 'c':
- nclients = atoi(optarg);
+ is_non_init_param_set = true;
if (nclients <= 0 || nclients > MAXCLIENTS)
{
fprintf(stderr, "invalid number of clients: %d\n", nclients);
@@ -2629,6 +2633,7 @@ main(int argc, char **argv)
#endif /* HAVE_GETRLIMIT */
break;
case 'j': /* jobs */
+ is_non_init_param_set = true;
nthreads = atoi(optarg);
if (nthreads <= 0)
{
@@ -2637,9 +2642,11 @@ main(int argc, char **argv)
}
break;
case 'C':
+ is_non_init_param_set = true;
is_connect = true;
break;
case 'r':
+ is_non_init_param_set = true;
is_latencies = true;
break;
case 's':
@@ -2652,6 +2659,7 @@ main(int argc, char **argv)
}
break;
case 't':
+ is_non_init_param_set = true;
if (duration > 0)
{
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both.\n");
@@ -2665,6 +2673,7 @@ main(int argc, char **argv)
}
break;
case 'T':
+ is_non_init_param_set = true;
if (nxacts > 0)
{
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both.\n");
@@ -2681,12 +2690,14 @@ main(int argc, char **argv)
login = pg_strdup(optarg);
break;
case 'l':
+ is_non_init_param_set = true;
use_log = true;
break;
case 'q':
use_quiet = true;
break;
case 'f':
+ is_non_init_param_set = true;
ttype = 3;
filename = pg_strdup(optarg);
if (process_file(filename) == false || *sql_files[num_files - 1] == NULL)
@@ -2696,6 +2707,8 @@ main(int argc, char **argv)
{
char *p;
+ is_non_init_param_set = true;
+
if ((p = strchr(optarg, '=')) == NULL || p == optarg || *(p + 1) == '\0')
{
fprintf(stderr, "invalid variable definition: %s\n", optarg);
@@ -2716,6 +2729,7 @@ main(int argc, char **argv)
}
break;
case 'M':
+ is_non_init_param_set = true;
if (num_files > 0)
{
fprintf(stderr, "query mode (-M) should be specifiled before transaction scripts (-f)\n");
@@ -2731,6 +2745,7 @@ main(int argc, char **argv)
}
break;
case 'P':
+ is_non_init_param_set = true;
progress = atoi(optarg);
if (progress <= 0)
{
@@ -2745,6 +2760,8 @@ main(int argc, char **argv)
/* get a double from the beginning of option value */
double throttle_value = atof(optarg);
+ is_non_init_param_set = true;
+
if (throttle_value <= 0.0)
{
fprintf(stderr, "invalid rate limit: %s\n", optarg);
@@ -2764,6 +2781,7 @@ main(int argc, char **argv)
index_tablespace = pg_strdup(optarg);
break;
case 4:
+ is_non_init_param_set = true;
sample_rate = atof(optarg);
if (sample_rate <= 0.0 || sample_rate > 1.0)
{
@@ -2776,6 +2794,7 @@ main(int argc, char **argv)
fprintf(stderr, "--aggregate-interval is not currently supported on Windows");
exit(1);
#else
+ is_non_init_param_set = true;
agg_interval = atoi(optarg);
if (agg_interval <= 0)
{
@@ -2808,6 +2827,12 @@ main(int argc, char **argv)
if (is_init_mode)
{
+ if (is_non_init_param_set)
+ {
+ fprintf(stderr, "some parameters cannot be used in initialize mode\n");
+ exit(1);
+ }
+
init(is_no_vacuum);
exit(0);
}
Included is the patch to enhance the behavior of pgbench in this regard
IMO. Here is a sample session after patching:$ ./pgbench -c 10 -T 300 -S -i test
some parameters cannot be used in initialize mode
I have not tested, but the patch looks ok in principle.
I'm not sure of the variable name "is_non_init_parameter_set". I would
suggest "benchmarking_option_set"?
Also, to be consistent, maybe it should check that no
initialization-specific option are set when benchmarking.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabien,
I have not tested, but the patch looks ok in principle.
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.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
Attachments:
pgbench-v2.patchtext/x-patch; charset=us-asciiDownload
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index c0e5e24..67d7960 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -2520,6 +2520,9 @@ main(int argc, char **argv)
char *filename = NULL;
bool scale_given = false;
+ bool benchmarking_option_set = false;
+ bool initializing_option_set = false;
+
CState *state; /* status of clients */
TState *threads; /* array of thread */
@@ -2599,12 +2602,14 @@ main(int argc, char **argv)
break;
case 'S':
ttype = 1;
+ benchmarking_option_set = true;
break;
case 'N':
ttype = 2;
+ benchmarking_option_set = true;
break;
case 'c':
- nclients = atoi(optarg);
+ benchmarking_option_set = true;
if (nclients <= 0 || nclients > MAXCLIENTS)
{
fprintf(stderr, "invalid number of clients: %d\n", nclients);
@@ -2629,6 +2634,7 @@ main(int argc, char **argv)
#endif /* HAVE_GETRLIMIT */
break;
case 'j': /* jobs */
+ benchmarking_option_set = true;
nthreads = atoi(optarg);
if (nthreads <= 0)
{
@@ -2637,9 +2643,11 @@ main(int argc, char **argv)
}
break;
case 'C':
+ benchmarking_option_set = true;
is_connect = true;
break;
case 'r':
+ benchmarking_option_set = true;
is_latencies = true;
break;
case 's':
@@ -2652,6 +2660,7 @@ main(int argc, char **argv)
}
break;
case 't':
+ benchmarking_option_set = true;
if (duration > 0)
{
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both.\n");
@@ -2665,6 +2674,7 @@ main(int argc, char **argv)
}
break;
case 'T':
+ benchmarking_option_set = true;
if (nxacts > 0)
{
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both.\n");
@@ -2681,12 +2691,14 @@ main(int argc, char **argv)
login = pg_strdup(optarg);
break;
case 'l':
+ benchmarking_option_set = true;
use_log = true;
break;
case 'q':
use_quiet = true;
break;
case 'f':
+ benchmarking_option_set = true;
ttype = 3;
filename = pg_strdup(optarg);
if (process_file(filename) == false || *sql_files[num_files - 1] == NULL)
@@ -2696,6 +2708,8 @@ main(int argc, char **argv)
{
char *p;
+ benchmarking_option_set = true;
+
if ((p = strchr(optarg, '=')) == NULL || p == optarg || *(p + 1) == '\0')
{
fprintf(stderr, "invalid variable definition: %s\n", optarg);
@@ -2708,6 +2722,7 @@ main(int argc, char **argv)
}
break;
case 'F':
+ initializing_option_set = true;
fillfactor = atoi(optarg);
if ((fillfactor < 10) || (fillfactor > 100))
{
@@ -2716,6 +2731,7 @@ main(int argc, char **argv)
}
break;
case 'M':
+ benchmarking_option_set = true;
if (num_files > 0)
{
fprintf(stderr, "query mode (-M) should be specifiled before transaction scripts (-f)\n");
@@ -2731,6 +2747,7 @@ main(int argc, char **argv)
}
break;
case 'P':
+ benchmarking_option_set = true;
progress = atoi(optarg);
if (progress <= 0)
{
@@ -2745,6 +2762,8 @@ main(int argc, char **argv)
/* get a double from the beginning of option value */
double throttle_value = atof(optarg);
+ benchmarking_option_set = true;
+
if (throttle_value <= 0.0)
{
fprintf(stderr, "invalid rate limit: %s\n", optarg);
@@ -2756,14 +2775,19 @@ main(int argc, char **argv)
break;
case 0:
/* This covers long options which take no argument. */
+ if (foreign_keys || unlogged_tables)
+ initializing_option_set = true;
break;
case 2: /* tablespace */
+ initializing_option_set = true;
tablespace = pg_strdup(optarg);
break;
case 3: /* index-tablespace */
+ initializing_option_set = true;
index_tablespace = pg_strdup(optarg);
break;
case 4:
+ benchmarking_option_set = true;
sample_rate = atof(optarg);
if (sample_rate <= 0.0 || sample_rate > 1.0)
{
@@ -2776,6 +2800,7 @@ main(int argc, char **argv)
fprintf(stderr, "--aggregate-interval is not currently supported on Windows");
exit(1);
#else
+ benchmarking_option_set = true;
agg_interval = atoi(optarg);
if (agg_interval <= 0)
{
@@ -2808,9 +2833,23 @@ main(int argc, char **argv)
if (is_init_mode)
{
+ if (benchmarking_option_set)
+ {
+ fprintf(stderr, "some parameters cannot be used in initializing mode\n");
+ exit(1);
+ }
+
init(is_no_vacuum);
exit(0);
}
+ else
+ {
+ if (initializing_option_set)
+ {
+ fprintf(stderr, "some parameters cannot be used in benchmarking mode\n");
+ exit(1);
+ }
+ }
/* Use DEFAULT_NXACTS if neither nxacts nor duration is specified. */
if (nxacts <= 0 && duration <= 0)
Hello Tatsuo-san,
Thanks for the review. I have registered it to Aug Commit fest.
https://commitfest.postgresql.org/action/patch_view?id=1532I'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.
Attachments:
pgbench-v2+.patchtext/x-diff; name=pgbench-v2+.patchDownload
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)
{
Fabien,
Hello Tatsuo-san,
Thanks for the review. I have registered it to Aug Commit fest.
https://commitfest.postgresql.org/action/patch_view?id=1532I'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.
Thank you for the review and patch. Looks good. I changed the status
to "Ready for Committer". I will wait for a fewd days and if there's
no objection, I will commit the patch.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabien,
Find attached a patch which adds these changes to your current
version.Thank you for the review and patch. Looks good. I changed the status
to "Ready for Committer". I will wait for a fewd days and if there's
no objection, I will commit the patch.
I have committed the patch. Thanks!
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers