Horton

Thread

Commits

  1. pgbench: Improve error-handling in \sleep command.

  1. [PATCH] pgbench: improve \sleep meta command

    miyake_kouta <miyake_kouta@oss.nttdata.com> — 2021-03-05T01:51:04Z

    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

  2. RE: [PATCH] pgbench: improve \sleep meta command

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2021-03-05T03:57:56Z

    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

  3. RE: [PATCH] pgbench: improve \sleep meta command

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2021-03-05T04:10:19Z

    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

  4. RE: [PATCH] pgbench: improve \sleep meta command

    miyake_kouta <miyake_kouta@oss.nttdata.com> — 2021-03-08T01:51:27Z

    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

  5. RE: [PATCH] pgbench: improve \sleep meta command

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2021-03-08T05:58:04Z

    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

  6. Re: [PATCH] pgbench: improve \sleep meta command

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-08T07:16:24Z

    
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..." 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

  7. RE: [PATCH] pgbench: improve \sleep meta command

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2021-03-08T07:33:05Z

    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

  8. Re: [PATCH] pgbench: improve \sleep meta command

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-08T14:10:26Z

    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.

  9. Re: [PATCH] pgbench: improve \sleep meta command

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-08T15:54:18Z

    
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

  10. Re: [PATCH] pgbench: improve \sleep meta command

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-16T16:49:51Z

    
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

  11. RE: [PATCH] pgbench: improve \sleep meta command

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2021-03-17T07:40:30Z

    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

  12. Re: [PATCH] pgbench: improve \sleep meta command

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-18T07:32:37Z

    
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

  13. RE: [PATCH] pgbench: improve \sleep meta command

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2021-03-19T01:02:20Z

    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

  14. Re: [PATCH] pgbench: improve \sleep meta command

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-19T01:43:08Z

    
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

  15. Re: [PATCH] pgbench: improve \sleep meta command

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-22T03:06:28Z

    
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