From: | ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker ) |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
Cc: | Simon Riggs <simon(at)2ndquadrant(dot)com>, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: pgbench MAX_ARGS |
Date: | 2019-03-10 23:37:30 |
Message-ID: | d8jwol65mit.fsf@dalvik.ping.uio.no |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> On Wed, 27 Feb 2019 at 01:57, Simon Riggs <simon(at)2ndquadrant(dot)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
Attachment | Content-Type | Size |
---|---|---|
0001-pgbench-increase-the-maximum-number-of-variables-arg.patch | text/x-diff | 4.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Nikita Glukhov | 2019-03-10 23:49:07 | Add missing operator <->(box, point) |
Previous Message | Dagfinn Ilmari Mannsåker | 2019-03-10 23:07:53 | Re: what makes the PL cursor life-cycle must be in the same transaction? |