Hello Fujii-san, On Wed, 28 Jul 2021 00:20:21 +0900 Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> > > On 2021/07/27 11:02, Yugo NAGATA wrote: > > 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: > + * 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. > > What about commenting a bit more explicitly like the following? > > -------------------------------------------- > In CSTATE_FINISHED state, this disconnect_all() is 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. > -------------------------------------------- Thank you for the suggestion. 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); > > Ok, so you're commenting about the initial connection delay that's > measured when -C is not specified. But I'm not sure if this comment > here is really helpful. Seem rather confusing?? Ok. I removed this comment. > I found another disconnect_all(). > > /* XXX should this be connection time? */ > disconnect_all(state, nclients); > > The measurement is also not necessary here. > So the above comment should be removed or updated? I think this disconnect_all will be a no-op because all connections should be already closed in threadRun(), but I left it just to be sure that connections are all cleaned-up. I updated the comment for explaining above. I attached the updated patch. Could you please look at this? 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..bf9649251b 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,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; } @@ -6550,7 +6565,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); /* @@ -6615,6 +6634,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 +6658,6 @@ 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; } /* GO */ @@ -6846,9 +6858,7 @@ threadRun(void *arg) } done: - start = pg_time_now(); disconnect_all(state, nstate); - thread->conn_duration += pg_time_now() - start; if (thread->logfile) {