On Tue, Aug 8, 2017 at 10:50 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote: > > Hello Mahahiko-san, > > My 0.02€ about the patch & feature, which only reflect my point of view:
Thank you for reviewing the patch! > Please add a number to patches to avoid ambiguities. This was patch was the > second sent on the thread. > > There is no need to have both custom_init & init functions. I'll suggest to > remove function "init", rename "custom_init" as "init", and make the option > default to what is appropriate so that it initialize the schema as > expected, and there is only one initialization mechanism. > > I would suggest to make initialization sub options (no-vacuum, > foreign-key...) rely on updating the initialization operations instead of > being maintained separately. Currently "--no-vacuum --custom-init=vacuum" > would skip vacuum, which is quite debatable... > > I'm not sure of the "custom-initialize" option name, but why not. I suggest > to remove "is_initialize_suite", and make custom-initialize require -i as > seems logical and upward compatible. > > ISTM that there should be short names for the phases. Maybe using only one > letter could simplify the code significantly, dropping commas and list, eg: > "t" for "create_table", "d" for "load_data", "p" for "create_pkey", "f" for > "create_fkey", "v" for "vacuum". > > I do not think that allowing a space in the list is a shell-wise idea. I think we can use it like --custom-initialize="create_table, vacuum" which is similar to what we specify a connection option to psql for example. But it will be unnecessary if we have the one letter version. > I'm also wondering whether using a list is a good option, because it implies > a large parse function, list management and so on. With the one letter > version, it could be just a string to be scanned char by char for > operations. I basically agree with the one letter version. But I'm concerned that it'll confuse users if we have more initialization steps for the pgbench initialization. If we add more various initialization steps it makes pgbench command hard to read and the users might have to remember these options. > > Otherwise, at least allow short two letter codes (eg: ct ld pk fk va), in > order to avoid writing a very long thing on the command line, which is quite > a pain: > > sh> pgbench > --custom-initialize=create_table,load_data,primary_key,foreign_key,vacuum > ... > > vs > > sh> pgbench -i -I tdpfv ... > > Maybe there could be short-hands for usual setups, eg "default" for "tdpv" > or maybe "ct,ld,pk,va", "full" for "tdpfv" or maybe "ct,ld,pk,fk,va"... If --custom-initialize option requires for i option to be set, "pgbench -i" means the initialization with full steps and "pgbench -i --custom-initialize=..." means the initialization with custom operation steps. > Typo in doc "initailize" -> "initialize". Option values should be presented > in their logical execution order, i.e. put vacuum at the end. > > Typo in help "initilize" -> "initialize". I would suggest to drop the space > in the > option value in the presentation so that quotes are not needed. > > Remove the "no-primary-keys" from the long option array as it has > disappeared. You might consider make "custom-initialize" take the 'I' short > option. > > Ranting unrelated to this patch: the automatic aid type switching based on > scale is a bad idea (tm), because when trying to benchmark it means that > changing the scale also changes the schema, and you really do not need that. > ISTM that it should always use INT8. Hmm, I think it's a valid point. Should we allow users to specify like the above thing in the custom initialization feature as well? 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
