Thread

Commits

pgbench: increase the maximum number of variables/arguments
- a47841528107 12.0 landed

pgbench MAX_ARGS

Simon Riggs <simon@2ndquadrant.com> — 2019-02-26T09:41:02Z

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.

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.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgbench MAX_ARGS

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-02-26T12:19:50Z

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.

Re: pgbench MAX_ARGS

Simon Riggs <simon@2ndquadrant.com> — 2019-02-26T12:57:14Z

On Tue, 26 Feb 2019 at 12:19, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

>
> Hello Simon,
>

Thanks for reviewing Fabien,


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


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

I've put it as 256 args now.

The overhead of that is about 2kB, so not really an issue.

People are using pgbench for real testing, so no need for setting it too
low.

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

Added as you suggest.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgbench MAX_ARGS

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2019-02-26T14:02:39Z

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

Re: pgbench MAX_ARGS

Andres Freund <andres@anarazel.de> — 2019-02-26T17:37:58Z

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

Re: pgbench MAX_ARGS

Simon Riggs <simon@2ndquadrant.com> — 2019-02-26T17:45:04Z

On Tue, 26 Feb 2019 at 17:38, Andres Freund <andres@anarazel.de> wrote:

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

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.

> The eighties want their constants back ;)
>

Made me smile, thanks. ;-)

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgbench MAX_ARGS

Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-26T17:51:23Z

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

Re: pgbench MAX_ARGS

Andres Freund <andres@anarazel.de> — 2019-02-26T17:56:57Z

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 ;)

Re: pgbench MAX_ARGS

David Rowley <david.rowley@2ndquadrant.com> — 2019-02-28T13:20:26Z

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

Re: pgbench MAX_ARGS

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2019-03-10T23:37:30Z

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

Re: pgbench MAX_ARGS

David Rowley <david.rowley@2ndquadrant.com> — 2019-03-11T10:07:25Z

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

Re: pgbench MAX_ARGS

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-03-11T15:59:50Z

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

Re: pgbench MAX_ARGS

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2019-03-11T17:04:03Z

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

Re: pgbench MAX_ARGS

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-03-11T17:20:21Z

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