Re: Enhancing pgbench parameter checking - Mailing list 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
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)