David Rowley <david.row...@2ndquadrant.com> writes:
> On Wed, 27 Feb 2019 at 01:57, Simon Riggs <si...@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?= <ilm...@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
