On Fri, 20 Aug 2021 02:05:27 +0900
Fujii Masao <[email protected]> wrote:
>
> On 2021/08/11 13:56, Fujii Masao wrote:
> > Yes, but I was thinking that's a bug that we should fix.
> > IOW, I was thinking that, in v13, both connection and disconnection delays
> > should be measured whether -C is specified or not, *per spec*.
> > But, in v13, the disconnection delays are not measured in the cases
> > where -C is specified and not specified. So I was thinking that this is
> > a bug and we should fix those both cases.
> >
> > But you're thinking that, in v13, the disconnection delays don't need to
> > be measured because they are not measured for now?
>
> Please let me clarify my thought.
Thank you for your clarification.
>
> In master and v14,
>
> # Expected behavior
> (1) Both connection and disconnection delays should be measured
> only when -C is specified, but not otherwise.
> (2) When -C is specified, since each transaction establishes and closes
> a connection, those delays should be measured for each transaction.
>
> # Current behavior
> (1) Connection delay is measured whether -C is specified or not.
> (2) Even when -C is specified, disconnection delay is NOT measured
> at the end of transaction.
>
> # What the patch should do
> (1) Make pgbench skip measuring connection and disconnection delays
> if not necessary (i.e., -C is not specified).
> (2) Make pgbench measure the disconnection delays whenever
> the connection is closed at the end of transaction, when -C is
> specified.
I agree with you. This is what the patch for pg14 does. We don't need to measure
disconnection delay when -C is not specified because the output just reports
"initial connection time".
> In v13 or before,
>
> # Expected behavior
> (1) Both connection and disconnection delays should be measured
> whether -C is specified or not. Because information about those delays
> is used for the benchmark result report.
> (2) When -C is specified, since each transaction establishes and closes
> a connection, those delays should be measured for each transaction.
>
> # Current behavior
> (1)(2) Disconnection delay is NOT measured whether -C is specified or not.
>
> # What the patch should do
> (1)(2) Make pgbench measure the disconnection delays whenever
> the connection is closed at the end of transaction (for -C case)
> and the end of thread (for NOT -C case).
Ok. That makes sense. The output reports "including connections establishing"
and "excluding connections establishing" regardless with -C, so we should
measure delays in the same way.
I updated the patch for pg13 to measure disconnection delay when -C is not
specified. I attached the updated patch for pg13 as well as one for pg14
which is same as attached before.
>
> Anyway, I changed the status of this patch to "Waiting on Author" in CF.
I returned the status to "Ready for Committer".
Could you please review this?
Regards,
Yugo Nagata
--
Yugo NAGATA <[email protected]>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 338c0152a2..0058f33333 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3285,8 +3285,12 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
if (is_connect)
{
+ instr_time start = now;
+
+ INSTR_TIME_SET_CURRENT_LAZY(start);
finishCon(st);
- INSTR_TIME_SET_ZERO(now);
+ INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_ACCUM_DIFF(thread->conn_time, now, start);
}
if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded)
@@ -3310,7 +3314,28 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
*/
case CSTATE_ABORTED:
case CSTATE_FINISHED:
- finishCon(st);
+ /*
+ * In CSTATE_FINISHED state, this finishCon() is unnecessary
+ * under -C/--connect because all the connections that this
+ * thread established should have already been closed at the end
+ * of transactions. So we need to measure the disconnection
+ * delays here only when -C/--connect is not specified.
+ *
+ * In CSTATE_ABORTED state, the measurement is no longer
+ * necessary because we cannot report complete results anyways
+ * in this case.
+ */
+ if (!is_connect)
+ {
+ instr_time start = now;
+
+ INSTR_TIME_SET_CURRENT_LAZY(start);
+ finishCon(st);
+ INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_ACCUM_DIFF(thread->conn_time, now, start);
+ }
+ else
+ finishCon(st);
return;
}
}
@@ -6210,6 +6235,12 @@ main(int argc, char **argv)
latency_late += thread->latency_late;
INSTR_TIME_ADD(conn_total_time, thread->conn_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);
/*
@@ -6237,8 +6268,7 @@ threadRun(void *arg)
{
TState *thread = (TState *) arg;
CState *state = thread->state;
- instr_time start,
- end;
+ instr_time start;
int nstate = thread->nstate;
int remains = nstate; /* number of remaining clients */
socket_set *sockets = alloc_socket_set(nstate);
@@ -6502,10 +6532,8 @@ threadRun(void *arg)
}
done:
- INSTR_TIME_SET_CURRENT(start);
disconnect_all(state, nstate);
- INSTR_TIME_SET_CURRENT(end);
- INSTR_TIME_ACCUM_DIFF(thread->conn_time, end, start);
+
if (thread->logfile)
{
if (agg_interval > 0)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..bf9649251b 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,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;
}
@@ -6550,7 +6565,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);
/*
@@ -6615,6 +6634,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 +6658,6 @@ 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;
}
/* GO */
@@ -6846,9 +6858,7 @@ threadRun(void *arg)
}
done:
- start = pg_time_now();
disconnect_all(state, nstate);
- thread->conn_duration += pg_time_now() - start;
if (thread->logfile)
{
