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

Hi Febien,

I send you my review result and refactoring patch. I think that your patch has
good function and many people surely want to use! I hope that my review comment
will be good for your patch.


* 1. Complete words and variable in source code and sgml document.
It is readable for user and developper that new patch completes words and
variables in existing source code. For example, SECONDS is NUM etc.

* 2. Output format in result for more readable.
I think taht output format should be simple and intuitive. Your patch's output
format is not easy to read very much. So I fix simple format and add Average
latency. My proposed format is good for readable and programing processing.

> [mitsu-ko@localhost postgresql]$ bin/pgbench -T10 -P5 -c2 -j2
> starting vacuum...end.
> 5.0 s     [thread 1]: tps = 1015.576032, AverageLatency(ms) = 0.000984663
> 5.0 s     [thread 0]: tps = 1032.580794, AverageLatency(ms) = 0.000968447
> 10.0 s [thread 0]: tps = 1129.591189, AverageLatency(ms) = 0.000885276
> 10.0 s [thread 1]: tps = 1126.267776, AverageLatency(ms) = 0.000887888

However, interesting of output format(design) is different depending on the
person:-). If you like other format, fix it.


* 3. Thread name in output format is not nesessary.
I cannot understand that thread name is displayed in each progress. I think that
it does not need. I hope that output result sould be more simple also in a lot of
thread. My images is here,

> [mitsu-ko@localhost postgresql]$ bin/pgbench -T10 -P5 -c2 -j2
> starting vacuum...end.
> 5.0 s     : tps = 2030.576032, AverageLatency(ms) = 0.000984663
> 10.0 s : tps = 2250.591189, AverageLatency(ms) = 0.000885276

This output format is more simple and intuitive. If you need result in each
threads, please tell us the reason.


* 4. Slipping the progress time.
Whan I executed this patch in long time, I found slipping the progress time. This
problem image is here.

 > [mitsu-ko@localhost postgresql]$ bin/pgbench -T10 -P5 -c2
 > starting vacuum...end.
 > 5.1 s     : tps = 2030.576032, AverageLatency(ms) = 0.000984663
 > 10.2 s : tps = 2250.591189, AverageLatency(ms) = 0.000885276

It has problem in method of calculate progress time. It needs to fix collect, or
displaying time format will be like '13:00:00'. If you select later format, it
will fit in postgresql log and other contrib modules that are like
pg_stat_statements.


Best regards,
--
Mitsumasa KONDO