Thread: Verifying variable names in pgbench
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
Robert Haas <robertmhaas@gmail.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
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.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
On Mon, Jan 4, 2010 at 10:00 PM, Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> wrote: > > Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.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
Robert Haas <robertmhaas@gmail.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
On Mon, Jan 4, 2010 at 10:44 PM, Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> wrote: > Robert Haas <robertmhaas@gmail.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