Re: Fix around conn_duration in pgbench - Mailing list pgsql-hackers

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