Thread: 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>
Attachment
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.
Attachment
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] 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 <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 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 <nagata@sraoss.co.jp>
Attachment
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 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 <nagata@sraoss.co.jp> -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
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 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
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>
Attachment
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
threadestablished should have already been closed at the end of transactions. So we don't need to measure the
disconnectiondelays here.
In CSTATE_ABORTED state, the measurement is no longer necessary because we cannot report complete results anyways in
thiscase.
--------------------------------------------
>
>> - /* 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 threadestablished should have already been closed at the end of transactions. So we don't need to measure the disconnectiondelays here. > > In CSTATE_ABORTED state, the measurement is no longer necessary because we cannot report complete results anyways in thiscase. > -------------------------------------------- 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>
Attachment
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>
Attachment
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>
Attachment
>> 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>
Attachment
>>> 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
Hello 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>
Attachment
>> 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
Attachment
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>