Re: Fix around conn_duration in pgbench

Hello Fujii-san,

On Wed, 28 Jul 2021 00:20:21 +0900
Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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(dot)fujii(at)oss(dot)nttdata(dot)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 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.
> --------------------------------------------

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(at)sraoss(dot)co(dot)jp>