Re: [PATCH] add --progress option to pgbench (submission 3)

(2013/06/28 3:17), Fabien COELHO wrote:
> Attached is patch version 5.
>
> It includes this solution for fork emulation, one report per thread instead of a
> global report. Some code duplication for that.
It's good coding. I test configure option with --disable-thread-safety and not. 
My test results were same as your proposal. It fix problems that compatiblity and 
progress time is off to the side, too. Here is the test results.

* with --disable-thread-safety
[mitsu-ko@localhost postgresql]$ bin/pgbench -T 600 -c10 -j5 -P 5
starting vacuum...end.
progress 1: 5.0 s, 493.8 tps, 4.050 ms lat
progress 2: 5.0 s, 493.2 tps, 4.055 ms lat
progress 3: 5.0 s, 474.6 tps, 4.214 ms lat
progress 4: 5.0 s, 479.1 tps, 4.174 ms lat
progress 0: 5.0 s, 469.5 tps, 4.260 ms lat

* without --disable-thread-safety (normal)
[mitsu-ko@localhost postgresql]$ bin/pgbench -T 600 -c10 -j5 -P 5
starting vacuum...end.
progress: 5.0 s, 2415.0 tps, 4.141 ms lat
progress: 10.0 s, 2445.5 tps, 4.089 ms lat
progress: 15.0 s, 2442.2 tps, 4.095 ms lat
progress: 20.0 s, 2414.3 tps, 4.142 ms lat

> Finally, I've added a latency measure as defended by Mitsumasa. However the
> formula must be updated for the throttling patch.
Thanks! In benchmark test, it is not good to over throttle. It is difficult to 
set appropriate options which are number of client or number of threads. These 
result will help to set appropriate throttle options. We can easy to search by 
these information which is indicated as high tps and low latency as possible.

I have small comments. I think that 'lat' is not generally abbreviation of 
'latency'. But I don't know good abbreviation. If you have any good abbreviation, 
please send us revise version. And, please fix under following code. It might be 
degrade by past your patches.

-           "  -P, --progress SEC       show thread progress report every SEC seconds\n"
+           "  -P, --progress NUM       show thread progress report every NUM seconds\n"

-                tps = 1000000.0 * (count-last_count) / run;
+                tps = 1000000.0 * (count - last_count) / run;

My comments are that's all. If you send latest patch, I'm going to set ready for 
commiter.


I also test your throttle patch. My impression of this patch is good, but it does 
not necessary to execute with progress option. Because, in the first place, 
throttle patch is controlling transaction of pgbench, and it does not need to 
display progress which will be same information which is expected by a user. A 
user who uses throttle patch will think that throttle patch can control 
transaction exactly, and it is not debugging option. So I think that it had 
better to increase the accuracy of throttle patch, and it does not need to exist 
together of both patches. If you think that it cannot exist together, I suggest 
that forbidding simultaneously progress option and throttle option.

Best regards,
--
Mitsumasa KONDO
NTT Open Source Software Center