pgbench: option delaying queries till connections establishment?

Hi,

I am trying to run a few benchmarks measuring the effects of patch to
make GetSnapshotData() faster in the face of larger numbers of
established connections.

Before the patch connection establishment often is very slow due to
contention. The first few connections are fast, but after that it takes
increasingly long. The first few connections constantly hold
ProcArrayLock in shared mode, which then makes it hard for new
connections to acquire it exclusively (I'm addressing that to a
significant degree in the patch FWIW).

But for a fair comparison of the runtime effects I'd like to only
compare the throughput for when connections are actually usable,
otherwise I end up benchmarking few vs many connections, which is not
useful. And because I'd like to run the numbers for a lot of different
numbers of connections etc, I can't just make each run several hour
longs to make the initial minutes not matter much.

Therefore I'd like to make pgbench wait till it has established all
connections, before they run queries.

Does anybody else see this as being useful?

If so, should this be done unconditionally? A new option? Included in an
existing one somehow?

Greetings,

Andres Freund

Hi,

On 2020-02-27 10:01:00 -0800, Andres Freund wrote:
> If so, should this be done unconditionally? A new option? Included in an
> existing one somehow?

FWIW, leaving windows, error handling, and other annoyances aside, this
can be implemented fairly simply. See below.

As an example of the difference:

Before:
andres@awork3:~/build/postgres/dev-optimize/vpath$ ./src/bin/pgbench/pgbench -M prepared -c 5000 -j 100 -T 100 -P1 -S
starting vacuum...end.
progress: 100.4 s, 515307.4 tps, lat 1.374 ms stddev 7.739
transaction type: <builtin: select only>
scaling factor: 30
query mode: prepared
number of clients: 5000
number of threads: 100
duration: 100 s
number of transactions actually processed: 51728348
latency average = 1.374 ms
latency stddev = 7.739 ms
tps = 513802.541226 (including connections establishing)
tps = 521342.427158 (excluding connections establishing)


Note that there's no progress report until the end. That's because the
main thread didn't get a connection until the other threads were done.


After:

pgbench -M prepared -c 5000 -j 100 -T 100 -P1 -S
starting vacuum...end.
progress: 1.5 s, 9943.5 tps, lat 4.795 ms stddev 14.822
progress: 2.0 s, 380312.6 tps, lat 1.728 ms stddev 15.461
progress: 3.0 s, 478811.1 tps, lat 2.052 ms stddev 31.687
progress: 4.0 s, 470804.6 tps, lat 1.941 ms stddev 24.661



I think this also shows that "including/excluding connections
establishing" as well as some of the other stats reported pretty
bogus. In the 'before' case a substantial numer of the connections had
not yet been established until the end of the test run!



diff --git i/src/bin/pgbench/pgbench.c w/src/bin/pgbench/pgbench.c
index 1159757acb0..1a82c6a290e 100644
--- i/src/bin/pgbench/pgbench.c
+++ w/src/bin/pgbench/pgbench.c
@@ -310,6 +310,8 @@ typedef struct RandomState
 /* Various random sequences are initialized from this one. */
 static RandomState base_random_sequence;
 
+pthread_barrier_t conn_barrier;
+
 /*
  * Connection state machine states.
  */
@@ -6110,6 +6112,8 @@ main(int argc, char **argv)
 
     /* start threads */
 #ifdef ENABLE_THREAD_SAFETY
+    pthread_barrier_init(&conn_barrier, NULL, nthreads);
+
     for (i = 0; i < nthreads; i++)
     {
         TState     *thread = &threads[i];
@@ -6265,6 +6269,8 @@ threadRun(void *arg)
     INSTR_TIME_SET_CURRENT(thread->conn_time);
     INSTR_TIME_SUBTRACT(thread->conn_time, thread->start_time);
 
+    pthread_barrier_wait(&conn_barrier);
+
     /* explicitly initialize the state machines */
     for (i = 0; i < nstate; i++)
     {

Greetings,

Andres Freund

At Thu, 27 Feb 2020 10:51:29 -0800, Andres Freund <[hidden email]> wrote in
I see it useful. In most cases we don't care connection time of
pgbench. Especially in the mentioned case the result is just bogus.  I
think the reason for "including/excluding connection establishing" is
not that people wants to see how long connection took to establish but
that how long the substantial work took.  If each client did run with
continuously re-establishing new connections the connection time would
be useful, but actually all the connections are established at once at
the beginning.

So FWIW I prefer that the barrier is applied by default (that is, it
can be disabled) and the progress time starts at the time all clients
has been established.

> starting vacuum...end.
+ time to established 5000 connections: 1323ms
! progress: 1.0 s, 330000.5 tps, lat 2.795 ms stddev 14.822
! progress: 2.0 s, 380312.6 tps, lat 1.728 ms stddev 15.461
! progress: 3.0 s, 478811.1 tps, lat 2.052 ms stddev 31.687
! progress: 4.0 s, 470804.6 tps, lat 1.941 ms stddev 24.661
regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello Andres,

> Therefore I'd like to make pgbench wait till it has established all
> connections, before they run queries.

> Does anybody else see this as being useful?

Yes, I think that having this behavior available would make sense.

> If so, should this be done unconditionally?

Dunno. I should think about it. I'd say probably.

Pgbench is more or less designed to run a long hopefully steady-state
benchmark, so that the initial connection setup is always negligeable. Not
complying with this hypothesis quite often leads to weird results.

> A new option?

Maybe, if not unconditional.

If there is an unconditional barrier, the excluding/including connection
stuff does not make a lot of sense when not under -C, if it did make any
sense before…

> Included in an existing one somehow?

Which one would you suggest?

Adding a synchronization barrier should be simple enough, I thought about
it in the past.

However, I'd still be wary that it is no silver bullet: if you start a lot
of threads compared to the number of available cores, pgbench would
basically overload the system, and you would experience a lot of waiting
time which reflects that the client code has not got enough cpu time.
Basically you would be testing the OS process/thread management
performance.

On my 4-core laptop, with a do-nothing script (\set i 0):

   sh> pgbench -T 10 -f nope.sql -P 1 -j 10 -c 10
   latency average = 0.000 ms
   latency stddev = 0.049 ms
   tps = 21048841.630291 (including connections establishing)
   tps = 21075139.938887 (excluding connections establishing)

   sh> pgbench -T 10 -f nope.sql -P 1 -j 100 -c 100
   latency average = 0.002 ms
   latency stddev = 0.470 ms
   tps = 23846777.241370 (including connections establishing)
   tps = 24132252.146257 (excluding connections establishing)

Throughput is slightly better, latency average and variance explode
because each thread is given stretches of cpu time to advance, then wait
for the next round of cpu time.

--
Fabien.

Hello Kyotaro-san,

Yep.

> (that is, it can be disabled)

On reflection, I'm not sure I see a use case for not running the barrier
if it is available.

> and the progress time starts at the time all clients has been
> established.

Yep, the start time should be set after the connection barrier, and
possibly before a start barrier to ensure that no transaction has started
before the start time: although performance measures are expected to be
slightly false because of how they are measured (measuring takes time),
from a benchmarking perspective the displayed result should be <= the
actual performance.

Now, again, if long benchmarks are run, which for a db should more or less
always be the case, this should not matter much.

>> starting vacuum...end.
> + time to established 5000 connections: 1323ms

Yep, maybe showing the initial connection time separately.

--
Fabien.

Hi,

On 2020-02-29 15:29:19 +0100, Fabien COELHO wrote:
> Pgbench is more or less designed to run a long hopefully steady-state
> benchmark, so that the initial connection setup is always negligeable. Not
> complying with this hypothesis quite often leads to weird results.

I don't think this is a good starting point. Sure, a longer run will
yield more precise results, and one needs more than just an
instantaneous measurement. But in a lot of cases we want to use pgbench
to measure a lot of different variations, making it infeasible for each
run to be all that long.

Of course whether that's feasible depends on the workload (e.g. readonly
runs can be shorter than read/write runs).

Also note that in the case that made me look at this, you'd have to run
the test for *weeks* to drown out the performance difference that's
solely caused by difference in how long individual connects are
established. Partially because the "excluding connection establishing"
number is entirely broken, but also because fewer connections having
been established changes the performance so much.


I think we should also consider making pgbench actually use non-blocking
connection establishment. It seems pretty weird that that's the one
libpq operation where we don't? In particular for -C, with -c > -j,
that makes the results pretty meaningless.


> Adding a synchronization barrier should be simple enough, I thought about it
> in the past.
>
> However, I'd still be wary that it is no silver bullet: if you start a lot
> of threads compared to the number of available cores, pgbench would
> basically overload the system, and you would experience a lot of waiting
> time which reflects that the client code has not got enough cpu time.
> Basically you would be testing the OS process/thread management performance.

Sure, that's possible. But I don't see what that has to do with the
barrier?

Also, most scripts actually have client/server interaction...

Greetings,

Andres Freund

Hello Andres,

> FWIW, leaving windows, error handling, and other annoyances aside, this
> can be implemented fairly simply. See below.

Attached an attempt at improving things.

I've put 2 barriers: one so that all threads are up, one when all
connections are setup and the bench is ready to go.

I've done a blind attempt at implementing the barrier stuff on windows.

I've changed the performance calculations depending on -C or not. Ramp-up
effects are smoothed.

I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.

I've tried to do some variable renaming to distinguish timestamps and
intervals.

This is work in progress.

--
Fabien.

Hi,

On 2020-03-01 22:16:06 +0100, Fabien COELHO wrote:
>
> Hello Andres,
>
> > FWIW, leaving windows, error handling, and other annoyances aside, this
> > can be implemented fairly simply. See below.
>
> Attached an attempt at improving things.

Awesome!


> I've put 2 barriers: one so that all threads are up, one when all
> connections are setup and the bench is ready to go.

I'd done similarly locally.

Slight aside: Have you ever looked at moving pgbench to non-blocking
connection establishment? It seems weird to use non-blocking everywhere
but connection establishment.


> I've done a blind attempt at implementing the barrier stuff on windows.

Neat.


> I've changed the performance calculations depending on -C or not. Ramp-up
> effects are smoothed.


> I've merged all time-related stuff (time_t, instr_time, int64) to use a
> unique type (pg_time_usec_t) and set of functions/macros, which simplifies
> the code somehow.

Hm.  I'm not convinced it's a good idea for pgbench to do its own thing
here.


How about using 'struct unknown_type *unused' instead of "unused"?
Because the void *unused will accept everything...


> +/* Thread synchronization barriers */
> +static pthread_barrier_t
> + start_barrier, /* all threads are started */
> + bench_barrier; /* benchmarking ready to start */
> +

We don't really need two barriers here. The way that pthread barriers
are defined is that they 'reset' after all the threads have arrived. You
can argue we still want that, but ...



> @@ -5165,20 +5151,16 @@ printSimpleStats(const char *prefix, SimpleStats *ss)
>
>  /* print out results */
>  static void
> -printResults(StatsData *total, instr_time total_time,
> - instr_time conn_total_time, int64 latency_late)
> +printResults(StatsData *total,

Given that we're changing the output (for the better) of pgbench again,
I wonder if we should add the pgbench version to the benchmark
output. Otherwise it seems easy to end up e.g. seeing a performance
difference between pg12 and pg14, where all that's actually happening is
a different output because each run used the respective pgbench version.



> + pg_time_usec_t total_delay, /* benchmarking time */
> + pg_time_usec_t conn_total_delay, /* is_connect */
> + pg_time_usec_t conn_elapsed_delay, /* !is_connect */
> + int64 latency_late)

I'm not a fan of naming these 'delay'. To me that doesn't sounds like
it's about the time the total benchmark has taken.


Keeping these separate makes sense to me, they're just so wildly
different.


For me the function name sounds like you're getting the usec out of a
pg_time. Not that it's getting a new timestamp.


> +#define PG_TIME_SET_CURRENT_LAZY(t) \
> + if ((t) == 0) \
> + (t) = pg_time_get_usec()
> +
> +#define PG_TIME_GET_DOUBLE(t) (0.000001 * (t))
>  #endif /* INSTR_TIME_H */

I'd make it an inline function instead of this.

Greetings,

Andres Freund

Hello Andres,

> Slight aside: Have you ever looked at moving pgbench to non-blocking
> connection establishment? It seems weird to use non-blocking everywhere
> but connection establishment.

Nope. If there is some interest, why not. The reason for not doing it is
that the typical use case is just to connect once at the beginning so that
connections do not matter anyway. Now with -C it makes sense.

>> I've changed the performance calculations depending on -C or not. Ramp-up
>> effects are smoothed.
>
>> I've merged all time-related stuff (time_t, instr_time, int64) to use a
>> unique type (pg_time_usec_t) and set of functions/macros, which simplifies
>> the code somehow.
>
> Hm.  I'm not convinced it's a good idea for pgbench to do its own thing
> here.

Having 3 time types (in fact, 4, double is used as well for some
calculations) in just one file to deal with time does not help much to
understand the code, and there is quite a few line to translate from one
to the other.

>> +static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads);
>
> How about using 'struct unknown_type *unused' instead of "unused"?
> Because the void *unused will accept everything...

Never encountered this pattern. It does not seem to be used anywhere in pg
sources. I'd be afraid that some compilers would complain. I can try
anyway.

>> +/* Thread synchronization barriers */
>> +static pthread_barrier_t
>> + start_barrier, /* all threads are started */
>> + bench_barrier; /* benchmarking ready to start */
>> +
>
> We don't really need two barriers here. The way that pthread barriers
> are defined is that they 'reset' after all the threads have arrived. You
> can argue we still want that, but ...

Yes, one barrier could be reused.

>>  /* print out results */
>>  static void
>> -printResults(StatsData *total, instr_time total_time,
>> - instr_time conn_total_time, int64 latency_late)
>> +printResults(StatsData *total,
>
> Given that we're changing the output (for the better) of pgbench again,
> I wonder if we should add the pgbench version to the benchmark
> output.

Dunno. Maybe.

> Otherwise it seems easy to end up e.g. seeing a performance
> difference between pg12 and pg14, where all that's actually happening is
> a different output because each run used the respective pgbench version.

Yep.

>> + pg_time_usec_t total_delay, /* benchmarking time */
>> + pg_time_usec_t conn_total_delay, /* is_connect */
>> + pg_time_usec_t conn_elapsed_delay, /* !is_connect */
>> + int64 latency_late)
>
> I'm not a fan of naming these 'delay'. To me that doesn't sounds like
> it's about the time the total benchmark has taken.

Hmmm… I'd like to differentiate variable names which contain timestamp
versus those which contain intervals, given that it is the same underlying
type. That said, I'm not very happy with "delay" either.

What would you suggest?

>> +pg_time_get_usec(void)
>
> For me the function name sounds like you're getting the usec out of a
> pg_time. Not that it's getting a new timestamp.

Ok, I'll think of something else, possibly "pg_now"? "pg_time_now"?

>> +#define PG_TIME_SET_CURRENT_LAZY(t) \
>> + if ((t) == 0) \
>> + (t) = pg_time_get_usec()
>> +
>> +#define PG_TIME_GET_DOUBLE(t) (0.000001 * (t))
>>  #endif /* INSTR_TIME_H */
>
> I'd make it an inline function instead of this.

I did it that way because it was already done with defines on instr_time,
but I'm fine with inline.

I'll try to look at it over the week-end.

--
Fabien.

Hello Andres,

>> I've changed the performance calculations depending on -C or not. Ramp-up
>> effects are smoothed.
>
>> I've merged all time-related stuff (time_t, instr_time, int64) to use a
>> unique type (pg_time_usec_t) and set of functions/macros, which simplifies
>> the code somehow.
>
> Hm.  I'm not convinced it's a good idea for pgbench to do its own thing
> here.

Given the unjustifiable heterogeneousness it induces and the simpler code
after the move, I think it is much better. Pgbench cloc is smaller after
barrier are added (4655 to 4650) thanks to that and a few other code
simplifications. Removing all INSTR_TIME_* costly macros is a relief in
itself…

>> +static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads);
>
> How about using 'struct unknown_type *unused' instead of "unused"?

Haven't done it because I found no other instances in pg, and anyway this
code is only used once locally and NULL is passed.

>> +static pthread_barrier_t
>> + start_barrier, /* all threads are started */
>> + bench_barrier; /* benchmarking ready to start */
>
> We don't really need two barriers here.

Indeed. Down to one.

>>  /* print out results */
>
> Given that we're changing the output (for the better) of pgbench again,
> I wonder if we should add the pgbench version to the benchmark
> output.

Not sure about it, but done anyway.

>> + pg_time_usec_t total_delay, /* benchmarking time */
>> + pg_time_usec_t conn_total_delay, /* is_connect */
>> + pg_time_usec_t conn_elapsed_delay, /* !is_connect */
>> + int64 latency_late)
>
> I'm not a fan of naming these 'delay'. To me that doesn't sounds like
> it's about the time the total benchmark has taken.

I have used '_duration', and tried to clarify some field and variable
names depending on what data they actually hold.

>> + printf("tps = %f (including reconnection times)\n", tps);
>> + printf("tps = %f (without initial connection establishing)\n", tps);
>
> Keeping these separate makes sense to me, they're just so wildly
> different.

Yep. I've added a comment about that.

>> +static inline pg_time_usec_t
>> +pg_time_get_usec(void)
>
> For me the function name sounds like you're getting the usec out of a
> pg_time. Not that it's getting a new timestamp.

I've used "pg_time_now()".

>> +#define PG_TIME_SET_CURRENT_LAZY(t) \
>> + if ((t) == 0) \
>> + (t) = pg_time_get_usec()
>
> I'd make it an inline function instead of this.

Done "pg_time_now_lazy(&now)"

I have also simplified the code around thread creation & join because it
was a mess: thread 0 was run in the middle of the stat collection loop…

I have updated the doc with actual current output, but excluding the
version display which would have to be changed between releases.

--
Fabien.

Hallo Andres,

> Slight aside: Have you ever looked at moving pgbench to non-blocking
> connection establishment? It seems weird to use non-blocking everywhere
> but connection establishment.

Attached an attempt at doing that, mostly done for fun. It seems to be a
little slower on a local socket.

What do you think?

Maybe it would be worth having it with an option?

--
Fabien.

Hello,

>>> I've merged all time-related stuff (time_t, instr_time, int64) to use a
>>> unique type (pg_time_usec_t) and set of functions/macros, which simplifies
>>> the code somehow.
>>
>> Hm.  I'm not convinced it's a good idea for pgbench to do its own thing
>> here.

I really think that the refactoring part is a good thing because cloc and
cost is reduced (time arithmetic is an ugly pain with instr_time).

I have split the patch.

* First patch reworks time measurements in pgbench.

It creates a convenient pg_time_usec_t and use it everywhere, getting rid
of "instr_time_t". The code is somehow simplified wrt what time are taken
and what they mean.

Instead of displaying 2 tps at the end, which is basically insane, it
shows one tps for --connect, which includes reconnection times, and one
tps for the usual one connection at startup which simply ignores the
initial connection time.

This (mostly) refactoring reduces the cloc.

* Second patch adds a barrier before starting the bench

It applies on top of the previous one. The initial imbalance due to thread
creation times is smoothed.

I may add a --start-on option afterwards so that several pgbench (running
on distinct hosts) can be synchronized, which would be implemented as a
delay inserted by thread 0 before the barrier.

The windows implementation is more or less blind, if someone can confirm
that it works, it would be nice.

--
Fabien.

> On 17 May 2020, at 11:55, Fabien COELHO <[hidden email]> wrote:

> I have split the patch.
>
> * First patch reworks time measurements in pgbench.

> * Second patch adds a barrier before starting the bench
>
> It applies on top of the previous one. The initial imbalance due to thread creation times is smoothed.

The usecs patch fails to apply to HEAD, can you please submit a rebased version
of this patchset.  Also, when doing so, can you please rename the patches such
that sort alphabetically in the order in which they are intended to be applied.
The CFBot patch tester will otherwise try to apply them out of order which
cause errors.

cheers ./daniel

Ok. Attached.

--
Fabien.

Dear Fabien, Andres

I think your idea is good, hence I put some comments as a reviewer.
I focus on only the linux code because I'm not familiar with the Windows system. Sorry.

[For patch A]

Please complete fixes for the documentation. At least the following sentence should be fixed:
```
The last two lines report the number of transactions per second, figured with and without counting the time to start database sessions.
```

> -starting vacuum...end.

I think any other options should be disabled in the example, therefore please leave this line.

> +       /* explicitly initialize the state machines */
> +       for (int i = 0; i < nstate; i++)
> +       {
> +               state[i].state = CSTATE_CHOOSE_SCRIPT;
> +       }

I'm not sure but I think braces should be removed in our coding conventions.

Other changes are being reviewed now.

[For patch B]

> +       /* GO */
> +       pthread_barrier_wait(&barrier);

The current implementation is too simple. If nthreads >= 2 and connection fails in the one thread,
the other one will wait forever.
Some special treatments are needed in the `done` code block and here.


[others]

> > (that is, it can be disabled)
>
> On reflection, I'm not sure I see a use case for not running the barrier
> if it is available.

If the start point changes and there is no way to disable this feature,
the backward compatibility will be completely violated.
It means that tps cannot be compared to older versions under the same conditions,
and It may hide performance-related issues.
I think it's not good.


Best regards,
Hayato Kuroda
FUJITSU LIMITED

-----Original Message-----
From: Fabien COELHO <[hidden email]>
Sent: Saturday, July 4, 2020 3:34 PM
To: Daniel Gustafsson <[hidden email]>
Cc: Andres Freund <[hidden email]>; [hidden email]
Subject: Re: pgbench: option delaying queries till connections establishment?


Ok. Attached.

--
Fabien.

Dear Fabien;

> The current implementation is too simple. If nthreads >= 2 and connection fails in the one thread,
> the other one will wait forever.

I attached the very preliminary patch for solving the problem.
Even if threads fail to connect, the others can go through the barrier.
But I think this implementation is not good...


Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hello,

> Please complete fixes for the documentation. At least the following sentence should be fixed:
> ```
> The last two lines report the number of transactions per second, figured with and without counting the time to start database sessions.
> ```

Indeed. I scanned the file but did not find other places that needed
updating.

>> -starting vacuum...end.
>
> I think any other options should be disabled in the example, therefore please leave this line.

Yes.

>> +       for (int i = 0; i < nstate; i++)
>> +       {
>> +               state[i].state = CSTATE_CHOOSE_SCRIPT;
>> +       }
>
> I'm not sure but I think braces should be removed in our coding conventions.

Not sure either. I'm not for having too many braces anyway, so I removed
them.

>> +       /* GO */
>> +       pthread_barrier_wait(&barrier);
>
> The current implementation is too simple. If nthreads >= 2 and connection fails in the one thread,
> the other one will wait forever.
> Some special treatments are needed in the `done` code block and here.

Indeed. I took your next patch with an added explanation. I'm unclear
whether proceeding makes much sense though, that is some thread would run
the test and other would have aborted. Hmmm.
ISTM that there is another patch in the queue which needs barriers to
delay some initialization so as to fix a corner case bug, in which case
the behavior would be mandatory. The current submission could add an
option to skip the barrier synchronization, but I'm not enthousiastic to
add an option and remove it shortly later.

Moreover, the "compatibility" is with nothing which does not make much
sense. When testing with many threads and clients, the current
implementation make threads start when they are ready, which means that
you can have threads issuing transactions while others are not yet
connected or not even started, so that the actually measured performance
is quite awkward for a short bench. ISTM that allowing such a backward
compatible strange behavior does not serve pg users. If the user want the
old unreliable behavior, they can use a old pgbench, and obtain unreliable
figures.

For these two reasons, I'm inclined not to add an option to skip these
barriers, but this can be debatted further.

Attached 2 updated patches.

--
Fabien.

Dear Fabien,

> Indeed. I scanned the file but did not find other places that needed
> updating.

> Yes.

> Not sure either. I'm not for having too many braces anyway, so I removed
> them.

I checked your fixes and I think it's OK.
Finally, please move the doc fixes to patch B in order to separate patches
completely.

> Indeed. I took your next patch with an added explanation. I'm unclear
> whether proceeding makes much sense though, that is some thread would run
> the test and other would have aborted. Hmmm.

Your comment looks good, thanks.
In the previous version pgbench starts benchmarking even if some connections fail.
And users can notice the connection failure by stderr output.
Hence the current specification may be enough.
If you agree, please remove the following lines:

```
+ * It is unclear whether it is worth doing anything rather than
+ * coldly exiting with an error message.
```

> ISTM that there is another patch in the queue which needs barriers to
> delay some initialization so as to fix a corner case bug, in which case
> the behavior would be mandatory. The current submission could add an
> option to skip the barrier synchronization, but I'm not enthousiastic to
> add an option and remove it shortly later.

Could you tell me which patch you mention? Basically I agree what you say,
but I want to check it.

Hayato Kuroda
FUJITSU LIMITED

Hello,

>> Indeed. I took your next patch with an added explanation. I'm unclear
>> whether proceeding makes much sense though, that is some thread would run
>> the test and other would have aborted. Hmmm.
>
> Your comment looks good, thanks. In the previous version pgbench starts
> benchmarking even if some connections fail. And users can notice the
> connection failure by stderr output. Hence the current specification may
> be enough.

Usually I run many pgbench through scripts, so I'm probably not there to
check a lone stderr failure at the beginning if performance figures are
actually reported.

> If you agree, please remove the following lines:
>
> ```
> + * It is unclear whether it is worth doing anything rather than
> + * coldly exiting with an error message.
> ```

I can remove the line, but I strongly believe that reporting performance
figures if some client connection failed thus the bench could not run as
prescribed is a bad behavior. The good news is that it is probably quite
unlikely. So I'd prefer to keep it and possibly submit a patch to change
the behavior.

>> ISTM that there is another patch in the queue which needs barriers to
>> delay some initialization so as to fix a corner case bug, in which case
>> the behavior would be mandatory. The current submission could add an
>> option to skip the barrier synchronization, but I'm not enthousiastic to
>> add an option and remove it shortly later.
>
> Could you tell me which patch you mention? Basically I agree what you say,
> but I want to check it.

Should be this one: https://commitfest.postgresql.org/30/2624/,

--
Fabien.

Dear Fabien,

> Usually I run many pgbench through scripts, so I'm probably not there to
> check a lone stderr failure at the beginning if performance figures are
> actually reported.

> I can remove the line, but I strongly believe that reporting performance
> figures if some client connection failed thus the bench could not run as
> prescribed is a bad behavior. The good news is that it is probably quite
> unlikely. So I'd prefer to keep it and possibly submit a patch to change
> the behavior.

I agree such a situation is very bad, and I understood you have a plan to
submit patches for fix it. If so leaving lines as a TODO is OK.

> Should be this one: https://commitfest.postgresql.org/30/2624/

This discussion is still on-going, but I can see that the starting time
may be delayed for looking up all pgbench-variables.
(I think the status of this thread might be wrong. it should be
'Needs review,' but now 'Waiting on Author.')

This patch is mostly good and can change a review status soon,
however, I think it should wait that related patch.
Please discuss how to fix it with Tom, and this will commit soon.

Hayato Kuroda
FUJITSU LIMITED