On Sun, Aug 27, 2017 at 5:12 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote: > > Hello Masahiko-san, > >> Attached latest v4 patch. Please review it.
Thank you for reviewing this patch! > > Patch applies, compiles. > > The messages/options do not seem to work properly: > > sh> ./pgbench -i -I t > done. Fixed this so that it ouptut "creating tables..." as you pointed out. > Does not seem to have initialized the tables although it was requested... > > sh> ./pgbench -i -I d > creating tables... > 100000 of 100000 tuples (100%) done (elapsed 0.08 s, remaining 0.00 s) > done. > > It seems that "d" triggered table creation... In fact it seems that the > work is done correctly, but the messages are not in the right place. Fixed, but I just removed "creating tables..." from -I d command. I think it's not good if we change the output messages by this patch. > Also another issue: > > sh> ./pgbench -i --foreign-keys > creating tables... > 100000 of 100000 tuples (100%) done (elapsed 0.09 s, remaining 0.00 s) > vacuum... > set primary keys... > done. > > Foreign keys do not seem to have been set... Please check that all really > work as expected. Fixed. > > About the documentation: > > If a native English speaker could review the text, it would be great. > > At least: "Required to invoke" -> "Require to invoke". 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'm really hesitating about the out of order processing of options. If the > user writes > > sh> pgbench -i --no-vacuum -I v > done. > > Then does it make sense to ignore the last thing the user asked for? ISTM > that processing options in order and keeping the last resulting spec is more > natural. Appending contradictory options can happen easily when scripting, > and usual what is meant is the last one. Agreed. I changed it so that it processes options in order and keeps the last resulting spec. > > Again, as pointed out in the previous review, I do not like much > checkCustomCmds implementation: switch/case, fprintf and return on error > which will trigger another fprintf and error above... ISTM that you should > either take into account previous comments or explain why you disagree with > them, but not send the same code without addressing them in any way. Sorry, I didn't mean to ignore, I'd just missed the comment. Fixed it. Attached latest patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
pgbench_custom_initialization_v5.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
