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 <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>