On Tue, 31 Aug 2021 15:39:18 +0900 (JST) Tatsuo Ishii <is...@sraoss.co.jp> wrote:
> >> >> > My 0.02€: From a benchmarking perspective, ISTM that it makes sense to > >> >> > include disconnection times, which are clearly linked to connections, > >> >> > especially with -C. So I'd rather have the more meaningful figure even > >> >> > at the price of a small change in an undocumented feature. > >> >> > >> >> +1. The aim of -C is trying to measure connection overhead which > >> >> naturally includes disconnection overhead. > >> > > >> > I think it is better to measure disconnection delays when -C is > >> > specified in > >> > pg 14. This seems not necessary when -C is not specified because pgbench > >> > just > >> > reports "initial connection time". > >> > >> Ok. > >> > >> > However, what about pg13 or later? Do you think we should also change the > >> > behavior of pg13 or later? If so, should we measure disconnection delay > >> > even > >> > when -C is not specified in pg13? > >> > >> You mean "pg13 or before"? > > > > Sorry, you are right. I mean "pg13 or before". > > I would think we should leave as it is for pg13 and before to not surprise > users. Ok. Thank you for your opinion. I also agree with not changing the behavior of long-stable branches, and I think this is the same opinion as Fujii-san. Attached is the patch to fix to measure disconnection delays that can be applied to pg14 or later. Regards, Yugo Nagata -- Yugo NAGATA <nag...@sraoss.co.jp>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index bca136bdd5..d4b8d538f6 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -3553,8 +3553,12 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg) if (is_connect) { + pg_time_usec_t start = now; + + pg_time_now_lazy(&start); finishCon(st); - now = 0; + now = pg_time_now(); + thread->conn_duration += now - start; } if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) @@ -3578,6 +3582,17 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg) */ case CSTATE_ABORTED: case CSTATE_FINISHED: + /* + * In CSTATE_FINISHED state, this finishCon() is a no-op + * under -C/--connect because all the connections that this + * thread established should have already been closed at the end + * of transactions. So we don't need to measure the disconnection + * delays here. + * + * In CSTATE_ABORTED state, the measurement is no longer + * necessary because we cannot report complete results anyways + * in this case. + */ finishCon(st); return; } @@ -6538,7 +6553,11 @@ main(int argc, char **argv) bench_start = thread->bench_start; } - /* XXX should this be connection time? */ + /* + * All connections should be already closed in threadRun(), so this + * disconnect_all() will be a no-op, but clean up the connecions just + * to be sure. We don't need to measure the disconnection delays here. + */ disconnect_all(state, nclients); /* @@ -6827,9 +6846,7 @@ threadRun(void *arg) } done: - start = pg_time_now(); disconnect_all(state, nstate); - thread->conn_duration += pg_time_now() - start; if (thread->logfile) {