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 <masao(dot)fujii(at)oss(dot)nttdata(dot)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