Re: pgbench MAX_ARGS - Mailing list pgsql-hackers

David Rowley <david.rowley@2ndquadrant.com> writes:

> On Wed, 27 Feb 2019 at 01:57, Simon Riggs <simon@2ndquadrant.com> wrote:
>> I've put it as 256 args now.
>
> I had a look at this and I see you've added some docs to mention the
> number of parameters that are allowed; good.
>
> +   <application>pgbench</application> supports up to 256 variables in one
> +   statement.
>
> However, the code does not allow 256 variables as the documents claim.
> Per >= in:
>
>   if (cmd->argc >= MAX_ARGS)
>   {
>   fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n",
>
> For it to be 256 that would have to be > MAX_ARGS.

Which would overflow 'char *argv[MAX_ARGS];'.

> I also don't agree with this change:
>
> - MAX_ARGS - 1, cmd->lines.data);
> + MAX_ARGS, cmd->lines.data);
>
> The 0th element of the argv array was for the sql, per:
>
> cmd->argv[0] = sql;
>
> then the 9 others were for the variables, so the MAX_ARGS - 1 was
> correct originally.

The same goes for backslash commands, which use argv[] for each argument
word in the comand, and argv[0] for the command word itself.

> I think some comments in the area to explain the 0th is for the sql
> would be a good idea too, that might stop any confusion in the
> future. I see that's documented in the struct header comment, but
> maybe worth a small note around that error message just to confirm the
> - 1 is not a mistake, and neither is the >= MAX_ARGS.

I have done this in the updated version of the patch, attached.

> Probably it's fine to define MAX_ARGS to 256 then put back the
> MAX_ARGS - 1 code so that we complain if we get more than 255....
> unless 256 is really needed, of course, in which case MAX_ARGS will
> need to be 257.

I've kept it at 256, and adjusted the docs to say 255.

> The test also seems to test that 256 variables in a statement gives an
> error. That contradicts the documents that have been added, which say
> 256 is the maximum allowed.

I've adjusted the test (and the \shell test) to check for 256 variables
(arguments) exactly, and manually verified that it doesn't error on 255.

> Setting to WoA

Setting back to NR.

- ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."                              -- Skud's Meta-Law

From 5d14c9c6ee9ba5c0a67ce7baf127704803d87a42 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Sun, 10 Mar 2019 23:20:32 +0000
Subject: [PATCH] pgbench: increase the maximum number of variables/arguments

pgbench has a strange restriction to only allow 10 arguments, which is
too low for some real world uses.

Since MAX_ARGS is only used for an array of pointers and an array of
integers increasing this should not be a problem, so increase value to 256.

Add coments to clarify that MAX_ARGS includes the SQL statement or
backslash metacommand, respectively, since this caused some confusion as
to whether there was an off-by-one error in the error checking and
message.
---
 doc/src/sgml/ref/pgbench.sgml                |  2 ++
 src/bin/pgbench/pgbench.c                    | 15 ++++++++++-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 28 ++++----------------
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 9d18524834..e1ab73e582 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -916,6 +916,8 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
    value can be inserted into a SQL command by writing
    <literal>:</literal><replaceable>variablename</replaceable>.  When running more than
    one client session, each session has its own set of variables.
+   <application>pgbench</application> supports up to 255 variables in one
+   statement.
   </para>
 
    <table id="pgbench-automatic-variables">
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 5df54a8e57..4789ab92ee 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -476,7 +476,12 @@ typedef struct
  */
 #define SQL_COMMAND        1
 #define META_COMMAND    2
-#define MAX_ARGS        10
+
+/*
+ * max number of backslash command arguments or SQL variables,
+ * including the command or SQL statement itself
+ */
+#define MAX_ARGS        256
 
 typedef enum MetaCommand
 {
@@ -4124,6 +4129,10 @@ parseQuery(Command *cmd)
             continue;
         }
 
+        /*
+         * cmd->argv[0] is the SQL statement itself, so the max number of
+         * arguments is one less than MAX_ARGS
+         */
         if (cmd->argc >= MAX_ARGS)
         {
             fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n",
@@ -4461,6 +4470,10 @@ process_backslash_command(PsqlScanState sstate, const char *source)
     /* For all other commands, collect remaining words. */
     while (expr_lex_one_word(sstate, &word_buf, &word_offset))
     {
+        /*
+         * my_command->argv[0] is the command itself, so the max number of
+         * arguments is one less than MAX_ARGS
+         */
         if (j >= MAX_ARGS)
             syntax_error(source, lineno, my_command->first_line, my_command->argv[0],
                          "too many arguments", NULL, -1);
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 930ff4ebb9..6b586210a2 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -597,11 +597,10 @@ my @errors = (
 }
     ],
     [
-        'sql too many args', 1, [qr{statement has too many arguments.*\b9\b}],
-        q{-- MAX_ARGS=10 for prepared
+        'sql too many args', 1, [qr{statement has too many arguments.*\b255\b}],
+        q{-- MAX_ARGS=256 for prepared
 \set i 0
-SELECT LEAST(:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i);
-}
+SELECT LEAST(}.join(', ', (':i') x 256).q{)}
     ],
 
     # SHELL
@@ -619,25 +618,8 @@ SELECT LEAST(:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i);
     [ 'shell missing command', 1, [qr{missing command }], q{\shell} ],
     [
         'shell too many args', 1, [qr{too many arguments in command "shell"}],
-        q{-- 257 arguments to \shell
-\shell echo \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F \
- 0 1 2 3 4 5 6 7 8 9 A B C D E F
-}
+        q{-- 256 arguments to \shell
+\shell }.join(' ', ('echo') x 256)
     ],
 
     # SET
-- 
2.21.0