Re: pgbench: Skipping the creating primary keys after initialization

On Mon, Aug 28, 2017 at 4:59 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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