|From:||Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>|
|To:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: pgbench: Skipping the creating primary keys after initialization|
|Views:||Raw Message | Whole Thread | Download mbox|
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".
> I meant the added one about -I, not the pre-existing one about -i.
>>> 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
> initialize_cmds initialization: rather use pg_strdup instead of
> -I: pg_free before pg_strdup to avoid a small memory leak?
> 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.
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.
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
|Next Message||Michael Paquier||2017-08-29 01:30:55||Improving overflow checks when adding tuple to PGresult Re: [GENERAL] Retrieving query results|
|Previous Message||Craig Ringer||2017-08-29 01:26:13||Re: Custom allocators in libpq|