Hello Fujii-san,
Thank you for looking at it.
On Tue, 27 Jul 2021 03:04:35 +0900
Fujii Masao <[email protected]> wrote:
> case CSTATE_FINISHED:
> + /* per-thread last disconnection time is not
> measured */
>
> Could you tell me why we don't need to do this measurement?
We don't need to do it because it is already done in CSTATE_END_TX state when
the transaction successfully finished. Also, we don't need it when the thread
is aborted (that it, in CSTATE_ABORTED case) because we can't report complete
results anyway in such cases.
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);
> done:
> start = pg_time_now();
> disconnect_all(state, nstate);
> thread->conn_duration += pg_time_now() - start;
>
> We should measure the disconnection time here only when -C option specified
> (i.e., is_connect variable is true)? Though, I'm not sure how much this
> change is helpful to reduce the performance overhead....
You are right. We are measuring the disconnection time only when -C option is
specified, but it is already done at the end of transaction (i.e.,
CSTATE_END_TX).
We need disconnection here only when we get an error.
Therefore, we don't need the measurement here.
I attached the updated patch.
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..ffd74db3ad 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,12 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
*/
case CSTATE_ABORTED:
case CSTATE_FINISHED:
+ /*
+ * 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.
+ */
finishCon(st);
return;
}
@@ -6615,6 +6625,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 +6649,7 @@ 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;
+ /* connection delay is measured globally between the barriers */
}
/* GO */
@@ -6846,9 +6850,7 @@ threadRun(void *arg)
}
done:
- start = pg_time_now();
disconnect_all(state, nstate);
- thread->conn_duration += pg_time_now() - start;
if (thread->logfile)
{
