[PATCH] pgbench: improve \sleep meta command
Hi.
I created a patch to improve \sleep meta command in pgbench.
There are two problems with the current pgbench implementation of
\sleep.
First, when the input is like "\sleep foo s" , this string will be
treated as 0 through atoi function, and no error will be raised.
Second, when the input is like "\sleep :some_bool s" and some_bool is
set to True or False, this bool will be treated as 0 as well.
However, I think we should catch this error, so I suggest my patch to
detect this and raise errors.
Regards.
--
Kota Miyake
Attachments:
v01_pgbench_sleep.patchtext/x-diff; name=v01_pgbench_sleep.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31a4df45f5..3f28b022d7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2860,7 +2860,18 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs)
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
}
+
+ for (int i=0; i<strlen(var); i++)
+ {
+ if(!isdigit(var[i]))
+ {
+ pg_log_error("\\sleep command argument must be an integer (not \"%s\")", var);
+ return false;
+ }
+ }
+
usec = atoi(var);
+
}
else
usec = atoi(argv[1]);
@@ -4647,7 +4658,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
* will be parsed with atoi, which ignores trailing non-digit
* characters.
*/
- if (my_command->argc == 2 && my_command->argv[1][0] != ':')
+ if (my_command->argv[1][0] != ':')
{
char *c = my_command->argv[1];
@@ -4655,9 +4666,18 @@ process_backslash_command(PsqlScanState sstate, const char *source)
c++;
if (*c)
{
- my_command->argv[2] = c;
- offsets[2] = offsets[1] + (c - my_command->argv[1]);
- my_command->argc = 3;
+ if (my_command->argc ==2)
+ {
+ my_command->argv[2] = c;
+ offsets[2] = offsets[1] + (c - my_command->argv[1]);
+ my_command->argc = 3;
+ }
+ else
+ {
+ syntax_error(source, lineno, my_command->first_line, my_command->argv[0],
+ "\\sleep command argument must be an integer",
+ my_command->argv[1], offsets[1] - start_offset);
+ }
}
}
Dear Miyake-san,
I agree your suggestions and I think this patch is basically good.
I put some comments:
* When the following line is input, the error message is not happy.
I think output should be " \sleep command argument must be an integer...".
\sleep foo
-> pgbench: fatal: test.sql:5: unrecognized time unit, must be us, ms or s (foo) in command "sleep"
\sleep foo
^ error found here
I'm not sure but I think this is caused because `my_command->argv[2]` becomes "foo".
* A blank is missed in some lines, for example:
+ if (my_command->argc ==2)
A blank should be added between a variable and an operator.
Could you fix them?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Miyake-san,
I'm not sure but I think this is caused because `my_command->argv[2]` becomes "foo".
Sorry, I missed some lines in your patch. Please ignore this analysis.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Thank you for your comments!
I fixed my patch based on your comment, and attached it to this mail.
2021-03-05 12:57, kuroda.hayato@fujitsu.com wrote:
* When the following line is input, the error message is not happy.
I think output should be " \sleep command argument must be an
integer...".\sleep foo
-> pgbench: fatal: test.sql:5: unrecognized time unit, must be us, ms
or s (foo) in command "sleep"
\sleep foo
^ error found here
When argc == 2, pgbench assumes that (1) argv[1] is just a number (e.g.
\sleep 10) or (2) argv[1] is a pair of a number and a time unit (e.g.
\sleep 10ms).
So I fixed the problem as follows:
1) When argv[1] starts with a number, raises an error depending on
whether the time unit is correct or not.
2) When argv[1] does not starts with a number, raises an error because
it must be an integer.
With this modification, the behavior for input should be as follows.
"\sleep 10" -> pass
"\sleep 10s" -> pass
"\sleep 10foo" -> "unrecognized time unit" error
"\sleep foo10" -> "\sleep command argument must be an integer..." error
Is this OK? Please tell me what you think.
* A blank is missed in some lines, for example:
+ if (my_command->argc ==2)
A blank should be added between a variable and an operator.
OK! I fixed it.
Regards
--
Kota Miyake
Attachments:
v02_pgbench_sleep.patchtext/x-diff; name=v02_pgbench_sleep.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f1d98be2d2..822461dce6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2861,7 +2861,18 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs)
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
}
+
+ for (int i = 0; i < strlen(var); i++)
+ {
+ if(!isdigit(var[i]))
+ {
+ pg_log_error("\\sleep command argument must be an integer (not \"%s\")", var);
+ return false;
+ }
+ }
+
usec = atoi(var);
+
}
else
usec = atoi(argv[1]);
@@ -4648,7 +4659,7 @@ process_backslash_command(PsqlScanState sstate, const char *source)
* will be parsed with atoi, which ignores trailing non-digit
* characters.
*/
- if (my_command->argc == 2 && my_command->argv[1][0] != ':')
+ if (my_command->argv[1][0] != ':')
{
char *c = my_command->argv[1];
@@ -4656,9 +4667,19 @@ process_backslash_command(PsqlScanState sstate, const char *source)
c++;
if (*c)
{
- my_command->argv[2] = c;
- offsets[2] = offsets[1] + (c - my_command->argv[1]);
- my_command->argc = 3;
+ /* Raise an error if argv[1] does not start with a number */
+ if (my_command->argc == 2 && my_command->argv[1] != c)
+ {
+ my_command->argv[2] = c;
+ offsets[2] = offsets[1] + (c - my_command->argv[1]);
+ my_command->argc = 3;
+ }
+ else
+ {
+ syntax_error(source, lineno, my_command->first_line, my_command->argv[0],
+ "\\sleep command argument must be an integer",
+ my_command->argv[1], offsets[1] - start_offset);
+ }
}
}
Dear Miyake-san,
Thank you for updating the patch.
When argc == 2, pgbench assumes that (1) argv[1] is just a number (e.g.
\sleep 10) or (2) argv[1] is a pair of a number and a time unit (e.g.
\sleep 10ms).
I see.
So I fixed the problem as follows:
1) When argv[1] starts with a number, raises an error depending on
whether the time unit is correct or not.
2) When argv[1] does not starts with a number, raises an error because
it must be an integer.With this modification, the behavior for input should be as follows.
"\sleep 10" -> pass
"\sleep 10s" -> pass
"\sleep 10foo" -> "unrecognized time unit" error
"\sleep foo10" -> "\sleep command argument must be an integer..." errorIs this OK? Please tell me what you think.
I confirmed your code and how it works, it's OK.
Finally the message should be "a positive integer" in order to handle
the following case:
\set time -1
\sleep :time
-> pgbench: error: \sleep command argument must be an integer (not "-1")
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
On 2021/03/08 14:58, kuroda.hayato@fujitsu.com wrote:
Dear Miyake-san,
Thank you for updating the patch.
When argc == 2, pgbench assumes that (1) argv[1] is just a number (e.g.
\sleep 10) or (2) argv[1] is a pair of a number and a time unit (e.g.
\sleep 10ms).I see.
So I fixed the problem as follows:
1) When argv[1] starts with a number, raises an error depending on
whether the time unit is correct or not.
2) When argv[1] does not starts with a number, raises an error because
it must be an integer.With this modification, the behavior for input should be as follows.
"\sleep 10" -> pass
"\sleep 10s" -> pass
"\sleep 10foo" -> "unrecognized time unit" error
"\sleep foo10" -> "\sleep command argument must be an integer..." errorIs this OK? Please tell me what you think.
I confirmed your code and how it works, it's OK.
Finally the message should be "a positive integer" in order to handle
the following case:\set time -1
\sleep :time-> pgbench: error: \sleep command argument must be an integer (not "-1")
Isn't it better to accept even negative sleep time like currently pgbench does?
Otherwise we always need to check the variable is a positive integer
(for example, using \if command) when using it as the sleep time in \sleep
command. That seems inconvenient.
+ syntax_error(source, lineno, my_command->first_line, my_command->argv[0],
+ "\\sleep command argument must be an integer",I like the error message like "invalid sleep time, must be an integer".
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Dear Fujii-san, Miyake-san
Isn't it better to accept even negative sleep time like currently pgbench does?
Otherwise we always need to check the variable is a positive integer
(for example, using \if command) when using it as the sleep time in \sleep
command. That seems inconvenient.
Both of them are acceptable for me.
But we should write down how it works when the negative value is input if we adopt.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
On 2021-Mar-08, kuroda.hayato@fujitsu.com wrote:
Dear Fujii-san, Miyake-san
Isn't it better to accept even negative sleep time like currently pgbench does?
Otherwise we always need to check the variable is a positive integer
(for example, using \if command) when using it as the sleep time in \sleep
command. That seems inconvenient.Both of them are acceptable for me.
But we should write down how it works when the negative value is input if we adopt.
Not sleeping at all seems a good reaction (same as for zero, I guess.)
--
�lvaro Herrera 39�49'30"S 73�17'W
"XML!" Exclaimed C++. "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
On 2021/03/08 23:10, Alvaro Herrera wrote:
On 2021-Mar-08, kuroda.hayato@fujitsu.com wrote:
Dear Fujii-san, Miyake-san
Isn't it better to accept even negative sleep time like currently pgbench does?
Otherwise we always need to check the variable is a positive integer
(for example, using \if command) when using it as the sleep time in \sleep
command. That seems inconvenient.Both of them are acceptable for me.
But we should write down how it works when the negative value is input if we adopt.Not sleeping at all seems a good reaction (same as for zero, I guess.)
+1. BTW, IIUC currently \sleep works in that way.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021/03/09 0:54, Fujii Masao wrote:
On 2021/03/08 23:10, Alvaro Herrera wrote:
On 2021-Mar-08, kuroda.hayato@fujitsu.com wrote:
Dear Fujii-san, Miyake-san
Isn't it better to accept even negative sleep time like currently pgbench does?
Otherwise we always need to check the variable is a positive integer
(for example, using \if command) when using it as the sleep time in \sleep
command. That seems inconvenient.Both of them are acceptable for me.
But we should write down how it works when the negative value is input if we adopt.Not sleeping at all seems a good reaction (same as for zero, I guess.)
+1. BTW, IIUC currently \sleep works in that way.
Attached is the updated version of the patch.
Currently a variable whose value is a negative number is allowed to be
specified as a sleep time as follows. In this case \sleep command doesn't
sleep. The patch doesn't change this behavior at all.
\set hoge -1
\sleep :hoge s
Currently a variable whose value is a double is allowed to be specified as
a sleep time as follows. In this case the integer value that atoi() converts
the double number into is used as a sleep time. The patch also doesn't
change this behavior at all because I've not found any strong reason to
ban that usage.
\set hoge 10 / 4.0
\sleep :hoge s
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v03_pgbench_sleep.patchtext/plain; charset=UTF-8; name=v03_pgbench_sleep.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e69d43b26b..a6d91d1089 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2953,7 +2953,24 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs)
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
}
+
usec = atoi(var);
+ if (usec == 0)
+ {
+ char *c = var;
+
+ /* Skip sign */
+ if (*c == '+' || *c == '-')
+ c++;
+
+ /* Raise an error if the value of a variable is not a number */
+ if (!isdigit((unsigned char) *c))
+ {
+ pg_log_error("%s: invalid sleep time \"%s\" for variable \"%s\"",
+ argv[0], var, argv[1] + 1);
+ return false;
+ }
+ }
}
else
usec = atoi(argv[1]);
@@ -4788,17 +4805,41 @@ process_backslash_command(PsqlScanState sstate, const char *source)
* will be parsed with atoi, which ignores trailing non-digit
* characters.
*/
- if (my_command->argc == 2 && my_command->argv[1][0] != ':')
+ if (my_command->argv[1][0] != ':')
{
char *c = my_command->argv[1];
+ bool have_digit = false;
- while (isdigit((unsigned char) *c))
+ /* Skip sign */
+ if (*c == '+' || *c == '-')
c++;
+
+ /* Require at least one digit */
+ if (*c && isdigit((unsigned char) *c))
+ have_digit = true;
+
+ /* Eat all digits */
+ while (*c && isdigit((unsigned char) *c))
+ c++;
+
if (*c)
{
- my_command->argv[2] = c;
- offsets[2] = offsets[1] + (c - my_command->argv[1]);
- my_command->argc = 3;
+ if (my_command->argc == 2 && have_digit)
+ {
+ my_command->argv[2] = c;
+ offsets[2] = offsets[1] + (c - my_command->argv[1]);
+ my_command->argc = 3;
+ }
+ else
+ {
+ /*
+ * Raise an error if argument starts with non-digit
+ * character (after sign).
+ */
+ syntax_error(source, lineno, my_command->first_line, my_command->argv[0],
+ "invalid sleep time, must be an integer",
+ my_command->argv[1], offsets[1] - start_offset);
+ }
}
}
Dear Fujii-san,
Thank you for updating the patch.
I understand that you don't want to change the current specification.
```diff
+ if (usec == 0)
+ {
+ char *c = var;
+
+ /* Skip sign */
+ if (*c == '+' || *c == '-')
+ c++;
```In my understanding the skip is not necessary, because
plus sign is already removed in the executeMetaCommand() and minus value can be returned by atoi().
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
On 2021/03/17 16:40, kuroda.hayato@fujitsu.com wrote:
Dear Fujii-san,
Thank you for updating the patch.
Thanks for the review!
I understand that you don't want to change the current specification.
```diff + if (usec == 0) + { + char *c = var; + + /* Skip sign */ + if (*c == '+' || *c == '-') + c++; ```In my understanding the skip is not necessary, because
plus sign is already removed in the executeMetaCommand() and minus value can be returned by atoi().
Yes, you're right. I removed that check from the patch.
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
v04_pgbench_sleep.patchtext/plain; charset=UTF-8; name=v04_pgbench_sleep.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e69d43b26b..48ce1712cc 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2953,7 +2953,16 @@ evaluateSleep(CState *st, int argc, char **argv, int *usecs)
pg_log_error("%s: undefined variable \"%s\"", argv[0], argv[1] + 1);
return false;
}
+
usec = atoi(var);
+
+ /* Raise an error if the value of a variable is not a number */
+ if (usec == 0 && !isdigit((unsigned char) *var))
+ {
+ pg_log_error("%s: invalid sleep time \"%s\" for variable \"%s\"",
+ argv[0], var, argv[1] + 1);
+ return false;
+ }
}
else
usec = atoi(argv[1]);
@@ -4788,17 +4797,41 @@ process_backslash_command(PsqlScanState sstate, const char *source)
* will be parsed with atoi, which ignores trailing non-digit
* characters.
*/
- if (my_command->argc == 2 && my_command->argv[1][0] != ':')
+ if (my_command->argv[1][0] != ':')
{
char *c = my_command->argv[1];
+ bool have_digit = false;
- while (isdigit((unsigned char) *c))
+ /* Skip sign */
+ if (*c == '+' || *c == '-')
c++;
+
+ /* Require at least one digit */
+ if (*c && isdigit((unsigned char) *c))
+ have_digit = true;
+
+ /* Eat all digits */
+ while (*c && isdigit((unsigned char) *c))
+ c++;
+
if (*c)
{
- my_command->argv[2] = c;
- offsets[2] = offsets[1] + (c - my_command->argv[1]);
- my_command->argc = 3;
+ if (my_command->argc == 2 && have_digit)
+ {
+ my_command->argv[2] = c;
+ offsets[2] = offsets[1] + (c - my_command->argv[1]);
+ my_command->argc = 3;
+ }
+ else
+ {
+ /*
+ * Raise an error if argument starts with non-digit
+ * character (after sign).
+ */
+ syntax_error(source, lineno, my_command->first_line, my_command->argv[0],
+ "invalid sleep time, must be an integer",
+ my_command->argv[1], offsets[1] - start_offset);
+ }
}
}
Dear Fujii-san,
I confirmed your patch and some parse functions, and I agree
the check condition in evaluateSleep() is correct.
No problem is found.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
On 2021/03/19 10:02, kuroda.hayato@fujitsu.com wrote:
Dear Fujii-san,
I confirmed your patch and some parse functions, and I agree
the check condition in evaluateSleep() is correct.
No problem is found.
Thanks for reviewing the patch!
Barring any objection, I will commit this patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021/03/19 10:43, Fujii Masao wrote:
On 2021/03/19 10:02, kuroda.hayato@fujitsu.com wrote:
Dear Fujii-san,
I confirmed your patch and some parse functions, and I agree
the check condition in evaluateSleep() is correct.
No problem is found.Thanks for reviewing the patch!
Barring any objection, I will commit this patch.
Pushed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION