Re: Fix around conn_duration in pgbench
On Wed, 1 Sep 2021 17:07:43 +0900 Fujii Masao 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
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
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
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();
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)
{
Re: Fix around conn_duration in pgbench
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.
Re: Fix around conn_duration in pgbench
On Tue, 31 Aug 2021 15:39:18 +0900 (JST)
Tatsuo Ishii 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
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();
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)
{
Re: Fix around conn_duration in pgbench
>> >> > 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
Re: Fix around conn_duration in pgbench
On Tue, 31 Aug 2021 14:46:42 +0900 (JST) Tatsuo Ishii 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
Re: Fix around conn_duration in pgbench
>> > 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
Re: Fix around conn_duration in pgbench
Hello Fabien, Ishii-san, On Tue, 31 Aug 2021 14:18:48 +0900 (JST) Tatsuo Ishii 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
Re: Fix around conn_duration in pgbench
>>> 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.jp
Re: Fix around conn_duration in pgbench
Hello Fujii-san,
On Mon, 30 Aug 2021 23:36:30 +0900
Fujii Masao 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
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)
{
Re: Fix around conn_duration in pgbench
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.
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
> On Mon, 30 Aug 2021 14:22:49 +0900 (JST) > Tatsuo Ishii 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
Re: Fix around conn_duration in pgbench
On Mon, 30 Aug 2021 14:22:49 +0900 (JST) Tatsuo Ishii 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
Re: Fix around conn_duration in pgbench
>> 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
Re: Fix around conn_duration in pgbench
On Fri, 20 Aug 2021 02:05:27 +0900
Fujii Masao 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
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 338c0152a2..0058f3 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 @@
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
Hello Fujii-san, On Thu, 5 Aug 2021 16:16:48 +0900 Fujii Masao 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
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
On Fri, 30 Jul 2021 15:26:51 +0900 Fujii Masao 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
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
Hello Fujii-san,
On Fri, 30 Jul 2021 02:01:08 +0900
Fujii Masao 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
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)
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
Hello Fujii-san,
On Wed, 28 Jul 2021 00:20:21 +0900
Fujii Masao 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 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();
> >
> > and the barrier point where all the thread finish making initial
> > connections.
> >
> > /* GO */
> > THREAD_BARRIER_WAIT();
>
> 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
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();
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) 100 * 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 +=
Re: Fix around conn_duration in pgbench
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 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(); and the barrier point where all the thread finish making initial connections. /* GO */ THREAD_BARRIER_WAIT(); 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
Re: Fix around conn_duration in pgbench
Hello Fujii-san,
Thank you for looking at it.
On Tue, 27 Jul 2021 03:04:35 +0900
Fujii Masao 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();
and the barrier point where all the thread finish making initial connections.
/* GO */
THREAD_BARRIER_WAIT();
> 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
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();
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) 100 * 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)
{
Re: Fix around conn_duration in pgbench
On 2021/06/30 14:43, Yugo NAGATA wrote: On Wed, 30 Jun 2021 14:35:37 +0900 Yugo NAGATA wrote: Hello Asif, On Tue, 29 Jun 2021 13:21:54 + Asif Rehman 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 tested This 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] https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2106181535400.3146194%40pseudo 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
Re: Fix around conn_duration in pgbench
On Wed, 30 Jun 2021 14:35:37 +0900
Yugo NAGATA wrote:
> Hello Asif,
>
> On Tue, 29 Jun 2021 13:21:54 +
> Asif Rehman 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 tested
> >
> > This 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]
> https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2106181535400.3146194%40pseudo
>
> 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
--
Yugo NAGATA
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();
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) 100 * 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 */
Re: Fix around conn_duration in pgbench
Hello Asif,
On Tue, 29 Jun 2021 13:21:54 +
Asif Rehman 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 tested
>
> This 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]
https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2106181535400.3146194%40pseudo
I attached the updated patach.
Regards,
Yugo Nagata
--
Yugo NAGATA
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();
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();
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) 100 * 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();
-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 */
Re: Fix around conn_duration in pgbench
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
Re: Fix around conn_duration in pgbench
On Mon, 14 Jun 2021 10:57:07 +0200 (CEST) Fabien COELHO 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] 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] https://www.postgresql.org/message-id/flat/TYCPR01MB5870057375ACA8A73099C649F5349%40TYCPR01MB5870.jpnprd01.prod.outlook.com. Regards, Yugo Nagata -- Yugo NAGATA
Re: Fix around conn_duration in pgbench
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.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();
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();
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) 100 * 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();
-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 */
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
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(>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 */
