Hello Masahiko-san,

Attached latest v4 patch. Please review it.

Patch applies, compiles.

The messages/options do not seem to work properly:

 sh> ./pgbench -i -I t
 done.

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.

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.


About the documentation:

If a native English speaker could review the text, it would be great.

At least: "Required to invoke" -> "Require to invoke".


About the code:

is_no_vacuum should be a bool?

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.

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.

--
Fabien.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to