Re: [HACKERS] Confusing error message in pgbench

[Fabien COELHO]

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

Re: [HACKERS] Confusing error message in pgbench

[Tatsuo Ishii]

>> 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

Re: [HACKERS] Confusing error message in pgbench

[Tatsuo Ishii]

> 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

Re: [HACKERS] Confusing error message in pgbench

[Tom Lane]

Robert Haas  writes:
> On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii  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

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(_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(_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 */

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

Re: [HACKERS] Confusing error message in pgbench

[Fabien COELHO]

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.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++)

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

Re: [HACKERS] Confusing error message in pgbench

[Robert Haas]

On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii  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

[HACKERS] Confusing error message in pgbench

[Tatsuo Ishii]

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
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++)

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