On Tue, 31 Aug 2021 15:39:18 +0900 (JST)
Tatsuo Ishii <[email protected]> wrote:
> >> >> > My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
> >> >> > include disconnection times, which are clearly linked to connections,
> >> >> > especially with -C. So I'd rather have the more meaningful figure even
> >> >> > at the price of a small change in an undocumented feature.
> >> >>
> >> >> +1. The aim of -C is trying to measure connection overhead which
> >> >> naturally includes disconnection overhead.
> >> >
> >> > I think it is better to measure disconnection delays when -C is
> >> > specified in
> >> > pg 14. This seems not necessary when -C is not specified because pgbench
> >> > just
> >> > reports "initial connection time".
> >>
> >> Ok.
> >>
> >> > However, what about pg13 or later? Do you think we should also change the
> >> > behavior of pg13 or later? If so, should we measure disconnection delay
> >> > even
> >> > when -C is not specified in pg13?
> >>
> >> You mean "pg13 or before"?
> >
> > Sorry, you are right. I mean "pg13 or before".
>
> I would think we should leave as it is for pg13 and before to not surprise
> users.
Ok. Thank you for your opinion. I also agree with not changing the behavior of
long-stable branches, and I think this is the same opinion as Fujii-san.
Attached is the patch to fix to measure disconnection delays that can be
applied to
pg14 or later.
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index bca136bdd5..d4b8d538f6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3553,8 +3553,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)
@@ -3578,6 +3582,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;
}
@@ -6538,7 +6553,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);
/*
@@ -6827,9 +6846,7 @@ threadRun(void *arg)
}
done:
- start = pg_time_now();
disconnect_all(state, nstate);
- thread->conn_duration += pg_time_now() - start;
if (thread->logfile)
{
