On 2021/08/31 16:56, Fabien COELHO wrote:
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.
I agree that this is not a bug fix, so this is not a matter suitable for for
backpatching. Maybe for pg14.
+1. So we reached the consensus!
Attached is the updated version of the patch. Based on Nagata-san's latest
patch,
I just modified the comment slightly and ran pgindent. Barring any objection,
I will commit this patch only in master and v14.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index bca136bdd5..a33c91dced 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,19 @@ advanceConnectionState(TState *thread, CState *st,
StatsData *agg)
*/
case CSTATE_ABORTED:
case CSTATE_FINISHED:
+
+ /*
+ * Don't measure the disconnection delays here
even if in
+ * CSTATE_FINISHED and -C/--connect option is
specified.
+ * Because in this case all the connections
that this thread
+ * established are closed at the end of
transactions and the
+ * disconnection delays should have already
been measured at
+ * that moment.
+ *
+ * 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 +6555,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 +6848,7 @@ threadRun(void *arg)
}
done:
- start = pg_time_now();
disconnect_all(state, nstate);
- thread->conn_duration += pg_time_now() - start;
if (thread->logfile)
{
