|From:||Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>|
|To:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: pgbench: Skipping the creating primary keys after initialization|
|Views:||Raw Message | Whole Thread | Download mbox|
On Sun, Aug 27, 2017 at 5:12 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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
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)
> 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)
> set primary keys...
> 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?
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
> 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.
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
|Next Message||Craig Ringer||2017-08-28 05:18:35||psql --batch|
|Previous Message||Craig Ringer||2017-08-28 01:40:11||Re: WIP: Separate log file for extension|