Re: [HACKERS] 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

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