On Mon, Aug 28, 2017 at 4:59 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> Hello Masahiko-san,
>
> Patch applies cleanly, compiles, works for me.
Thank you for reviewing!
>
>>> At least: "Required to invoke" -> "Require to invoke".
>>
>>
>> Fixed.
>
>
> I meant the added one about -I, not the pre-existing one about -i.
Fixed.
>>> About the code:
>>>
>>> is_no_vacuum should be a bool?
>>
>>
>> We can change it but I think there is no difference actually. So
>> keeping it would be better.
>
>
> I would like to insist a little bit: the name was declared as an int but
> passed to init and used as a bool there before the patch. Conceptually what
> is meant is really a bool, and I see no added value bar saving a few strokes
> to have an int. ISTM that recent pgbench changes have started turning old
> int-for-bool habits into using bool when bool is meant.
Since is_no_vacuum is a existing one, if we follow the habit we should
change other similar variables as well: is_init_mode,
do_vacuum_accounts and debug. And I think we should change them in a
separated patch.
>
> initialize_cmds initialization: rather use pg_strdup instead of
> pg_malloc/strcpy?
Fixed.
> -I: pg_free before pg_strdup to avoid a small memory leak?
Fixed.
> I'm not sure I would have bothered with sizeof(char), but why not!
>
> I'm still a little bit annoyed by checkCustomCmds: when unhappy, it shows an
> error message and return false, which immediatly results in exit(1). However
> the pattern elsewhere in pgbench is to show the error and exit immediatly. I
> would suggest to simplify by void-ing the function and exiting instead of
> returning false.
Agreed, fixed.
After more thought, I'm bit inclined to not have a short option for
--custom-initialize because this option will be used for not primary
cases. It would be better to save short options for future
enhancements of pgbench. Thought?
Attached latest patch.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers