Thread: pgbench MAX_ARGS

Hello Simon,

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

Fine with me on principle.

Patch applies cleanly, compiles.

However, 3 TAP tests fails on the "too many argument" test case, what a 
surprise:-)

Probably I would have chosen a smaller value, eg 32 or 64, but I have not 
seen your use case.

> While there, correct an off by one error in the error message when you hit
> the limit. The highest argument id is MAX_ARGS - 1, but the max number of
> arguments is MAX_ARGS.

Argh! Indeed.

I notice that there is no documentation update, which just shows that 
indeed there are no documentation about the number of arguments, maybe the 
patch could add a sentence somewhere? There is no limit discussed in the 
PREPARE documentation, I tested up to 20. I'd sugggest to add something at 
the end of the paragraph about variable substitution in the "Custom 
Script" section, eg "A maximum of XX variable references can appear within 
an SQL command.".

-- 
Fabien.

Simon Riggs <simon@2ndquadrant.com> writes:

> diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
> index ad15ba66ea..2e4957c09a 100644
> --- a/src/bin/pgbench/t/001_pgbench_with_server.pl
> +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
> @@ -587,10 +587,19 @@ 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.*\b256\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(
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i, :i,
:i,:i, :i, :i, :i, :i,
 
> +:i);
>  }
>      ],

Instead of that wall of :i's, would it not be clearer to use the
repetition operator?

      [
-        '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.*\b256\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 257).q{);
 }
     ],

- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

Hi,

On 2019-02-26 12:57:14 +0000, Simon Riggs wrote:
> On Tue, 26 Feb 2019 at 12:19, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> I've put it as 256 args now.
> 
> The overhead of that is about 2kB, so not really an issue.

Why not just allocate it dynamically? Seems weird to have all these
MAX_ARGS, MAX_SCRIPTS ... commands. The eighties want their constants
back ;)

Greetings,

Andres Freund

Simon Riggs <simon@2ndquadrant.com> writes:
> On Tue, 26 Feb 2019 at 17:38, Andres Freund <andres@anarazel.de> wrote:
>> Why not just allocate it dynamically? Seems weird to have all these
>> MAX_ARGS, MAX_SCRIPTS ... commands.

> For me, its a few minutes work to correct a problem and report to the
> community.
> Dynamic allocation, run-time errors is all getting too time consuming for a
> small thing.

FWIW, I agree --- that's moving the goalposts further than seems
justified.

            regards, tom lane

On 2019-02-26 12:51:23 -0500, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > On Tue, 26 Feb 2019 at 17:38, Andres Freund <andres@anarazel.de> wrote:
> >> Why not just allocate it dynamically? Seems weird to have all these
> >> MAX_ARGS, MAX_SCRIPTS ... commands.
> 
> > For me, its a few minutes work to correct a problem and report to the
> > community.
> > Dynamic allocation, run-time errors is all getting too time consuming for a
> > small thing.
> 
> FWIW, I agree --- that's moving the goalposts further than seems
> justified.

I'm fine with applying a patch to just adjust the constant, but I'd also
appreciate somebody just being motivated by my message to remove the
constants ;)

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.

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

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.

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.

Setting to WoA

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

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

On Mon, 11 Mar 2019 at 12:37, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> David Rowley <david.rowley@2ndquadrant.com> writes:
> > 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.

> Setting back to NR.

The patch looks good to me. I'm happy for it to be marked as ready for
committer.  Fabien, do you want to have another look?

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 3/11/19 6:07 AM, David Rowley wrote:
> On Mon, 11 Mar 2019 at 12:37, Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
>> David Rowley <david.rowley@2ndquadrant.com> writes:
>>> 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.
>> Setting back to NR.
> The patch looks good to me. I'm happy for it to be marked as ready for
> committer.  Fabien, do you want to have another look?
>



I think we've spent enough time on this. Committed with minor changes.


cheers


andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

> I think we've spent enough time on this. Committed with minor changes.

Thanks for committing it. However, I can't see it in git. Did you forget
to push?

> cheers
>
>
> andrew

- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

On 3/11/19 1:04 PM, Dagfinn Ilmari Mannsåker wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>
>> I think we've spent enough time on this. Committed with minor changes.
> Thanks for committing it. However, I can't see it in git. Did you forget
> to push?
>
>


Ooops, yes, done now.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services