Re: Enhancing pgbench parameter checking

Hi,

It is pretty annoying that pgbench does not check parameter which
should not be used with -i. I often type like:

pgbench -c 10 -T 300 -S -i test

and accidentally initialize pgbench database. This is pretty
uncomfortable if the database is huge since initializing huge database
takes long time. Why don't we check the case? Included is the patch to
enhance the behavior of pgbench in this regard IMO. Here is a sample
session after patching:

$ ./pgbench -c 10 -T 300 -S -i test
some parameters cannot be used in initialize mode

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

> Included is the patch to enhance the behavior of pgbench in this regard
> IMO. Here is a sample session after patching:
>
> $ ./pgbench -c 10 -T 300 -S -i test
> some parameters cannot be used in initialize mode

I have not tested, but the patch looks ok in principle.

I'm not sure of the variable name "is_non_init_parameter_set". I would
suggest "benchmarking_option_set"?

Also, to be consistent, maybe it should check that no
initialization-specific option are set when benchmarking.

--
Fabien.

Fabien,

> I have not tested, but the patch looks ok in principle.

Thanks for the review. I have registered it to Aug Commit fest.
https://commitfest.postgresql.org/action/patch_view?id=1532

> I'm not sure of the variable name "is_non_init_parameter_set". I would suggest "benchmarking_option_set"?

Ok, I will replace the variable name as you suggested.

> Also, to be consistent, maybe it should check that no initialization-specific option are set when benchmarking.

Good suggesition. Here is the v2 patch.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Hello Tatsuo-san,

> Thanks for the review. I have registered it to Aug Commit fest.
> https://commitfest.postgresql.org/action/patch_view?id=1532
>
>> I'm not sure of the variable name "is_non_init_parameter_set". I would
>> suggest "benchmarking_option_set"?
>
> Ok, I will replace the variable name as you suggested.
>
>> Also, to be consistent, maybe it should check that no initialization-specific option are set when benchmarking.
>
> Good suggesition. Here is the v2 patch.

I applied it without problem and tested it.

* It seems that -c is ignored, the atoi() line has been removed.

* Option -q is initialization specific, but not detected as such like the
other, although there is a specific detection later. I think that it would
be better to use the systematic approach, and to remove the specific
check.

* I would name the second boolean "initialization_option_set", as it is
describe like that in the documentation.

* I would suggest the following error messages:
"some options cannot be used in initialization (-i) mode\n" and
"some options cannot be used in benchmarking mode\n".
Although these messages are rough, I think that they are enough and avoid
running something unexpected, which is your purpose.

Find attached a patch which adds these changes to your current version.

--
Fabien.

Fabien,

> Hello Tatsuo-san,
>
>> Thanks for the review. I have registered it to Aug Commit fest.
>> https://commitfest.postgresql.org/action/patch_view?id=1532
>>
>>> I'm not sure of the variable name "is_non_init_parameter_set". I would
>>> suggest "benchmarking_option_set"?
>>
>> Ok, I will replace the variable name as you suggested.
>>
>>> Also, to be consistent, maybe it should check that no
>>> initialization-specific option are set when benchmarking.
>>
>> Good suggesition. Here is the v2 patch.
>
> I applied it without problem and tested it.
>
>
> * It seems that -c is ignored, the atoi() line has been removed.
>
> * Option -q is initialization specific, but not detected as such like
> * the other, although there is a specific detection later. I think that
> * it would be better to use the systematic approach, and to remove the
> * specific check.
>
> * I would name the second boolean "initialization_option_set", as it is
> * describe like that in the documentation.
>
>
> * I would suggest the following error messages:
> "some options cannot be used in initialization (-i) mode\n" and
> "some options cannot be used in benchmarking mode\n".
> Although these messages are rough, I think that they are enough and
> avoid running something unexpected, which is your purpose.
>
>
> Find attached a patch which adds these changes to your current
> version.

Thank you for the review and patch. Looks good. I changed the status
to "Ready for Committer". I will wait for a fewd days and if there's
no objection, I will commit the patch.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Fabien,

>> Find attached a patch which adds these changes to your current
>> version.
>
> Thank you for the review and patch. Looks good. I changed the status
> to "Ready for Committer". I will wait for a fewd days and if there's
> no objection, I will commit the patch.

I have committed the patch. Thanks!

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp