Thread: 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 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 ratelimit: %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 supportedon 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.
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 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 ratelimit: %s\n", optarg); @@ -2756,14 +2775,19 @@ main(int argc, char **argv) break; case 0: /* This coverslong 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 supportedon 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=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 optionscannot 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.
Fabien, > 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. 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
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