On Sun, Aug 27, 2017 at 5:12 PM, Fabien COELHO <coelho@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers