Lists: | pgsql-hackers |
---|
From: | Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Verifying variable names in pgbench |
Date: | 2009-12-28 03:38:59 |
Message-ID: | 20091228123859.92F8.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
We can define variables with any names in pgbench,
but only can refer them with names that consist of [A-Za-z0-9_]+ .
It could cause confusion discussed here:
http://archives.postgresql.org/message-id/4B272833.8080500@2ndquadrant.com
The attached patch verifies variable names at definition.
$ pgbench -D var:name=value
(global): invalid variable name 'var:name'
It would help users to determine why their custom scripts failed
when they misuse "\setXXX :varname" (the colon should be removed).
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
pgbench_verify_varname_20091228.patch | application/octet-stream | 2.6 KB |
From: | Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Verifying variable names in pgbench |
Date: | 2010-01-04 01:34:42 |
Message-ID: | 20100104103442.98BA.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > The attached patch verifies variable names at definition.
> > $ pgbench -D var:name=value
> > (global): invalid variable name 'var:name'
>
> I have reviewed this patch. I think that the basic idea of rejecting
> invalid variable names is probably a good one, but I'm not totally
> happy with the implementation. In particular:
>
> 1. The code that prints the invalid variable name message seems
> bizarrely complex and inexplicable relative to the way errors are
> handled elsewhere in the code. If we really need to do this, it
> should be in its own function, not buried inside putVariable(), but
> isn't there some simpler alternative?
We can remove the complexity if we give up showing the command (arg0)
in error messages. Shall we remove it? Simplified patch attached.
> 2. I think it would be worth abstracting the actual test into a
> separate function, like isLegalVariableName().
> 3. In the department of nitpicking, I believe that the for loop test
> should be written as something like name[i] != '\0' rather than just
> name[i], for clarity.
Adjusted.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
pgbench_verify_varname_20100104.patch | application/octet-stream | 2.7 KB |
From: | Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | Re: Verifying variable names in pgbench |
Date: | 2010-01-05 03:00:51 |
Message-ID: | 20100105120050.ABF0.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> We can remove the complexity if we give up showing the command (arg0)
> in error messages. Shall we remove it? Simplified patch attached.
Here is the proposal for the arg0 issue.
I added "context" argument to putVariable(). The context is a command
name for \setXXX commands, 'option' for -D option, or 'startup' for
internal assignment in startup.
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
pgbench_verify_varname_20100105.patch | application/octet-stream | 4.7 KB |
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Verifying variable names in pgbench |
Date: | 2010-01-05 03:33:47 |
Message-ID: | 603c8f071001041933g152066cct7941721f9a498978@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Mon, Jan 4, 2010 at 10:00 PM, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>
> Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>
>> We can remove the complexity if we give up showing the command (arg0)
>> in error messages. Shall we remove it? Simplified patch attached.
>
> Here is the proposal for the arg0 issue.
> I added "context" argument to putVariable(). The context is a command
> name for \setXXX commands, 'option' for -D option, or 'startup' for
> internal assignment in startup.
What is currently done for other, similar error messages?
...Robert
From: | Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Verifying variable names in pgbench |
Date: | 2010-01-05 03:44:38 |
Message-ID: | 20100105124438.ABF4.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> What is currently done for other, similar error messages?
Current error messages are:
for commands: "<context>: out of memory"
for others : "Couldn't allocate memory for variable"
The new message is: "<context>: out of memory for variable '<name>'"
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Verifying variable names in pgbench |
Date: | 2010-01-05 03:51:10 |
Message-ID: | 603c8f071001041951r64a3a9as9672435b3a00efaf@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Mon, Jan 4, 2010 at 10:44 PM, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> What is currently done for other, similar error messages?
>
> Current error messages are:
> for commands: "<context>: out of memory"
> for others : "Couldn't allocate memory for variable"
>
> The new message is: "<context>: out of memory for variable '<name>'"
OK, I see. I think this is reasonable, then.
...Robert