(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


--
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