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
Attachments:
pgbench_verify_varname_20091228.patchapplication/octet-stream; name=pgbench_verify_varname_20091228.patchDownload
diff -cprN head/contrib/pgbench/pgbench.c work/contrib/pgbench/pgbench.c
*** head/contrib/pgbench/pgbench.c 2009-12-15 16:28:45.231538000 +0900
--- work/contrib/pgbench/pgbench.c 2009-12-28 12:25:08.389037115 +0900
*************** putVariable(CState *st, char *name, char
*** 451,456 ****
--- 451,482 ----
if (var == NULL)
{
Variable *newvars;
+ int i;
+
+ /*
+ * Check for the name only when declaring a new variable to avoid
+ * overhead.
+ */
+ for (i = 0; name[i]; i++)
+ {
+ if (!isalnum((unsigned char) name[i]) && name[i] != '_')
+ {
+ Command **file;
+ Command *command;
+ const char *argv0;
+
+ /* Command might not be available during startup. */
+ if ((file = sql_files[st->use_file]) != NULL &&
+ (command = file[st->state]) != NULL &&
+ command->argc > 0 && command->argv[0] != NULL)
+ argv0 = command->argv[0];
+ else
+ argv0 = "(global)";
+
+ fprintf(stderr, "%s: invalid variable name '%s'\n", argv0, name);
+ return false;
+ }
+ }
if (st->variables)
newvars = (Variable *) realloc(st->variables,
*************** putVariable(CState *st, char *name, char
*** 459,465 ****
newvars = (Variable *) malloc(sizeof(Variable));
if (newvars == NULL)
! return false;
st->variables = newvars;
--- 485,491 ----
newvars = (Variable *) malloc(sizeof(Variable));
if (newvars == NULL)
! goto out_of_memory;
st->variables = newvars;
*************** putVariable(CState *st, char *name, char
*** 493,498 ****
--- 519,528 ----
}
return true;
+
+ out_of_memory:
+ fprintf(stderr, "Couldn't allocate memory for variable\n");
+ return false;
}
static char *
*************** main(int argc, char **argv)
*** 1875,1884 ****
*p++ = '\0';
if (putVariable(&state[0], optarg, p) == false)
- {
- fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
break;
case 'F':
--- 1905,1911 ----
*************** main(int argc, char **argv)
*** 1959,1968 ****
for (j = 0; j < state[0].nvariables; j++)
{
if (putVariable(&state[i], state[0].variables[j].name, state[0].variables[j].value) == false)
- {
- fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
}
}
--- 1986,1992 ----
*************** main(int argc, char **argv)
*** 2040,2049 ****
for (i = 0; i < nclients; i++)
{
if (putVariable(&state[i], "scale", val) == false)
- {
- fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
}
--- 2064,2070 ----
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
Attachments:
pgbench_verify_varname_20100104.patchapplication/octet-stream; name=pgbench_verify_varname_20100104.patchDownload
diff -cprN head/contrib/pgbench/pgbench.c work/contrib/pgbench/pgbench.c
*** head/contrib/pgbench/pgbench.c 2010-01-04 09:10:26.638773000 +0900
--- work/contrib/pgbench/pgbench.c 2010-01-04 10:26:43.729385726 +0900
*************** getVariable(CState *st, char *name)
*** 431,436 ****
--- 431,451 ----
return NULL;
}
+ /* check whether the name consists of alphabets, numerals and underscores. */
+ static bool
+ isLegalVariableName(const char *name)
+ {
+ int i;
+
+ for (i = 0; name[i] != '\0'; i++)
+ {
+ if (!isalnum((unsigned char) name[i]) && name[i] != '_')
+ return false;
+ }
+
+ return true;
+ }
+
static int
putVariable(CState *st, char *name, char *value)
{
*************** putVariable(CState *st, char *name, char
*** 452,457 ****
--- 467,482 ----
{
Variable *newvars;
+ /*
+ * Check for the name only when declaring a new variable to avoid
+ * overhead.
+ */
+ if (!isLegalVariableName(name))
+ {
+ fprintf(stderr, "invalid variable name '%s'\n", name);
+ return false;
+ }
+
if (st->variables)
newvars = (Variable *) realloc(st->variables,
(st->nvariables + 1) * sizeof(Variable));
*************** putVariable(CState *st, char *name, char
*** 459,465 ****
newvars = (Variable *) malloc(sizeof(Variable));
if (newvars == NULL)
! return false;
st->variables = newvars;
--- 484,490 ----
newvars = (Variable *) malloc(sizeof(Variable));
if (newvars == NULL)
! goto out_of_memory;
st->variables = newvars;
*************** putVariable(CState *st, char *name, char
*** 493,498 ****
--- 518,527 ----
}
return true;
+
+ out_of_memory:
+ fprintf(stderr, "Couldn't allocate memory for variable\n");
+ return false;
}
static char *
*************** main(int argc, char **argv)
*** 1875,1884 ****
*p++ = '\0';
if (putVariable(&state[0], optarg, p) == false)
- {
- fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
break;
case 'F':
--- 1904,1910 ----
*************** main(int argc, char **argv)
*** 1959,1968 ****
for (j = 0; j < state[0].nvariables; j++)
{
if (putVariable(&state[i], state[0].variables[j].name, state[0].variables[j].value) == false)
- {
- fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
}
}
--- 1985,1991 ----
*************** main(int argc, char **argv)
*** 2040,2049 ****
for (i = 0; i < nclients; i++)
{
if (putVariable(&state[i], "scale", val) == false)
- {
- fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
}
--- 2063,2069 ----
Import Notes
Reply to msg id not found: 603c8f070912292209i788c0ddj180ff042ade85fad@mail.gmail.com
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
Attachments:
pgbench_verify_varname_20100105.patchapplication/octet-stream; name=pgbench_verify_varname_20100105.patchDownload
*** pgbench.c Tue Jan 5 11:55:39 2010
--- pgbench.c.new Tue Jan 5 11:46:02 2010
*************** getVariable(CState *st, char *name)
*** 431,438 ****
return NULL;
}
static int
! putVariable(CState *st, char *name, char *value)
{
Variable key,
*var;
--- 431,453 ----
return NULL;
}
+ /* check whether the name consists of alphabets, numerals and underscores. */
+ static bool
+ isLegalVariableName(const char *name)
+ {
+ int i;
+
+ for (i = 0; name[i] != '\0'; i++)
+ {
+ if (!isalnum((unsigned char) name[i]) && name[i] != '_')
+ return false;
+ }
+
+ return true;
+ }
+
static int
! putVariable(CState *st, const char *context, char *name, char *value)
{
Variable key,
*var;
*************** putVariable(CState *st, char *name, char
*** 452,457 ****
--- 467,482 ----
{
Variable *newvars;
+ /*
+ * Check for the name only when declaring a new variable to avoid
+ * overhead.
+ */
+ if (!isLegalVariableName(name))
+ {
+ fprintf(stderr, "%s: invalid variable name '%s'\n", context, name);
+ return false;
+ }
+
if (st->variables)
newvars = (Variable *) realloc(st->variables,
(st->nvariables + 1) * sizeof(Variable));
*************** putVariable(CState *st, char *name, char
*** 459,465 ****
newvars = (Variable *) malloc(sizeof(Variable));
if (newvars == NULL)
! return false;
st->variables = newvars;
--- 484,490 ----
newvars = (Variable *) malloc(sizeof(Variable));
if (newvars == NULL)
! goto out_of_memory;
st->variables = newvars;
*************** putVariable(CState *st, char *name, char
*** 493,498 ****
--- 518,527 ----
}
return true;
+
+ out_of_memory:
+ fprintf(stderr, "%s: out of memory for variable '%s'\n", context, name);
+ return false;
}
static char *
*************** runShellCommand(CState *st, char *variab
*** 687,697 ****
return false;
}
snprintf(res, sizeof(res), "%d", retval);
! if (!putVariable(st, variable, res))
! {
! fprintf(stderr, "%s: out of memory\n", argv[0]);
return false;
- }
#ifdef DEBUG
printf("shell parameter name: %s, value: %s\n", argv[1], res);
--- 716,723 ----
return false;
}
snprintf(res, sizeof(res), "%d", retval);
! if (!putVariable(st, "setshell", variable, res))
return false;
#ifdef DEBUG
printf("shell parameter name: %s, value: %s\n", argv[1], res);
*************** top:
*** 987,995 ****
#endif
snprintf(res, sizeof(res), "%d", getrand(min, max));
! if (putVariable(st, argv[1], res) == false)
{
- fprintf(stderr, "%s: out of memory\n", argv[0]);
st->ecnt++;
return true;
}
--- 1013,1020 ----
#endif
snprintf(res, sizeof(res), "%d", getrand(min, max));
! if (!putVariable(st, argv[0], argv[1], res))
{
st->ecnt++;
return true;
}
*************** top:
*** 1057,1065 ****
}
}
! if (putVariable(st, argv[1], res) == false)
{
- fprintf(stderr, "%s: out of memory\n", argv[0]);
st->ecnt++;
return true;
}
--- 1082,1089 ----
}
}
! if (!putVariable(st, argv[0], argv[1], res))
{
st->ecnt++;
return true;
}
*************** main(int argc, char **argv)
*** 1874,1884 ****
}
*p++ = '\0';
! if (putVariable(&state[0], optarg, p) == false)
! {
! fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
break;
case 'F':
--- 1898,1905 ----
}
*p++ = '\0';
! if (!putVariable(&state[0], "option", optarg, p))
exit(1);
}
break;
case 'F':
*************** main(int argc, char **argv)
*** 1958,1968 ****
state[i].id = i;
for (j = 0; j < state[0].nvariables; j++)
{
! if (putVariable(&state[i], state[0].variables[j].name, state[0].variables[j].value) == false)
! {
! fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
}
}
--- 1979,1986 ----
state[i].id = i;
for (j = 0; j < state[0].nvariables; j++)
{
! if (!putVariable(&state[i], "startup", state[0].variables[j].name, state[0].variables[j].value))
exit(1);
}
}
}
*************** main(int argc, char **argv)
*** 2039,2049 ****
snprintf(val, sizeof(val), "%d", scale);
for (i = 0; i < nclients; i++)
{
! if (putVariable(&state[i], "scale", val) == false)
! {
! fprintf(stderr, "Couldn't allocate memory for variable\n");
exit(1);
- }
}
}
--- 2057,2064 ----
snprintf(val, sizeof(val), "%d", scale);
for (i = 0; i < nclients; i++)
{
! if (!putVariable(&state[i], "startup", "scale", val))
exit(1);
}
}
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