Hello Fujii-san,
On Wed, 28 Jul 2021 00:20:21 +0900
Fujii Masao <[email protected]> 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 <[email protected]> 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 <[email protected]>
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)
{
