RE: [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

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(dot)hayato(at)fujitsu(dot)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

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..." error
>
> Is 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(dot)hayato(at)fujitsu(dot)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..." error
>>
>> Is 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(dot)hayato(at)fujitsu(dot)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(dot)hayato(at)fujitsu(dot)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(dot)hayato(at)fujitsu(dot)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

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(dot)hayato(at)fujitsu(dot)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

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(dot)hayato(at)fujitsu(dot)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(dot)hayato(at)fujitsu(dot)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