Re: Fix around conn_duration in pgbench

On Fri, 30 Jul 2021 15:26:51 +0900
Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:

>
>
> On 2021/07/30 14:43, Yugo NAGATA wrote:
> > This patch fixes three issues of connection time measurement:
> >
> > 1. The initial connection time is measured and stored into conn_duration
> > but the result is never used.
> > 2. The disconnection time are not measured even when -C is specified.
> > 3. The disconnection time measurement at the end of threadRun() is
> > not necessary.
> >
> > The first one exists only in v14 and master, but others are also in v13 and
> > before. So, I think we can back-patch these fixes.
>
> Yes, you're right.
>
> >
> >> But the patch fails to be back-patched to v13 or before because
> >> pgbench's time logic was changed by commit 547f04e734.
> >> Do you have the patches that can be back-patched to v13 or before?
> >
> > I attached the patch for v13.
>
> Thanks for the patch!
>
> + /*
> + * In CSTATE_FINISHED state, this finishCon() is a 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 v13, the disconnection time needs to be measured in CSTATE_FINISHED
> because the connection can be closed here when -C is not specified?

When -C is not specified, the disconnection time is not measured even in
the patch for v14+. I don't think it is a problem because the
disconnection delay at the end of benchmark almost doesn't affect the tps.

>
> /* tps is about actually executed transactions */
> tps_include = ntx / time_include;
> tps_exclude = ntx /
> (time_include - (INSTR_TIME_GET_DOUBLE(conn_total_time) / nclients));
>
> BTW, this is a bit different topic from the patch, but in v13,
> tps excluding connection establishing is calculated in the above way.
> The total connection time is divided by the number of clients,
> but why do we need this division? Do you have any idea?

conn_total_time is a sum of connection delays measured over all threads
that are running concurrently. So, we try to get the average connection
delays by dividing by the number of clients, I think. However, I am not
sure this is the right way though, and in fact it was revised in the
recent commit so that we don't report the "tps excluding connection
establishing" especially when -C is specified.

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>