Re: Fix around conn_duration in pgbench

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>