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
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 1303217..32805ea 100644
--- a/contrib/pgbench/pgbench.c
+++ b/contrib/pgbench/pgbench.c
@@ -164,6 +164,7 @@ bool use_log; /* log transaction latencies to a file */
bool use_quiet; /* quiet logging onto stderr */
int agg_interval; /* log aggregates instead of individual
* transactions */
+int progress = 0; /* thread progress report every this seconds */
bool is_connect; /* establish connection for each transaction */
bool is_latencies; /* report per-command latencies */
int main_pid; /* main process id used in log filename */
@@ -354,6 +355,8 @@ usage(void)
" protocol for submitting queries to server (default: simple)\n"
" -n do not run VACUUM before tests\n"
" -N do not update tables \"pgbench_tellers\" and \"pgbench_branches\"\n"
+ " -P NUM, --progress NUM\n"
+ " show thread progress report about every NUM seconds\n"
" -r report average latency per command\n"
" -s NUM report this scale factor in output\n"
" -S perform SELECT-only transactions\n"
@@ -2115,6 +2118,7 @@ main(int argc, char **argv)
{"unlogged-tables", no_argument, &unlogged_tables, 1},
{"sampling-rate", required_argument, NULL, 4},
{"aggregate-interval", required_argument, NULL, 5},
+ {"progress", required_argument, NULL, 'P'},
{NULL, 0, NULL, 0}
};
@@ -2181,7 +2185,7 @@ main(int argc, char **argv)
state = (CState *) pg_malloc(sizeof(CState));
memset(state, 0, sizeof(CState));
- while ((c = getopt_long(argc, argv, "ih:nvp:dqSNc:j:Crs:t:T:U:lf:D:F:M:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "ih:nvp:dqSNc:j:Crs:t:T:U:lf:D:F:M:P:", long_options, &optindex)) != -1)
{
switch (c)
{
@@ -2336,6 +2340,16 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 'P':
+ progress = atoi(optarg);
+ if (progress <= 0)
+ {
+ fprintf(stderr,
+ "thread progress delay (-P) must not be negative (%s)\n",
+ optarg);
+ exit(1);
+ }
+ break;
case 0:
/* This covers long options which take no argument. */
break;
@@ -2712,6 +2726,10 @@ threadRun(void *arg)
int nstate = thread->nstate;
int remains = nstate; /* number of remaining clients */
int i;
+ /* for reporting progress in benchmark result */
+ int64 start_report = INSTR_TIME_GET_MICROSEC(thread->start_time);
+ int64 last_report = start_report;
+ int64 last_count = 0;
AggVals aggs;
@@ -2875,6 +2893,32 @@ threadRun(void *arg)
st->con = NULL;
}
}
+
+ /* per thread progress report, about every specified seconds */
+ if (progress)
+ {
+ instr_time now_time;
+ int64 now, run;
+ INSTR_TIME_SET_CURRENT(now_time);
+ now = INSTR_TIME_GET_MICROSEC(now_time);
+ run = now - last_report;
+ if (run >= progress * 1000000)
+ {
+ /* generate and show report */
+ int64 count = 0;
+ double tps;
+
+ for (i=0; i<nstate; i++)
+ count += state[i].cnt;
+
+ tps = 1000000.0 * (count - last_count) / run;
+ fprintf(stderr, "%.1f s\t[thread %d]: tps = %f, AverageLatency(ms) = %Lg\n",
+ (now - start_report)/1000000.0, thread->tid, tps, (long double) (1.0 / tps)
+ );
+ last_count = count;
+ last_report = now;
+ }
+ }
}
done:
diff --git a/doc/src/sgml/pgbench.sgml b/doc/src/sgml/pgbench.sgml
index 8775606..5d633d4 100644
--- a/doc/src/sgml/pgbench.sgml
+++ b/doc/src/sgml/pgbench.sgml
@@ -392,6 +392,16 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</>
</varlistentry>
<varlistentry>
+ <term><option>-P</option> <replaceable>seconds</></term>
+ <term><option>--progress</option> <replaceable>seconds</></term>
+ <listitem>
+ <para>
+ Show thread progress report about every specified seconds.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-s</option> <replaceable>scale_factor</></term>
<listitem>
<para>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
