Fix around conn_duration in pgbench
Hi,
TState has a field called "conn_duration" and this is, the comment says,
"cumulated connection and deconnection delays". This value is summed over
threads and reported as "average connection time" under -C/--connect.
If this options is not specified, the value is never used.
However, I found that conn_duration is calculated even when -C/--connect
is not specified, which is waste. SO we can remove this code as fixed in
the attached patch.
In addition, deconnection delays are not cumulated even under -C/--connect
in spite of mentioned in the comment. I also fixed this in the attached patch.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench_conn_duration.patchtext/x-diff; name=pgbench_conn_duration.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..1ec42a3ba2 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3536,8 +3536,10 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
if (is_connect)
{
+ pg_time_usec_t start = pg_time_now();
+
finishCon(st);
- now = 0;
+ thread->conn_duration += pg_time_now() - start;
}
if ((st->cnt >= nxacts && duration <= 0) || timer_exceeded)
@@ -6421,6 +6423,7 @@ main(int argc, char **argv)
thread->logfile = NULL; /* filled in later */
thread->latency_late = 0;
initStats(&thread->stats, 0);
+ thread->conn_duration = 0;
nclients_dealt += thread->nstate;
}
@@ -6584,14 +6587,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 */
Hello Yugo-san,
TState has a field called "conn_duration" and this is, the comment says,
"cumulated connection and deconnection delays". This value is summed over
threads and reported as "average connection time" under -C/--connect.
If this options is not specified, the value is never used.
Yep.
However, I found that conn_duration is calculated even when -C/--connect
is not specified, which is waste. SO we can remove this code as fixed in
the attached patch.
I'm fine with the implied code simplification, but it deserves a comment.
In addition, deconnection delays are not cumulated even under -C/--connect
in spite of mentioned in the comment. I also fixed this in the attached patch.
I'm fine with that, even if it only concerns is_connect. I've updated the
command to work whether now is initially set or not. I'm unsure whether
closing a pg connection actually waits for anything, so probably the
impact is rather small anyway. It cannot be usefully measured when
!is_connect, because thread do that when then feel like it, whereas on
connection we can use barriers to have something which makes sense.
Also, there is the issue of connection failures: the attached version adds
an error message and exit for initial connections consistently.
This is not done with is_connect, though, and I'm unsure what we should
really do.
--
Fabien.
Attachments:
pgbench-conn-duration-2.patchtext/x-diff; name=pgbench-conn-duration-2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..9d2abbfb68 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3171,6 +3171,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
if ((st->con = doConnect()) == NULL)
{
+ /* as the bench is already running, we do not abort */
pg_log_error("client %d aborted while establishing connection", st->id);
st->state = CSTATE_ABORTED;
break;
@@ -3536,8 +3537,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)
@@ -3561,6 +3566,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
*/
case CSTATE_ABORTED:
case CSTATE_FINISHED:
+ /* per-thread last disconnection time is not measured */
finishCon(st);
return;
}
@@ -4405,7 +4411,10 @@ runInitSteps(const char *initialize_steps)
initPQExpBuffer(&stats);
if ((con = doConnect()) == NULL)
+ {
+ pg_log_fatal("connection for initialization failed");
exit(1);
+ }
setup_cancel_handler(NULL);
SetCancelConn(con);
@@ -6332,7 +6341,10 @@ main(int argc, char **argv)
/* opening connection... */
con = doConnect();
if (con == NULL)
+ {
+ pg_log_fatal("setup connection failed");
exit(1);
+ }
pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
PQhost(con), PQport(con), nclients,
@@ -6548,7 +6560,7 @@ threadRun(void *arg)
if (thread->logfile == NULL)
{
pg_log_fatal("could not open logfile \"%s\": %m", logpath);
- goto done;
+ exit(1);
}
}
@@ -6561,6 +6573,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;
@@ -6572,26 +6585,13 @@ threadRun(void *arg)
{
if ((state[i].con = doConnect()) == NULL)
{
- /*
- * On connection failure, we meet the barrier here in place of
- * GO before proceeding to the "done" path which will cleanup,
- * so as to avoid locking the process.
- *
- * It is unclear whether it is worth doing anything rather
- * than coldly exiting with an error message.
- */
- THREAD_BARRIER_WAIT(&barrier);
- goto done;
+ /* coldly abort on connection failure */
+ pg_log_fatal("cannot create connection for thread %d client %d",
+ thread->tid, i);
+ exit(1);
}
}
-
- /* 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 */
On Mon, 14 Jun 2021 10:57:07 +0200 (CEST)
Fabien COELHO <coelho@cri.ensmp.fr> wrote:
However, I found that conn_duration is calculated even when -C/--connect
is not specified, which is waste. SO we can remove this code as fixed in
the attached patch.I'm fine with the implied code simplification, but it deserves a comment.
Thank you for adding comments!
In addition, deconnection delays are not cumulated even under -C/--connect
in spite of mentioned in the comment. I also fixed this in the attached patch.I'm fine with that, even if it only concerns is_connect. I've updated the
command to work whether now is initially set or not.
Ok. I agree with your update.
Also, there is the issue of connection failures: the attached version adds
an error message and exit for initial connections consistently.
This is not done with is_connect, though, and I'm unsure what we should
really do.
Well, as to connection failures, I think that we should discuss in the other
thread [1]/messages/by-id/TYCPR01MB5870057375ACA8A73099C649F5349@TYCPR01MB5870.jpnprd01.prod.outlook.com. where this issue was originally raised or in a new thread because
we can discuss this as a separated issue from the originally proposed patch.
[1]: /messages/by-id/TYCPR01MB5870057375ACA8A73099C649F5349@TYCPR01MB5870.jpnprd01.prod.outlook.com.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
This patch looks fine to me. master 48cb244fb9
The new status of this patch is: Ready for Committer
Hello Asif,
On Tue, 29 Jun 2021 13:21:54 +0000
Asif Rehman <asifr.rehman@gmail.com> wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not testedThis patch looks fine to me. master 48cb244fb9
The new status of this patch is: Ready for Committer
Thank you for reviewing this patch!
The previous patch included fixes about connection failures, but this part
was moved to another patch discussed in [1]/messages/by-id/alpine.DEB.2.22.394.2106181535400.3146194@pseudo.
[1]: /messages/by-id/alpine.DEB.2.22.394.2106181535400.3146194@pseudo
I attached the updated patach.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-2.patchtext/x-diff; name=pgbench-conn-duration-2.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d7479925cb..9d2abbfb68 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3171,6 +3171,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
if ((st->con = doConnect()) == NULL)
{
+ /* as the bench is already running, we do not abort */
pg_log_error("client %d aborted while establishing connection", st->id);
st->state = CSTATE_ABORTED;
break;
@@ -3536,8 +3537,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)
@@ -3561,6 +3566,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
*/
case CSTATE_ABORTED:
case CSTATE_FINISHED:
+ /* per-thread last disconnection time is not measured */
finishCon(st);
return;
}
@@ -4405,7 +4411,10 @@ runInitSteps(const char *initialize_steps)
initPQExpBuffer(&stats);
if ((con = doConnect()) == NULL)
+ {
+ pg_log_fatal("connection for initialization failed");
exit(1);
+ }
setup_cancel_handler(NULL);
SetCancelConn(con);
@@ -6332,7 +6341,10 @@ main(int argc, char **argv)
/* opening connection... */
con = doConnect();
if (con == NULL)
+ {
+ pg_log_fatal("setup connection failed");
exit(1);
+ }
pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
PQhost(con), PQport(con), nclients,
@@ -6548,7 +6560,7 @@ threadRun(void *arg)
if (thread->logfile == NULL)
{
pg_log_fatal("could not open logfile \"%s\": %m", logpath);
- goto done;
+ exit(1);
}
}
@@ -6561,6 +6573,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;
@@ -6572,26 +6585,13 @@ threadRun(void *arg)
{
if ((state[i].con = doConnect()) == NULL)
{
- /*
- * On connection failure, we meet the barrier here in place of
- * GO before proceeding to the "done" path which will cleanup,
- * so as to avoid locking the process.
- *
- * It is unclear whether it is worth doing anything rather
- * than coldly exiting with an error message.
- */
- THREAD_BARRIER_WAIT(&barrier);
- goto done;
+ /* coldly abort on connection failure */
+ pg_log_fatal("cannot create connection for thread %d client %d",
+ thread->tid, i);
+ exit(1);
}
}
-
- /* 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 */
On Wed, 30 Jun 2021 14:35:37 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:
Hello Asif,
On Tue, 29 Jun 2021 13:21:54 +0000
Asif Rehman <asifr.rehman@gmail.com> wrote:The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not testedThis patch looks fine to me. master 48cb244fb9
The new status of this patch is: Ready for Committer
Thank you for reviewing this patch!
The previous patch included fixes about connection failures, but this part
was moved to another patch discussed in [1].[1] /messages/by-id/alpine.DEB.2.22.394.2106181535400.3146194@pseudo
I attached the updated patach.
I am sorry but I attached the other patch. Attached in this post
is the latest patch.
Regards,
Yugo Nagata
Regards,
Yugo Nagata--
Yugo NAGATA <nagata@sraoss.co.jp>
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-3.patchtext/x-diff; name=pgbench-conn-duration-3.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4aeccd93af..ad81cf1101 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3539,8 +3539,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)
@@ -3564,6 +3568,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
*/
case CSTATE_ABORTED:
case CSTATE_FINISHED:
+ /* per-thread last disconnection time is not measured */
finishCon(st);
return;
}
@@ -6597,6 +6602,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;
@@ -6620,14 +6626,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 */
On 2021/06/30 14:43, Yugo NAGATA wrote:
On Wed, 30 Jun 2021 14:35:37 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:Hello Asif,
On Tue, 29 Jun 2021 13:21:54 +0000
Asif Rehman <asifr.rehman@gmail.com> wrote:The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not testedThis patch looks fine to me. master 48cb244fb9
The new status of this patch is: Ready for Committer
Thank you for reviewing this patch!
The previous patch included fixes about connection failures, but this part
was moved to another patch discussed in [1].[1] /messages/by-id/alpine.DEB.2.22.394.2106181535400.3146194@pseudo
I attached the updated patach.
I am sorry but I attached the other patch. Attached in this post
is the latest patch.
case CSTATE_FINISHED:
+ /* per-thread last disconnection time is not measured */
Could you tell me why we don't need to do this measurement?
- /* 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.
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....
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hello Fujii-san,
Thank you for looking at it.
On Tue, 27 Jul 2021 03:04:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> 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 <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-4.patchtext/x-diff; name=pgbench-conn-duration-4.patchDownload
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)
{
On 2021/07/27 11:02, Yugo NAGATA wrote:
Hello Fujii-san,
Thank you for looking at it.
On Tue, 27 Jul 2021 03:04:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> 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.
Understood.
I updated the comment.
Thanks!
+ * 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.What about commenting a bit more explicitly like the following?
--------------------------------------------
In CSTATE_FINISHED state, this disconnect_all() is 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.
--------------------------------------------
- /* 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);
Ok, so you're commenting about the initial connection delay that's
measured when -C is not specified. But I'm not sure if this comment
here is really helpful. Seem rather confusing??
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.
Ok.
I found another disconnect_all().
/* XXX should this be connection time? */
disconnect_all(state, nclients);
The measurement is also not necessary here.
So the above comment should be removed or updated?
I attached the updated patch.
Thanks!
Regards.
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hello Fujii-san,
On Wed, 28 Jul 2021 00:20:21 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/07/27 11:02, Yugo NAGATA wrote:
Hello Fujii-san,
Thank you for looking at it.
On Tue, 27 Jul 2021 03:04:35 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
+ * 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.What about commenting a bit more explicitly like the following?
--------------------------------------------
In CSTATE_FINISHED state, this disconnect_all() is 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.
--------------------------------------------
Thank you for the suggestion. 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);Ok, so you're commenting about the initial connection delay that's
measured when -C is not specified. But I'm not sure if this comment
here is really helpful. Seem rather confusing??
Ok. I removed this comment.
I found another disconnect_all().
/* XXX should this be connection time? */
disconnect_all(state, nclients);The measurement is also not necessary here.
So the above comment should be removed or updated?
I think this disconnect_all will be a no-op because all connections should
be already closed in threadRun(), but I left it just to be sure that
connections are all cleaned-up. I updated the comment for explaining above.
I attached the updated patch. Could you please look at this?
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-5.patchtext/x-diff; name=pgbench-conn-duration-5.patchDownload
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)
{
On 2021/07/28 16:15, Yugo NAGATA wrote:
I found another disconnect_all().
/* XXX should this be connection time? */
disconnect_all(state, nclients);The measurement is also not necessary here.
So the above comment should be removed or updated?I think this disconnect_all will be a no-op because all connections should
be already closed in threadRun(), but I left it just to be sure that
connections are all cleaned-up. I updated the comment for explaining above.I attached the updated patch. Could you please look at this?
Thanks for updating the patch! LGTM.
Barring any objection, I will commit the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021/07/29 2:23, Fujii Masao wrote:
On 2021/07/28 16:15, Yugo NAGATA wrote:
I found another disconnect_all().
����/* XXX should this be connection time? */
����disconnect_all(state, nclients);The measurement is also not necessary here.
So the above comment should be removed or updated?I think this disconnect_all will be a no-op because all connections should
be already closed in threadRun(), but I left it just to be sure that
connections are all cleaned-up. I updated the comment for explaining above.I attached the updated patch. Could you please look at this?
Thanks for updating the patch! LGTM.
This patch needs to be back-patched because it fixes the bug
in measurement of disconnection delays. Thought?
But the patch fails to be back-patched to v13 or before because
pgbench's time logic was changed by commit 547f04e734.
Do you have the patches that can be back-patched to v13 or before?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hello Fujii-san,
On Fri, 30 Jul 2021 02:01:08 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/07/29 2:23, Fujii Masao wrote:
On 2021/07/28 16:15, Yugo NAGATA wrote:
I found another disconnect_all().
/* XXX should this be connection time? */
disconnect_all(state, nclients);The measurement is also not necessary here.
So the above comment should be removed or updated?I think this disconnect_all will be a no-op because all connections should
be already closed in threadRun(), but I left it just to be sure that
connections are all cleaned-up. I updated the comment for explaining above.I attached the updated patch. Could you please look at this?
Thanks for updating the patch! LGTM.
This patch needs to be back-patched because it fixes the bug
in measurement of disconnection delays. Thought?
This patch fixes three issues of connection time measurement:
1. The initial connection time is measured and stored into conn_duration
but the result is never used.
2. The disconnection time are not measured even when -C is specified.
3. The disconnection time measurement at the end of threadRun() is
not necessary.
The first one exists only in v14 and master, but others are also in v13 and
before. So, I think we can back-patch these fixes.
But the patch fails to be back-patched to v13 or before because
pgbench's time logic was changed by commit 547f04e734.
Do you have the patches that can be back-patched to v13 or before?
I attached the patch for v13.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-for-pg13.patchtext/x-diff; name=pgbench-conn-duration-for-pg13.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 338c0152a2..3411556df8 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,6 +3314,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;
}
@@ -6210,6 +6225,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 +6258,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 +6522,7 @@ 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)
On 2021/07/30 14:43, Yugo NAGATA wrote:
This patch fixes three issues of connection time measurement:
1. The initial connection time is measured and stored into conn_duration
but the result is never used.
2. The disconnection time are not measured even when -C is specified.
3. The disconnection time measurement at the end of threadRun() is
not necessary.The first one exists only in v14 and master, but others are also in v13 and
before. So, I think we can back-patch these fixes.
Yes, you're right.
But the patch fails to be back-patched to v13 or before because
pgbench's time logic was changed by commit 547f04e734.
Do you have the patches that can be back-patched to v13 or before?I attached the patch for v13.
Thanks for the patch!
+ /*
+ * 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 v13, the disconnection time needs to be measured in CSTATE_FINISHED
because the connection can be closed here when -C is not specified?
/* tps is about actually executed transactions */
tps_include = ntx / time_include;
tps_exclude = ntx /
(time_include - (INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));
BTW, this is a bit different topic from the patch, but in v13,
tps excluding connection establishing is calculated in the above way.
The total connection time is divided by the number of clients,
but why do we need this division? Do you have any idea?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, 30 Jul 2021 15:26:51 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/07/30 14:43, Yugo NAGATA wrote:
This patch fixes three issues of connection time measurement:
1. The initial connection time is measured and stored into conn_duration
but the result is never used.
2. The disconnection time are not measured even when -C is specified.
3. The disconnection time measurement at the end of threadRun() is
not necessary.The first one exists only in v14 and master, but others are also in v13 and
before. So, I think we can back-patch these fixes.Yes, you're right.
But the patch fails to be back-patched to v13 or before because
pgbench's time logic was changed by commit 547f04e734.
Do you have the patches that can be back-patched to v13 or before?I attached the patch for v13.
Thanks for the patch!
+ /* + * 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 v13, the disconnection time needs to be measured in CSTATE_FINISHED
because the connection can be closed here when -C is not specified?
When -C is not specified, the disconnection time is not measured even in
the patch for v14+. I don't think it is a problem because the
disconnection delay at the end of benchmark almost doesn't affect the tps.
/* tps is about actually executed transactions */
tps_include = ntx / time_include;
tps_exclude = ntx /
(time_include - (INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));BTW, this is a bit different topic from the patch, but in v13,
tps excluding connection establishing is calculated in the above way.
The total connection time is divided by the number of clients,
but why do we need this division? Do you have any idea?
conn_total_time is a sum of connection delays measured over all threads
that are running concurrently. So, we try to get the average connection
delays by dividing by the number of clients, I think. However, I am not
sure this is the right way though, and in fact it was revised in the
recent commit so that we don't report the "tps excluding connection
establishing" especially when -C is specified.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
On 2021/08/01 14:50, Yugo NAGATA wrote:
When -C is not specified, the disconnection time is not measured even in
the patch for v14+. I don't think it is a problem because the
disconnection delay at the end of benchmark almost doesn't affect the tps.
What about v13 or before? That is, in v13, even when -C is not specified,
both the connection and disconnection delays are measured. Right?
If right, the time required to close the connection in CSTATE_FINISHED
state should also be measured?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hello Fujii-san,
On Thu, 5 Aug 2021 16:16:48 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/08/01 14:50, Yugo NAGATA wrote:
When -C is not specified, the disconnection time is not measured even in
the patch for v14+. I don't think it is a problem because the
disconnection delay at the end of benchmark almost doesn't affect the tps.What about v13 or before? That is, in v13, even when -C is not specified,
both the connection and disconnection delays are measured. Right?
No. Although there is a code measuring the thread->conn_time around
disconnect_all() in v13 or before;
done:
INSTR_TIME_SET_CURRENT(start);
disconnect_all(state, nstate);
INSTR_TIME_SET_CURRENT(end);
INSTR_TIME_ACCUM_DIFF(thread->conn_time, end, start);
this is a no-op because finishCon() is already called at CSTATE_ABORTED or
CSTATE_FINISHED. Therefore, in the end, the disconnection delay is not
measured even in v13.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
On 2021/08/05 18:02, Yugo NAGATA wrote:
this is a no-op because finishCon() is already called at CSTATE_ABORTED or
CSTATE_FINISHED. Therefore, in the end, the disconnection delay is not
measured even in v13.
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?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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.
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.
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).
Thought?
Anyway, I changed the status of this patch to "Waiting on Author" in CF.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, 20 Aug 2021 02:05:27 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> 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 <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-for-pg13-2.patchtext/x-diff; name=pgbench-conn-duration-for-pg13-2.patchDownload
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)
pgbench-conn-duration-pg14-5.patchtext/x-diff; name=pgbench-conn-duration-pg14-5.patchDownload
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)
{
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?
According to the patch tester, the patch does not apply.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
On Mon, 30 Aug 2021 14:22:49 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
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?According to the patch tester, the patch does not apply.
Well, that's because both the patch for PG14 and one for PG13
are discussed here.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA <nagata@sraoss.co.jp>
On Mon, 30 Aug 2021 14:22:49 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote: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?According to the patch tester, the patch does not apply.
Well, that's because both the patch for PG14 and one for PG13
are discussed here.
Oh, ok. So the patch tester is not smart enough to identify each patch
for particular branches.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
On 2021/08/26 12:13, Yugo NAGATA wrote:
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.
On second thought, it's more reasonable and less confusing not to
measure the disconnection delays at all? Since whether the benchmark result
should include the disconnection delays or not is not undocumented,
probably we cannot say strongly the current behavior (i.e., the disconnection
delays are not measured) is a bug. Also since the result has not included
the disconnection delays so far, the proposed change might slightly change
the benchmark numbers reported, which might confuse the users.
ISTM that at least it's unwise to change long-stable branches for this... Thought?
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.
Thanks! I pushed the part of the patch, which gets rid of unnecessary
measure of connection delays from pgbench.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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.On second thought, it's more reasonable and less confusing not to
measure the disconnection delays at all? Since whether the benchmark result
should include the disconnection delays or not is not undocumented,
probably we cannot say strongly the current behavior (i.e., the disconnection
delays are not measured) is a bug. Also since the result has not included
the disconnection delays so far, the proposed change might slightly change
the benchmark numbers reported, which might confuse the users.
ISTM that at least it's unwise to change long-stable branches for this...
Thought?
My 0.02�: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even at
the price of a small change in an undocumented feature.
--
Fabien.
Hello Fujii-san,
On Mon, 30 Aug 2021 23:36:30 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/08/26 12:13, Yugo NAGATA wrote:
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.On second thought, it's more reasonable and less confusing not to
measure the disconnection delays at all? Since whether the benchmark result
should include the disconnection delays or not is not undocumented,
probably we cannot say strongly the current behavior (i.e., the disconnection
delays are not measured) is a bug. Also since the result has not included
the disconnection delays so far, the proposed change might slightly change
the benchmark numbers reported, which might confuse the users.
ISTM that at least it's unwise to change long-stable branches for this... Thought?
Ok. I agree with you that it is better to not change the behavior of pg13 or
before at least. As for pg14 or later, I wonder that we can change it when pg14
is released because the output was already change in the commit 547f04e734,
although, I am not persisting to measure disconnection delay since the effect
to tps would be very slight. At least, if we decide to not measure disconnection
delays, I think we should fix as so, like the attached patch.
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.Thanks! I pushed the part of the patch, which gets rid of unnecessary
measure of connection delays from pgbench.
Thank you!
Regards, Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench-not-measure-disconnect.patchtext/x-diff; name=pgbench-not-measure-disconnect.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index bca136bdd5..6d895968fe 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -6538,7 +6538,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.
+ */
disconnect_all(state, nclients);
/*
@@ -6827,9 +6831,7 @@ threadRun(void *arg)
}
done:
- start = pg_time_now();
disconnect_all(state, nstate);
- thread->conn_duration += pg_time_now() - start;
if (thread->logfile)
{
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.On second thought, it's more reasonable and less confusing not to
measure the disconnection delays at all? Since whether the benchmark
result
should include the disconnection delays or not is not undocumented,
probably we cannot say strongly the current behavior (i.e., the
disconnection
delays are not measured) is a bug. Also since the result has not
included
the disconnection delays so far, the proposed change might slightly
change
the benchmark numbers reported, which might confuse the users.
ISTM that at least it's unwise to change long-stable branches for
this... Thought?My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even
at the price of a small change in an undocumented feature.
+1. The aim of -C is trying to measure connection overhead which
naturally includes disconnection overhead.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jpHello Fabien, Ishii-san,
On Tue, 31 Aug 2021 14:18:48 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
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.On second thought, it's more reasonable and less confusing not to
measure the disconnection delays at all? Since whether the benchmark
result
should include the disconnection delays or not is not undocumented,
probably we cannot say strongly the current behavior (i.e., the
disconnection
delays are not measured) is a bug. Also since the result has not
included
the disconnection delays so far, the proposed change might slightly
change
the benchmark numbers reported, which might confuse the users.
ISTM that at least it's unwise to change long-stable branches for
this... Thought?My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even
at the price of a small change in an undocumented feature.+1. The aim of -C is trying to measure connection overhead which
naturally includes disconnection overhead.
I think it is better to measure disconnection delays when -C is specified in
pg 14. This seems not necessary when -C is not specified because pgbench just
reports "initial connection time".
However, what about pg13 or later? Do you think we should also change the
behavior of pg13 or later? If so, should we measure disconnection delay even
when -C is not specified in pg13?
Regards,
Yugo Nagata
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA <nagata@sraoss.co.jp>
My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even
at the price of a small change in an undocumented feature.+1. The aim of -C is trying to measure connection overhead which
naturally includes disconnection overhead.I think it is better to measure disconnection delays when -C is specified in
pg 14. This seems not necessary when -C is not specified because pgbench just
reports "initial connection time".
Ok.
However, what about pg13 or later? Do you think we should also change the
behavior of pg13 or later? If so, should we measure disconnection delay even
when -C is not specified in pg13?
You mean "pg13 or before"?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
On Tue, 31 Aug 2021 14:46:42 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even
at the price of a small change in an undocumented feature.+1. The aim of -C is trying to measure connection overhead which
naturally includes disconnection overhead.I think it is better to measure disconnection delays when -C is specified in
pg 14. This seems not necessary when -C is not specified because pgbench just
reports "initial connection time".Ok.
However, what about pg13 or later? Do you think we should also change the
behavior of pg13 or later? If so, should we measure disconnection delay even
when -C is not specified in pg13?You mean "pg13 or before"?
Sorry, you are right. I mean "pg13 or before".
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA <nagata@sraoss.co.jp>
My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even
at the price of a small change in an undocumented feature.+1. The aim of -C is trying to measure connection overhead which
naturally includes disconnection overhead.I think it is better to measure disconnection delays when -C is specified in
pg 14. This seems not necessary when -C is not specified because pgbench just
reports "initial connection time".Ok.
However, what about pg13 or later? Do you think we should also change the
behavior of pg13 or later? If so, should we measure disconnection delay even
when -C is not specified in pg13?You mean "pg13 or before"?
Sorry, you are right. I mean "pg13 or before".
I would think we should leave as it is for pg13 and before to not surprise users.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
On Tue, 31 Aug 2021 15:39:18 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
My 0.02€: From a benchmarking perspective, ISTM that it makes sense to
include disconnection times, which are clearly linked to connections,
especially with -C. So I'd rather have the more meaningful figure even
at the price of a small change in an undocumented feature.+1. The aim of -C is trying to measure connection overhead which
naturally includes disconnection overhead.I think it is better to measure disconnection delays when -C is specified in
pg 14. This seems not necessary when -C is not specified because pgbench just
reports "initial connection time".Ok.
However, what about pg13 or later? Do you think we should also change the
behavior of pg13 or later? If so, should we measure disconnection delay even
when -C is not specified in pg13?You mean "pg13 or before"?
Sorry, you are right. I mean "pg13 or before".
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.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
pgbench-conn-duration-pg14-6.patchtext/x-diff; name=pgbench-conn-duration-pg14-6.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index bca136bdd5..d4b8d538f6 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,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;
}
@@ -6538,7 +6553,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 +6846,7 @@ threadRun(void *arg)
}
done:
- start = pg_time_now();
disconnect_all(state, nstate);
- thread->conn_duration += pg_time_now() - start;
if (thread->logfile)
{
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.
--
Fabien.
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
Attachments:
pgbench-conn-duration-pg14-7.patchtext/plain; charset=UTF-8; name=pgbench-conn-duration-pg14-7.patch; x-mac-creator=0; x-mac-type=0Download
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)
{
On 2021/09/01 1:10, Fujii Masao wrote:
+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.
Pushed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Wed, 1 Sep 2021 17:07:43 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
On 2021/09/01 1:10, Fujii Masao wrote:
+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.Pushed. Thanks!
Thank you!
--
Yugo NAGATA <nagata@sraoss.co.jp>