pgbench: Fix bug in measurement of disconnection delays.

MasaoFujii · MasaoFujii · commit 4dc528bfa7da · 2021-09-01T17:05:13.000+09:00
When -C/--connect option is specified, pgbench establishes and closes the connection for each transaction. In this case pgbench needs to measure the times taken for all those connections and disconnections, to include the average connection time in the benchmark result. But previously pgbench could not measure those disconnection delays. To fix the bug, this commit makes pgbench measure the disconnection delays whenever the connection is closed at the end of transaction, if -C/--connect option is specified. Back-patch to v14. Per discussion, we concluded not to back-patch to v13 or before because changing that behavior in stable branches would surprise users rather than providing benefits. Author: Yugo Nagata Reviewed-by: Fabien COELHO, Tatsuo Ishii, Asif Rehman, Fujii Masao Discussion: https://postgr.es/m/20210614151155.a393bc7d8fed183e38c9f52a@sraoss.co.jp
diff --git 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)
 	{
