Thread: 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 <andres@anarazel.de> wrote in 
> 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!

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

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,

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

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.


>
>  #ifdef WIN32
> +#define PTHREAD_BARRIER_SERIAL_THREAD (-1)
> +
>  /* Use native win32 threads on Windows */
>  typedef struct win32_pthread *pthread_t;
>  typedef int pthread_attr_t;
> +typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
>
>  static int    pthread_create(pthread_t *thread, pthread_attr_t *attr, void *(*start_routine) (void *), void *arg);
>  static int    pthread_join(pthread_t th, void **thread_return);
> +
> +static int    pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads);
> +static int    pthread_barrier_wait(pthread_barrier_t *barrier);
> +static int    pthread_barrier_destroy(pthread_barrier_t *barrier);

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.


> @@ -5239,8 +5220,16 @@ printResults(StatsData *total, instr_time total_time,
>                 0.001 * total->lag.sum / total->cnt, 0.001 * total->lag.max);
>      }
>
> -    printf("tps = %f (including connections establishing)\n", tps_include);
> -    printf("tps = %f (excluding connections establishing)\n", tps_exclude);
> +    if (is_connect)
> +    {
> +        printf("average connection time = %.3f ms\n", 0.001 * conn_total_delay / total->cnt);
> +        printf("tps = %f (including reconnection times)\n", tps);
> +    }
> +    else
> +    {
> +        printf("initial connection time = %.3f ms\n", 0.001 * conn_elapsed_delay);
> +        printf("tps = %f (without initial connection establishing)\n", tps);
> +    }

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


> +/*
> + * Simpler convenient interface
> + *
> + * The instr_time type is expensive when dealing with time arithmetic.
> + * Define a type to hold microseconds on top of this, suitable for
> + * benchmarking performance measures, eg in "pgbench".
> + */
> +typedef int64 pg_time_usec_t;
> +
> +static inline pg_time_usec_t
> +pg_time_get_usec(void)
> +{
> +    instr_time now;
> +
> +    INSTR_TIME_SET_CURRENT(now);
> +    return (pg_time_usec_t) INSTR_TIME_GET_MICROSEC(now);
> +}

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 <coelho@cri.ensmp.fr> 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

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

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
databasesessions. 
```

> -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 <coelho@cri.ensmp.fr>
Sent: Saturday, July 4, 2020 3:34 PM
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Andres Freund <andres@anarazel.de>; pgsql-hackers@postgresql.org
Subject: Re: pgbench: option delaying queries till connections establishment?


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

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
databasesessions.
 
> ```

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.

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

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

Hello,

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

Thanks.

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

Yep, that's it.

> (I think the status of this thread might be wrong. it should be
> 'Needs review,' but now 'Waiting on Author.')

I changed it to "Needs review".

> This patch is mostly good and can change a review status soon,
> however, I think it should wait that related patch.

Hmmm.

> Please discuss how to fix it with Tom,

I would not have the presumption to pressure Tom's agenda in any way!

> and this will commit soon.

and this will wait till its time comes. In the mean time, I think that you 
should put the patch status as you see fit, independently of the other 
patch: it seems unlikely that they would be committed together, and I'll 
have to merge the remaining one anyway.

-- 
Fabien.

Dear Fabien,

> and this will wait till its time comes. In the mean time, I think that you
> should put the patch status as you see fit, independently of the other
> patch: it seems unlikely that they would be committed together, and I'll
> have to merge the remaining one anyway.

OK. I found the related thread[1], and I understood you will submit another patch
on the thread.

PostgreSQL Patch Tester says all regression tests are passed, and
I change the status to "Ready for committer."

[1]: https://commitfest.postgresql.org/31/2827/

Thank you for discussing with me.

Hayato Kuroda
FUJITSU LIMITED

-----Original Message-----
From: Fabien COELHO <coelho@cri.ensmp.fr>
Sent: Wednesday, November 11, 2020 9:24 PM
To: Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com>
Cc: Andres Freund <andres@anarazel.de>; Daniel Gustafsson <daniel@yesql.se>; pgsql-hackers@postgresql.org
Subject: RE: pgbench: option delaying queries till connections establishment?


Hello,

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

Thanks.

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

Yep, that's it.

> (I think the status of this thread might be wrong. it should be
> 'Needs review,' but now 'Waiting on Author.')

I changed it to "Needs review".

> This patch is mostly good and can change a review status soon,
> however, I think it should wait that related patch.

Hmmm.

> Please discuss how to fix it with Tom,

I would not have the presumption to pressure Tom's agenda in any way!

> and this will commit soon.

and this will wait till its time comes. In the mean time, I think that you
should put the patch status as you see fit, independently of the other
patch: it seems unlikely that they would be committed together, and I'll
have to merge the remaining one anyway.

--
Fabien.

Hello!

On 2020-11-13 08:44, kuroda.hayato@fujitsu.com wrote:
> Dear Fabien,
> 
>> and this will wait till its time comes. In the mean time, I think that 
>> you
>> should put the patch status as you see fit, independently of the other
>> patch: it seems unlikely that they would be committed together, and 
>> I'll
>> have to merge the remaining one anyway.
> 
> OK. I found the related thread[1], and I understood you will submit
> another patch
> on the thread.
> 
> PostgreSQL Patch Tester says all regression tests are passed, and
> I change the status to "Ready for committer."
> 
> [1]: https://commitfest.postgresql.org/31/2827/
> 
> Thank you for discussing with me.
> 
> Hayato Kuroda
> FUJITSU LIMITED

 From the mentioned thread [2]:

>>> While trying to test a patch that adds a synchronization barrier in 
>>> pgbench [1] on Windows,
>> 
>> Thanks for trying that, I do not have a windows setup for testing, and
>> the sync code I wrote for Windows is basically blind coding:-(
> 
> FYI:
> 
> 1) It looks like pgbench will no longer support Windows XP due to the
> function DeleteSynchronizationBarrier. From
> https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-deletesynchronizationbarrier
> :
> 
> Minimum supported client: Windows 8 [desktop apps only]
> Minimum supported server: Windows Server 2012 [desktop apps only]
> 
> On Windows Server 2008 R2 (MSVC 2013) the 6-th version of the patch
> [1] has compiled without (new) warnings, but when running pgbench I
> got the following error:
> 
> The procedure entry point DeleteSynchronizationBarrier could not be
> located in the dynamic link library KERNEL32.dll.

IMO, it looks like either old Windows systems should not call new 
functions, or we should throw them a compilation error. (Update 
MIN_WINNT to 0x0602 = Windows 8 in src/include/port/win32.h?) In the 
second case it looks like the documentation should be updated too, see 
doc/src/sgml/installation.sgml:

<para>
  <productname>PostgreSQL</productname> can be expected to work on these 
operating
  systems: Linux (all recent distributions), Windows (XP and later),
  FreeBSD, OpenBSD, NetBSD, macOS, AIX, HP/UX, and Solaris.
  Other Unix-like systems may also work but are not currently
  being tested.  In most cases, all CPU architectures supported by
  a given operating system will work.  Look in
  <xref linkend="installation-platform-notes"/> below to see if
  there is information
  specific to your operating system, particularly if using an older 
system.
</para>

<...>

<para>
  The native Windows port requires a 32 or 64-bit version of Windows
  2000 or later. Earlier operating systems do
  not have sufficient infrastructure (but Cygwin may be used on
  those).  MinGW, the Unix-like build tools, and MSYS, a collection
  of Unix tools required to run shell scripts
  like <command>configure</command>, can be downloaded
  from <ulink url="http://www.mingw.org/"></ulink>.  Neither is
  required to run the resulting binaries; they are needed only for
  creating the binaries.
</para>

[2] 
https://www.postgresql.org/message-id/e5a09b790db21356376b6e73673aa07c%40postgrespro.ru

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello Marina,

>> 1) It looks like pgbench will no longer support Windows XP due to the
>> function DeleteSynchronizationBarrier. From
>> https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-deletesynchronizationbarrier
>>
>> Minimum supported client: Windows 8 [desktop apps only]
>> Minimum supported server: Windows Server 2012 [desktop apps only]

Thanks for the test and precise analysis!

Sigh.

I do not think that putting such version requirements are worth it just 
for the sake of pgbench.

In the attached version, I just comment out the call and add an 
explanation about why it is commented out. If pg overall version 
requirements are changed on windows, then it could be reinstated.

-- 
Fabien.

Hi!

On Thu, Feb 27, 2020 at 9:01 PM Andres Freund <andres@anarazel.de> wrote:
> 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).

Hmm...  Let's see the big picture.  You've recently committed a
patchset, which greatly improved the performance of GetSnapshotData().
And you're making further improvements in this direction.  But you're
getting trouble in measuring the effect, because Postgres is still
stuck on ProcArrayLock.  And in this thread you propose a workaround
for that implemented on the pgbench side.  My very dumb idea is
following: should we finally give a chance to more fair lwlocks rather
than inventing workarounds?

As I remember, your major argument against more fair lwlocks was the
idea that we should fix lwlocks use-cases rather than lwlock mechanism
themselves.  But can we expect that we fix all the lwlocks use-case in
any reasonable prospect?  My guess is 'no'.

Links
1. https://www.postgresql.org/message-id/CAPpHfdvJhO1qutziOp%3Ddy8TO8Xb4L38BxgKG4RPa1up1Lzh_UQ%40mail.gmail.com

------
Regards,
Alexander Korotkov

Hello!

On 2020-11-14 20:07, Alexander Korotkov wrote:
> Hmm...  Let's see the big picture.  You've recently committed a
> patchset, which greatly improved the performance of GetSnapshotData().
> And you're making further improvements in this direction.  But you're
> getting trouble in measuring the effect, because Postgres is still
> stuck on ProcArrayLock.  And in this thread you propose a workaround
> for that implemented on the pgbench side.  My very dumb idea is
> following: should we finally give a chance to more fair lwlocks rather
> than inventing workarounds?
> 
> As I remember, your major argument against more fair lwlocks was the
> idea that we should fix lwlocks use-cases rather than lwlock mechanism
> themselves.  But can we expect that we fix all the lwlocks use-case in
> any reasonable prospect?  My guess is 'no'.
> 
> Links
> 1.
> https://www.postgresql.org/message-id/CAPpHfdvJhO1qutziOp%3Ddy8TO8Xb4L38BxgKG4RPa1up1Lzh_UQ%40mail.gmail.com

Sorry I'm not familiar with the internal architecture of snapshots, 
locks etc. in postgres, but I wanted to ask - what exact kind of patch 
for fair lwlocks do you want to offer to the community? I applied the 
6-th version of the patch for fair lwlocks from [1] to the old master 
branch (see commit [2]), started many threads in pgbench (-M prepared -c 
1000 -j 500 -T 10 -P1 -S) and I did not receive stable first progress 
reports, which IIUC are one of the advantages of the discussed patch for 
the pgbench (see [3])...

[1] 
https://www.postgresql.org/message-id/CAPpHfduV3v3EG7K74-9htBZz_mpE993zGz-%3D2k5RNA3tqabUAA%40mail.gmail.com
[2] 
https://github.com/postgres/postgres/commit/84d514887f9ca673ae688d00f8b544e70f1ab270
[3] 
https://www.postgresql.org/message-id/20200227185129.hikscyenomnlrord%40alap3.anarazel.de

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2020-11-14 20:07:38 +0300, Alexander Korotkov wrote:
> Hmm...  Let's see the big picture.  You've recently committed a
> patchset, which greatly improved the performance of GetSnapshotData().
> And you're making further improvements in this direction.  But you're
> getting trouble in measuring the effect, because Postgres is still
> stuck on ProcArrayLock.

No, the problem was that I couldn't measure the before/after behaviour
reliably, because not all connections actually ever get established
*before* the GetSnapshotData() scability patchset. Which made the
numbers pointless, because we'd often end up with e.g. 80 connections
doing work pre-patch, and 800 post-patch; which obviously measures very
different things.

I think the issue really is that, independent of PG lock contention,
it'll take a while to establish all connections, and that starting to
benchmark with only some connections established will create pretty
pointless numbers.


> And in this thread you propose a workaround
> for that implemented on the pgbench side.  My very dumb idea is
> following: should we finally give a chance to more fair lwlocks rather
> than inventing workarounds?

Perhaps - I just don't think it's related to this thread. And how you're
going to address the overhead.

Greetings,

Andres Freund

Hi,

On 2020-11-17 00:09:34 +0300, Marina Polyakova wrote:
> Sorry I'm not familiar with the internal architecture of snapshots, locks
> etc. in postgres, but I wanted to ask - what exact kind of patch for fair
> lwlocks do you want to offer to the community? I applied the 6-th version of
> the patch for fair lwlocks from [1] to the old master branch (see commit
> [2]), started many threads in pgbench (-M prepared -c 1000 -j 500 -T 10 -P1
> -S) and I did not receive stable first progress reports, which IIUC are one
> of the advantages of the discussed patch for the pgbench (see [3])...

Thanks for running some benchmarks.

I think it's quite unsurprising that you'd see skewed numbers initially,
even with fairer locks. Just by virtue of pgbench starting threads and
each thread immediately starting to perform work, you are bound to get
odd pretty meaningless initial numbers. Even without contention, and
when using fewer connections than there are CPUs. And especially when
starting a larger number of connections, because the main pgbench thread
will get fewer and fewer scheduler slices because of the threads running
benchmarks already.

Regards,

Andres

> I think the issue really is that, independent of PG lock contention,
> it'll take a while to establish all connections, and that starting to
> benchmark with only some connections established will create pretty
> pointless numbers.

Yes. This is why I think that if we have some way to synchronize it should 
always be used, i.e. not an option.

-- 
Fabien.

On Sun, Nov 15, 2020 at 4:53 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> In the attached version, I just comment out the call and add an
> explanation about why it is commented out. If pg overall version
> requirements are changed on windows, then it could be reinstated.

It looks like macOS doesn't have pthread barriers (via cfbot 2021, now
with more operating systems):

pgbench.c:326:8: error: unknown type name 'pthread_barrier_t'
static pthread_barrier_t barrier;
^
pgbench.c:6128:2: error: implicit declaration of function
'pthread_barrier_init' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
pthread_barrier_init(&barrier, NULL, nthreads);
^

> It looks like macOS doesn't have pthread barriers (via cfbot 2021, now
> with more operating systems):

Indeed:-(

I'll look into that.

-- 
Fabien.

On Sat, Jan 2, 2021 at 9:15 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > It looks like macOS doesn't have pthread barriers (via cfbot 2021, now
> > with more operating systems):
>
> Indeed:-(
>
> I'll look into that.

Just for fun, the attached 0002 patch is a quick prototype of a
replacement for that stuff that seems to work OK on a Mac here.  (I'm
not sure if the Windows part makes sense or works.)

>>> It looks like macOS doesn't have pthread barriers (via cfbot 2021, now
>>> with more operating systems):
>>
>> Indeed:-(
>>
>> I'll look into that.
>
> Just for fun, the attached 0002 patch is a quick prototype of a
> replacement for that stuff that seems to work OK on a Mac here.  (I'm
> not sure if the Windows part makes sense or works.)

Thanks! That will definitely help because I do not have a Mac. I'll do 
some cleanup.

-- 
Fabien.

On Sun, Jan 3, 2021 at 9:49 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > Just for fun, the attached 0002 patch is a quick prototype of a
> > replacement for that stuff that seems to work OK on a Mac here.  (I'm
> > not sure if the Windows part makes sense or works.)
>
> Thanks! That will definitely help because I do not have a Mac. I'll do
> some cleanup.

I think the main things to clean up are:

1.  pthread_barrier_init() should check for errors from
pthread_cond_init() and pthread_mutex_init(), and return -1.
2.  pthread_barrier_destroy() should call pthread_cond_destroy() and
pthread_mutex_destroy().
3 . Decide if it's sane for the Windows-based emulation to be in here
too, or if it should stay in pgbench.c.  Or alternatively, if we're
emulating pthread stuff on Windows, why not also put the other pthread
emulation stuff from pgbench.c into a "ports" file; that seems
premature and overkill for your project.  I dunno.
4.  cfbot shows that it's not building on Windows because
HAVE_PTHREAD_BARRIER_WAIT is missing from Solution.pm.

As far as I know, it's OK and common practice to ignore the return
code from eg pthread_mutex_lock() and pthread_cond_wait(), with
rationale along the lines that there'd have to be a programming error
for them to fail in simple cases.

Unfortunately, cfbot can only tell us that it's building OK on a Mac,
but doesn't actually run the pgbench code to reach this stuff.  It's
not running check-world on that platform yet for the following asinine
reason:

connection to database failed: Unix-domain socket path
"/private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/src/bin/pg_upgrade/.s.PGSQL.58080"
is too long (maximum 103 bytes)

On Sat, Jan 9, 2021 at 8:13 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Sun, Jan 3, 2021 at 9:49 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > > Just for fun, the attached 0002 patch is a quick prototype of a
> > > replacement for that stuff that seems to work OK on a Mac here.  (I'm
> > > not sure if the Windows part makes sense or works.)
> >
> > Thanks! That will definitely help because I do not have a Mac. I'll do
> > some cleanup.
>
> I think the main things to clean up are:

I’m supposed to be on vacation this week, but someone left a shiny new
Arm Mac laptop near me, so here’s a cleaned up version.

> 1.  pthread_barrier_init() should check for errors from
> pthread_cond_init() and pthread_mutex_init(), and return -1.

Done.

> 2.  pthread_barrier_destroy() should call pthread_cond_destroy() and
> pthread_mutex_destroy().

Done.

> 3 . Decide if it's sane for the Windows-based emulation to be in here
> too, or if it should stay in pgbench.c.  Or alternatively, if we're
> emulating pthread stuff on Windows, why not also put the other pthread
> emulation stuff from pgbench.c into a "ports" file; that seems
> premature and overkill for your project.  I dunno.

I decided to solve only the macOS problem for now.  So in this
version, the A and B patches are exactly as you had them in your v7,
except that B includes “port/pg_pthread.h” instead of <pthread.h>.

Maybe it’d make sense to move the Win32 pthread emulation stuff out of
pgbench.c into src/port too (the pre-existing stuff, and the new
barrier stuff you added), but that seems like a separate patch, one
that I’m not best placed to write, and it’s not clear to me that we’ll
want to be using pthread APIs as our main abstraction if/when thread
usage increases in the PG source tree anyway.  Other opinions welcome.

> 4.  cfbot shows that it's not building on Windows because
> HAVE_PTHREAD_BARRIER_WAIT is missing from Solution.pm.

Fixed, I think.

Hello Thomas,

>> 3 . Decide if it's sane for the Windows-based emulation to be in here
>> too, or if it should stay in pgbench.c.  Or alternatively, if we're
>> emulating pthread stuff on Windows, why not also put the other pthread
>> emulation stuff from pgbench.c into a "ports" file; that seems
>> premature and overkill for your project.  I dunno.
>
> I decided to solve only the macOS problem for now.  So in this
> version, the A and B patches are exactly as you had them in your v7,
> except that B includes “port/pg_pthread.h” instead of <pthread.h>.
>
> Maybe it’d make sense to move the Win32 pthread emulation stuff out of
> pgbench.c into src/port too (the pre-existing stuff, and the new
> barrier stuff you added), but that seems like a separate patch, one
> that I’m not best placed to write, and it’s not clear to me that we’ll
> want to be using pthread APIs as our main abstraction if/when thread
> usage increases in the PG source tree anyway.  Other opinions welcome.

I think it would be much more consistent to move all the thread complement 
stuff there directly: Currently (v8) the windows implementation is in 
pgbench and the MacOS implementation in port, which is quite messy.

Attached is a patch set which does that. I cannot test it neither on 
Windows nor on MacOS. Path 1 & 2 are really independent.

-- 
Fabien.

Hello Dave,

>> I am running pgbench with the following
>> pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench
>> --protocol=simple
>>
>> Without pgbouncer I get around 5k TPS
>> with pgbouncer I get around 15k TPS
>>
>> Looking at the code connection initiation time should not be part of the
>> calculation so I' puzzled why pgbouncer is making such a dramatic
>> difference ?
>
> Turns out that for this specific test, pg is faster with a pooler.

This does not tell "why".

Does the pooler prepares statements, whereas "simple" does not?

-- 
Fabien.