Hello Fujii-san, On Fri, 30 Jul 2021 02:01:08 +0900 Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> > > On 2021/07/29 2:23, Fujii Masao wrote: > > > > > > On 2021/07/28 16:15, Yugo NAGATA wrote: > >>> 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? > > > > Thanks for updating the patch! LGTM. > > This patch needs to be back-patched because it fixes the bug > in measurement of disconnection delays. Thought? This patch fixes three issues of connection time measurement: 1. The initial connection time is measured and stored into conn_duration but the result is never used. 2. The disconnection time are not measured even when -C is specified. 3. The disconnection time measurement at the end of threadRun() is not necessary. The first one exists only in v14 and master, but others are also in v13 and before. So, I think we can back-patch these fixes. > But the patch fails to be back-patched to v13 or before because > pgbench's time logic was changed by commit 547f04e734. > Do you have the patches that can be back-patched to v13 or before? I attached the patch for v13. 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..3411556df8 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,6 +3314,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; } @@ -6210,6 +6225,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 +6258,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 +6522,7 @@ 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)