On Fri, 20 Aug 2021 02:05:27 +0900 Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> > On 2021/08/11 13:56, Fujii Masao wrote: > > Yes, but I was thinking that's a bug that we should fix. > > IOW, I was thinking that, in v13, both connection and disconnection delays > > should be measured whether -C is specified or not, *per spec*. > > But, in v13, the disconnection delays are not measured in the cases > > where -C is specified and not specified. So I was thinking that this is > > a bug and we should fix those both cases. > > > > But you're thinking that, in v13, the disconnection delays don't need to > > be measured because they are not measured for now? > > Please let me clarify my thought. Thank you for your clarification. > > In master and v14, > > # Expected behavior > (1) Both connection and disconnection delays should be measured > only when -C is specified, but not otherwise. > (2) When -C is specified, since each transaction establishes and closes > a connection, those delays should be measured for each transaction. > > # Current behavior > (1) Connection delay is measured whether -C is specified or not. > (2) Even when -C is specified, disconnection delay is NOT measured > at the end of transaction. > > # What the patch should do > (1) Make pgbench skip measuring connection and disconnection delays > if not necessary (i.e., -C is not specified). > (2) Make pgbench measure the disconnection delays whenever > the connection is closed at the end of transaction, when -C is > specified. I agree with you. This is what the patch for pg14 does. We don't need to measure disconnection delay when -C is not specified because the output just reports "initial connection time". > In v13 or before, > > # Expected behavior > (1) Both connection and disconnection delays should be measured > whether -C is specified or not. Because information about those delays > is used for the benchmark result report. > (2) When -C is specified, since each transaction establishes and closes > a connection, those delays should be measured for each transaction. > > # Current behavior > (1)(2) Disconnection delay is NOT measured whether -C is specified or not. > > # What the patch should do > (1)(2) Make pgbench measure the disconnection delays whenever > the connection is closed at the end of transaction (for -C case) > and the end of thread (for NOT -C case). Ok. That makes sense. The output reports "including connections establishing" and "excluding connections establishing" regardless with -C, so we should measure delays in the same way. I updated the patch for pg13 to measure disconnection delay when -C is not specified. I attached the updated patch for pg13 as well as one for pg14 which is same as attached before. > > Anyway, I changed the status of this patch to "Waiting on Author" in CF. I returned the status to "Ready for Committer". Could you please review 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 338c0152a2..0058f33333 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -3285,8 +3285,12 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg) if (is_connect) { + instr_time start = now; + + INSTR_TIME_SET_CURRENT_LAZY(start); finishCon(st); - INSTR_TIME_SET_ZERO(now); + INSTR_TIME_SET_CURRENT(now); + INSTR_TIME_ACCUM_DIFF(thread->conn_time, now, start); } if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded) @@ -3310,7 +3314,28 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg) */ case CSTATE_ABORTED: case CSTATE_FINISHED: - finishCon(st); + /* + * In CSTATE_FINISHED state, this finishCon() is unnecessary + * under -C/--connect because all the connections that this + * thread established should have already been closed at the end + * of transactions. So we need to measure the disconnection + * delays here only when -C/--connect is not specified. + * + * In CSTATE_ABORTED state, the measurement is no longer + * necessary because we cannot report complete results anyways + * in this case. + */ + if (!is_connect) + { + instr_time start = now; + + INSTR_TIME_SET_CURRENT_LAZY(start); + finishCon(st); + INSTR_TIME_SET_CURRENT(now); + INSTR_TIME_ACCUM_DIFF(thread->conn_time, now, start); + } + else + finishCon(st); return; } } @@ -6210,6 +6235,12 @@ main(int argc, char **argv) latency_late += thread->latency_late; INSTR_TIME_ADD(conn_total_time, thread->conn_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); /* @@ -6237,8 +6268,7 @@ threadRun(void *arg) { TState *thread = (TState *) arg; CState *state = thread->state; - instr_time start, - end; + instr_time start; int nstate = thread->nstate; int remains = nstate; /* number of remaining clients */ socket_set *sockets = alloc_socket_set(nstate); @@ -6502,10 +6532,8 @@ threadRun(void *arg) } done: - INSTR_TIME_SET_CURRENT(start); disconnect_all(state, nstate); - INSTR_TIME_SET_CURRENT(end); - INSTR_TIME_ACCUM_DIFF(thread->conn_time, end, start); + if (thread->logfile) { if (agg_interval > 0)
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) {