pgbench: Fix bug in measurement of disconnection delays.

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
index a3843c2de4c638fa7ae160eb281ab28719fbc7e4..d4cbaac3615392bb15d8c2eace4f86d4127eda5e 100644 (file)
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3552,8 +3552,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)
@@ -3577,6 +3581,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;
        }
@@ -6557,7 +6574,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);
 
    /*
@@ -6846,9 +6867,7 @@ threadRun(void *arg)
    }
 
 done:
-   start = pg_time_now();
    disconnect_all(state, nstate);
-   thread->conn_duration += pg_time_now() - start;
 
    if (thread->logfile)
    {