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

Reply via email to