Confusing error message in pgbench

Started by Tatsuo Ishiiover 8 years ago7 messages
#1Tatsuo Ishii
ishii@sraoss.co.jp
1 attachment(s)

I found an error message in pgbench is quite confusing.

pgbench -S -M extended -c 1 -T 30 test
query mode (-M) should be specified before any transaction scripts (-f or -b)

Since there's no -f or -b option is specified, users will be
confused. Actually the error occurs because pgbench implicitly
introduces a built in script for -S. To eliminate the confusion, I
think the error message should be fixed like this:

query mode (-M) should be specified before transaction type (-S or -N) or any transaction scripts (-f or -b)

Patch attached.

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.difftext/x-patch; charset=us-asciiDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..f7ef0ed 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3898,7 +3898,7 @@ main(int argc, char **argv)
 				benchmarking_option_set = true;
 				if (num_scripts > 0)
 				{
-					fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
+					fprintf(stderr, "query mode (-M) should be specified before transaction type (-S or -N) or any transaction scripts (-f or -b)\n");
 					exit(1);
 				}
 				for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)
#2Robert Haas
robertmhaas@gmail.com
In reply to: Tatsuo Ishii (#1)
Re: Confusing error message in pgbench

On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

I found an error message in pgbench is quite confusing.

pgbench -S -M extended -c 1 -T 30 test
query mode (-M) should be specified before any transaction scripts (-f or -b)

Since there's no -f or -b option is specified, users will be
confused. Actually the error occurs because pgbench implicitly
introduces a built in script for -S. To eliminate the confusion, I
think the error message should be fixed like this:

query mode (-M) should be specified before transaction type (-S or -N) or any transaction scripts (-f or -b)

Patch attached.

Not really objecting, but an even better fix might be to remove the
restriction on the order in which the options can be specified.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Tatsuo Ishii (#1)
1 attachment(s)
Re: Confusing error message in pgbench

Hello Tatsuo-san,

I found an error message in pgbench is quite confusing.

pgbench -S -M extended -c 1 -T 30 test
query mode (-M) should be specified before any transaction scripts (-f or -b)

Since there's no -f or -b option is specified, users will be
confused.

Indeed.

Actually the error occurs because pgbench implicitly introduces a built
in script for -S. To eliminate the confusion, I think the error message
should be fixed like this:

The idea is that -S/-N documentations say that it is just a shortcut for
-b, but the explanation (eg --help) is too far away.

query mode (-M) should be specified before transaction type (-S or -N)
or any transaction scripts (-f or -b)

I would suggest to make it even shorter, see attached:

query mode (-M) should be specified before any transaction scripts (-f,
-b, -S or -N).

I'm wondering whether it could/should be "any transaction script". My
English level does not allow to decide.

--
Fabien.

Attachments:

pgbench-query-mode-error-2.patchtext/x-diff; name=pgbench-query-mode-error-2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..9e41c07 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3898,7 +3898,7 @@ main(int argc, char **argv)
 				benchmarking_option_set = true;
 				if (num_scripts > 0)
 				{
-					fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
+					fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f, -b, -S or -N)\n");
 					exit(1);
 				}
 				for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)
#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#2)
1 attachment(s)
Re: Confusing error message in pgbench

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

I found an error message in pgbench is quite confusing.

Not really objecting, but an even better fix might be to remove the
restriction on the order in which the options can be specified.

Indeed. It doesn't look that hard: AFAICS the problem is just that
process_sql_command() is making premature decisions about whether to
extract parameters from the SQL commands. Proposed patch attached.

regards, tom lane

Attachments:

remove-pgbench-option-ordering-constraint.patchtext/x-diff; charset=us-ascii; name=remove-pgbench-option-ordering-constraint.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..ae78c7b 100644
*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
*************** init(bool is_no_vacuum)
*** 2844,2858 ****
  }
  
  /*
!  * Parse the raw sql and replace :param to $n.
   */
  static bool
! parseQuery(Command *cmd, const char *raw_sql)
  {
  	char	   *sql,
  			   *p;
  
! 	sql = pg_strdup(raw_sql);
  	cmd->argc = 1;
  
  	p = sql;
--- 2844,2861 ----
  }
  
  /*
!  * Replace :param with $n throughout the command's SQL text, which
!  * is a modifiable string in cmd->argv[0].
   */
  static bool
! parseQuery(Command *cmd)
  {
  	char	   *sql,
  			   *p;
  
! 	/* We don't want to scribble on cmd->argv[0] until done */
! 	sql = pg_strdup(cmd->argv[0]);
! 
  	cmd->argc = 1;
  
  	p = sql;
*************** parseQuery(Command *cmd, const char *raw
*** 2874,2880 ****
  
  		if (cmd->argc >= MAX_ARGS)
  		{
! 			fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n", MAX_ARGS - 1, raw_sql);
  			pg_free(name);
  			return false;
  		}
--- 2877,2884 ----
  
  		if (cmd->argc >= MAX_ARGS)
  		{
! 			fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n",
! 					MAX_ARGS - 1, cmd->argv[0]);
  			pg_free(name);
  			return false;
  		}
*************** parseQuery(Command *cmd, const char *raw
*** 2886,2891 ****
--- 2890,2896 ----
  		cmd->argc++;
  	}
  
+ 	pg_free(cmd->argv[0]);
  	cmd->argv[0] = sql;
  	return true;
  }
*************** process_sql_command(PQExpBuffer buf, con
*** 2983,2992 ****
  	my_command = (Command *) pg_malloc0(sizeof(Command));
  	my_command->command_num = num_commands++;
  	my_command->type = SQL_COMMAND;
- 	my_command->argc = 0;
  	initSimpleStats(&my_command->stats);
  
  	/*
  	 * If SQL command is multi-line, we only want to save the first line as
  	 * the "line" label.
  	 */
--- 2988,3003 ----
  	my_command = (Command *) pg_malloc0(sizeof(Command));
  	my_command->command_num = num_commands++;
  	my_command->type = SQL_COMMAND;
  	initSimpleStats(&my_command->stats);
  
  	/*
+ 	 * Install query text as the sole argv string.  If we are using a
+ 	 * non-simple query mode, we'll extract parameters from it later.
+ 	 */
+ 	my_command->argv[0] = pg_strdup(p);
+ 	my_command->argc = 1;
+ 
+ 	/*
  	 * If SQL command is multi-line, we only want to save the first line as
  	 * the "line" label.
  	 */
*************** process_sql_command(PQExpBuffer buf, con
*** 3000,3020 ****
  	else
  		my_command->line = pg_strdup(p);
  
- 	switch (querymode)
- 	{
- 		case QUERY_SIMPLE:
- 			my_command->argv[0] = pg_strdup(p);
- 			my_command->argc++;
- 			break;
- 		case QUERY_EXTENDED:
- 		case QUERY_PREPARED:
- 			if (!parseQuery(my_command, p))
- 				exit(1);
- 			break;
- 		default:
- 			exit(1);
- 	}
- 
  	return my_command;
  }
  
--- 3011,3016 ----
*************** main(int argc, char **argv)
*** 3896,3906 ****
  				break;
  			case 'M':
  				benchmarking_option_set = true;
- 				if (num_scripts > 0)
- 				{
- 					fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
- 					exit(1);
- 				}
  				for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)
  					if (strcmp(optarg, QUERYMODE[querymode]) == 0)
  						break;
--- 3892,3897 ----
*************** main(int argc, char **argv)
*** 4006,4011 ****
--- 3997,4020 ----
  		internal_script_used = true;
  	}
  
+ 	/* if not simple query mode, parse the script(s) to find parameters */
+ 	if (querymode != QUERY_SIMPLE)
+ 	{
+ 		for (i = 0; i < num_scripts; i++)
+ 		{
+ 			Command   **commands = sql_script[i].commands;
+ 			int			j;
+ 
+ 			for (j = 0; commands[j] != NULL; j++)
+ 			{
+ 				if (commands[j]->type != SQL_COMMAND)
+ 					continue;
+ 				if (!parseQuery(commands[j]))
+ 					exit(1);
+ 			}
+ 		}
+ 	}
+ 
  	/* compute total_weight */
  	for (i = 0; i < num_scripts; i++)
  		/* cannot overflow: weight is 32b, total_weight 64b */
#5Tatsuo Ishii
ishii@sraoss.co.jp
In reply to: Robert Haas (#2)
Re: Confusing error message in pgbench

Not really objecting, but an even better fix might be to remove the
restriction on the order in which the options can be specified.

+100 :-)

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

#6Tatsuo Ishii
ishii@sraoss.co.jp
In reply to: Tom Lane (#4)
Re: Confusing error message in pgbench

Not really objecting, but an even better fix might be to remove the
restriction on the order in which the options can be specified.

Indeed. It doesn't look that hard: AFAICS the problem is just that
process_sql_command() is making premature decisions about whether to
extract parameters from the SQL commands. Proposed patch attached.

Great. Patch looks good to me.

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

#7Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Tatsuo Ishii (#6)
Re: Confusing error message in pgbench

Indeed. It doesn't look that hard: AFAICS the problem is just that
process_sql_command() is making premature decisions about whether to
extract parameters from the SQL commands. Proposed patch attached.

Great. Patch looks good to me.

Too me as well: code looks ok, patch applies, compiles, make check
ok, manual tests with pgbench ok.

That is one more patch about pgbench in the queue.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers