On Mon, Aug 28, 2017 at 4:59 PM, Fabien COELHO <coe...@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
pgbench_custom_initialization_v6.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
