Thread: [HACKERS] Confusing error message in pgbench
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 transactionscripts (-f or -b)\n"); exit(1); } for (querymode = 0; querymode< NUM_QUERYMODE; querymode++)
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
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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 */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
> 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
>> 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
>> 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.