Hello Fujii-san, Thank you for looking at it.
On Tue, 27 Jul 2021 03:04:35 +0900 Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > case CSTATE_FINISHED: > + /* per-thread last disconnection time is not > measured */ > > Could you tell me why we don't need to do this measurement? We don't need to do it because it is already done in CSTATE_END_TX state when the transaction successfully finished. Also, we don't need it when the thread is aborted (that it, in CSTATE_ABORTED case) because we can't report complete results anyway in such cases. I updated the comment. > - /* no connection delay to record */ > - thread->conn_duration = 0; > + /* connection delay is measured globally between the barriers */ > > This comment is really correct? I was thinking that the measurement is not > necessary here because this is the case where -C option is not specified. This comment means that, when -C is not specified, the connection delay is measured between the barrier point where the benchmark starts /* READY */ THREAD_BARRIER_WAIT(&barrier); and the barrier point where all the thread finish making initial connections. /* GO */ THREAD_BARRIER_WAIT(&barrier); > done: > start = pg_time_now(); > disconnect_all(state, nstate); > thread->conn_duration += pg_time_now() - start; > > We should measure the disconnection time here only when -C option specified > (i.e., is_connect variable is true)? Though, I'm not sure how much this > change is helpful to reduce the performance overhead.... You are right. We are measuring the disconnection time only when -C option is specified, but it is already done at the end of transaction (i.e., CSTATE_END_TX). We need disconnection here only when we get an error. Therefore, we don't need the measurement here. I attached the updated patch. Regards, Yugo Nagata -- Yugo NAGATA <nag...@sraoss.co.jp>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 364b5a2e47..ffd74db3ad 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -3545,8 +3545,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) @@ -3570,6 +3574,12 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg) */ case CSTATE_ABORTED: case CSTATE_FINISHED: + /* + * Per-thread last disconnection time is not measured because it + * is already done when the transaction successfully finished. + * Also, we don't need it when the thread is aborted because we + * can't report complete results anyway in such cases. + */ finishCon(st); return; } @@ -6615,6 +6625,7 @@ threadRun(void *arg) thread_start = pg_time_now(); thread->started_time = thread_start; + thread->conn_duration = 0; last_report = thread_start; next_report = last_report + (int64) 1000000 * progress; @@ -6638,14 +6649,7 @@ threadRun(void *arg) goto done; } } - - /* compute connection delay */ - thread->conn_duration = pg_time_now() - thread->started_time; - } - else - { - /* no connection delay to record */ - thread->conn_duration = 0; + /* connection delay is measured globally between the barriers */ } /* GO */ @@ -6846,9 +6850,7 @@ threadRun(void *arg) } done: - start = pg_time_now(); disconnect_all(state, nstate); - thread->conn_duration += pg_time_now() - start; if (thread->logfile) {