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.fu...@oss.nttdata.com> wrote:
case CSTATE_FINISHED:
+ /* per-thread last disconnection time is not
measured */
Could you tell me why we don't need to do this measurement?
We don't need to do it because it is already done in CSTATE_END_TX state when
the transaction successfully finished. Also, we don't need it when the thread
is aborted (that it, in CSTATE_ABORTED case) because we can't report complete
results anyway in such cases.
Understood.
I updated the comment.
Thanks!
+ * Per-thread last disconnection time is not
measured because it
+ * is already done when the transaction
successfully finished.
+ * Also, we don't need it when the thread is
aborted because we
+ * can't report complete results anyway in such
cases.
What about commenting a bit more explicitly like the following?
--------------------------------------------
In CSTATE_FINISHED state, this disconnect_all() is no-op under -C/--connect
because all the connections that this thread established should have already
been closed at the end of transactions. So we don't need to measure the
disconnection delays here.
In CSTATE_ABORTED state, the measurement is no longer necessary because we
cannot report complete results anyways in this case.
--------------------------------------------
- /* no connection delay to record */
- thread->conn_duration = 0;
+ /* connection delay is measured globally between the barriers */
This comment is really correct? I was thinking that the measurement is not
necessary here because this is the case where -C option is not specified.
This comment means that, when -C is not specified, the connection delay is
measured between the barrier point where the benchmark starts
/* READY */
THREAD_BARRIER_WAIT(&barrier);
and the barrier point where all the thread finish making initial connections.
/* GO */
THREAD_BARRIER_WAIT(&barrier);
Ok, so you're commenting about the initial connection delay that's
measured when -C is not specified. But I'm not sure if this comment
here is really helpful. Seem rather confusing??
done:
start = pg_time_now();
disconnect_all(state, nstate);
thread->conn_duration += pg_time_now() - start;
We should measure the disconnection time here only when -C option specified
(i.e., is_connect variable is true)? Though, I'm not sure how much this change
is helpful to reduce the performance overhead....
You are right. We are measuring the disconnection time only when -C option is
specified, but it is already done at the end of transaction (i.e.,
CSTATE_END_TX).
We need disconnection here only when we get an error.
Therefore, we don't need the measurement here.
Ok.
I found another disconnect_all().
/* XXX should this be connection time? */
disconnect_all(state, nclients);
The measurement is also not necessary here.
So the above comment should be removed or updated?
I attached the updated patch.
Thanks!
Regards.
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
