Horton

Thread

Commits

  1. Fix new pthread code to respect --disable-thread-safety.

  2. Add missing pthread_barrier_t.

  3. pgbench: Improve time logic.

  4. pgbench: Refactor thread portability support.

  5. pgbench: Synchronize client threads.

  6. Fix bogus logic for skipping unnecessary partcollation dependencies.

  1. pgbench: option delaying queries till connections establishment?

    Andres Freund <andres@anarazel.de> — 2020-02-27T18:01:00Z

    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

  2. Re: pgbench: option delaying queries till connections establishment?

    Andres Freund <andres@anarazel.de> — 2020-02-27T18:51:29Z

    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

  3. Re: pgbench: option delaying queries till connections establishment?

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-02-28T06:00:39Z

    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

  4. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-02-29T14:29:19Z

    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.

  5. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-02-29T14:39:06Z

    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.

  6. Re: pgbench: option delaying queries till connections establishment?

    Andres Freund <andres@anarazel.de> — 2020-02-29T17:37:45Z

    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

  7. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-03-01T21:16:06Z

    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.

  8. Re: pgbench: option delaying queries till connections establishment?

    Andres Freund <andres@anarazel.de> — 2020-03-04T17:40:03Z

    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

  9. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-03-05T22:55:04Z

    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.

  10. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-03-07T08:24:43Z

    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.

  11. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-03-07T08:30:31Z

    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.

  12. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-05-17T09:55:43Z

    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.

  13. Re: pgbench: option delaying queries till connections establishment?

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-03T09:21:17Z

    > 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

  14. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-07-04T06:34:25Z

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

  15. RE: pgbench: option delaying queries till connections establishment?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2020-10-26T08:31:32Z

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

  16. RE: pgbench: option delaying queries till connections establishment?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2020-10-28T09:28:35Z

    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

  17. RE: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-11-02T18:59:11Z

    
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.

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

  18. RE: pgbench: option delaying queries till connections establishment?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2020-11-06T02:15:10Z

    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

  19. RE: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-11-07T17:33:27Z

    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.

  20. RE: pgbench: option delaying queries till connections establishment?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2020-11-11T11:11:48Z

    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

  21. RE: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-11-11T12:23:46Z

    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.

  22. RE: pgbench: option delaying queries till connections establishment?

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2020-11-13T05:44:56Z

    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.

  23. Re: pgbench: option delaying queries till connections establishment?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-11-14T13:48:51Z

    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

  24. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-11-14T15:53:17Z

    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.

  25. Re: pgbench: option delaying queries till connections establishment?

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-11-14T17:07:38Z

    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

  26. Re: pgbench: option delaying queries till connections establishment?

    Marina Polyakova <m.polyakova@postgrespro.ru> — 2020-11-16T21:09:34Z

    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

  27. Re: pgbench: option delaying queries till connections establishment?

    Andres Freund <andres@anarazel.de> — 2020-11-16T21:32:03Z

    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

  28. Re: pgbench: option delaying queries till connections establishment?

    Andres Freund <andres@anarazel.de> — 2020-11-16T21:53:51Z

    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

  29. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-11-17T05:58:35Z

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

  30. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2020-12-31T19:10:55Z

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

  31. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2021-01-01T20:15:07Z

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

Indeed:-(

I'll look into that.

-- 
Fabien.

  32. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-01-02T09:50:55Z

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

  33. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2021-01-02T20:49:01Z

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

  34. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-01-08T19:13:17Z

    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)

  35. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-01-17T21:54:38Z

    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.

  36. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2021-01-30T12:17:57Z

    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.

  37. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-03T05:23:13Z

    On Sun, Jan 31, 2021 at 1:18 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> 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.

Hmm.  Well this is totally subjective, but here's how I see this after
thinking about it a bit more:  macOS does actually have POSIX threads,
except for this tiny missing piece, so it's OK to write a toy
implementation that is reasonably conformant, and put it in there
using the usual AC_REPLACE_FUNCS machinery.  It will go away when
Apple eventually adds a real one.  Windows does not, and here we're
writing a very partial toy implementation that is far from conformant.
I think that's OK for pgbench's purposes, for now, but I'd prefer to
keep it inside pgbench.c.  I think at some point in the (hopefully not
too distant) future, we'll start working on thread support for the
backend, and then I think we'll probably come up with our own
abstraction over Windows and POSIX threads, rather than trying to use
POSIX API wrappers from Windows, so I don't really want this stuff in
the port library.  Does this make some kind of sense?

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

No worries.  For some reason I have a lot of computers; I'll try to
get this (or rather a version with the Windows stuff moved back)
passing on all of them soon, with the aim of making it committable.

  38. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-04T09:44:11Z

    On Wed, Mar 3, 2021 at 6:23 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Sun, Jan 31, 2021 at 1:18 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> > 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.
>
> Hmm.  Well this is totally subjective, but here's how I see this after
> thinking about it a bit more:  macOS does actually have POSIX threads,
> except for this tiny missing piece, so it's OK to write a toy
> implementation that is reasonably conformant, and put it in there
> using the usual AC_REPLACE_FUNCS machinery.  It will go away when
> Apple eventually adds a real one.  Windows does not, and here we're
> writing a very partial toy implementation that is far from conformant.
> I think that's OK for pgbench's purposes, for now, but I'd prefer to
> keep it inside pgbench.c.  I think at some point in the (hopefully not
> too distant) future, we'll start working on thread support for the
> backend, and then I think we'll probably come up with our own
> abstraction over Windows and POSIX threads, rather than trying to use
> POSIX API wrappers from Windows, so I don't really want this stuff in
> the port library.  Does this make some kind of sense?

Here is an attempt to move things in that direction.  It compiles
tests OK on Unix including macOS with and without
--disable-thread-safety, and it compiles on Windows (via CI) but I
don't yet know if it works there.

v10-0001-Add-missing-pthread_barrier_t.patch

Same as v8.  Adds the missing pthread_barrier_t support for macOS
only.  Based on the traditional configure symbol probe for now.  It's
possible that we'll later decide to use declarations to be more
future-proof against Apple's API versioning strategy, but I don't have
one of those weird cross-version compiler setups to investigate that
(see complaints from James Hilliard about the way we deal with
pwrite()).

v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch

New.  Abandons the concept of doing a fake pthread API on Windows in
pgbench.c, in favour of a couple of tiny local macros that abstract
over POSIX, Windows and threadless builds.  This results in less code,
and also fixes some minor problems I spotted in pre-existing code:
it's not OK to use (pthread_t) 0 as a special value, or to compare
pthread_t values with ==, or to assume that pthread APIs set errno.

v10-0003-pgbench-Improve-time-measurement-code.patch

Your original A patch, rebased over the above.  I haven't reviewed
this one.  It lacks a commit message.

v10-0004-pgbench-Synchronize-client-threads.patch

Adds in the barriers.

  39. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-05T05:22:12Z

    On Thu, Mar 4, 2021 at 10:44 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch

Here's a better version of that part.  I don't yet know if it actually
works on Windows...

  40. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-08T02:18:42Z

    On Fri, Mar 5, 2021 at 6:22 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, Mar 4, 2021 at 10:44 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch
>
> Here's a better version of that part.  I don't yet know if it actually
> works on Windows...

David Rowley kindly tested this for me on Windows and told me how to
fix one of the macros that had incorrect error checking on that OS.
So here's a new version.  I'm planning to commit 0001 and 0002 soon,
if there are no objections.  0003 needs some more review.

  41. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-10T04:54:41Z

    On Mon, Mar 8, 2021 at 3:18 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> David Rowley kindly tested this for me on Windows and told me how to
> fix one of the macros that had incorrect error checking on that OS.
> So here's a new version.  I'm planning to commit 0001 and 0002 soon,
> if there are no objections.  0003 needs some more review.

I made a few mostly cosmetic changes, pgindented and pushed all these patches.

  42. Re: pgbench: option delaying queries till connections establishment?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-13T02:46:46Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Mar 8, 2021 at 3:18 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>> David Rowley kindly tested this for me on Windows and told me how to
>> fix one of the macros that had incorrect error checking on that OS.
>> So here's a new version.  I'm planning to commit 0001 and 0002 soon,
>> if there are no objections.  0003 needs some more review.

> I made a few mostly cosmetic changes, pgindented and pushed all these patches.

So, gaur is not too happy with this:

ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -I../../src/port -DFRONTEND -I../../src/include  -D_USE_CTYPE_MACROS -D_XOPEN_SOURCE_EXTENDED  -I/usr/local/libxml2-2.6.23/include/libxml2 -I/usr/local/ssl-1.0.1e/include  -c -o strlcat.o strlcat.c
pthread_barrier_wait.c: In function 'pthread_barrier_init':
pthread_barrier_wait.c:24:2: error: incompatible type for argument 2 of 'pthread_cond_init'
/usr/include/pthread.h:378:5: note: expected 'pthread_condattr_t' but argument is of type 'void *'
pthread_barrier_wait.c:26:2: error: incompatible type for argument 2 of 'pthread_mutex_init'
/usr/include/pthread.h:354:5: note: expected 'pthread_mutexattr_t' but argument is of type 'void *'
make[2]: *** [pthread_barrier_wait.o] Error 1

Checking the man pages, it seems that this ancient HPUX version
is using some pre-POSIX API spec in which pthread_cond_init takes a
pthread_condattr_t rather than a pointer to pthread_condattr_t.
Similarly for pthread_mutex_init.

While it's likely that we could work around that, it's my
opinion that we shouldn't have to, because gaur is building with
--disable-thread-safety.  If that switch has any meaning at all,
it should be that we don't try to use thread infrastructure.
Was any thought given to being able to opt out of this patchset
to support that configure option?

			regards, tom lane

  43. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-13T03:00:29Z

    On Sat, Mar 13, 2021 at 3:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Checking the man pages, it seems that this ancient HPUX version
> is using some pre-POSIX API spec in which pthread_cond_init takes a
> pthread_condattr_t rather than a pointer to pthread_condattr_t.
> Similarly for pthread_mutex_init.

Wow.

> While it's likely that we could work around that, it's my
> opinion that we shouldn't have to, because gaur is building with
> --disable-thread-safety.  If that switch has any meaning at all,
> it should be that we don't try to use thread infrastructure.
> Was any thought given to being able to opt out of this patchset
> to support that configure option?

Oops.  The pgbench code was tested under --disable-thread-safety, but
it didn't occur to me that the AC_REPLACE_FUNCS search for
pthread_barrier_wait should also be conditional on that; I will now go
and try to figure out how to do that.

  44. Re: pgbench: option delaying queries till connections establishment?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-13T03:08:55Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Sat, Mar 13, 2021 at 3:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Was any thought given to being able to opt out of this patchset
>> to support that configure option?

> Oops.  The pgbench code was tested under --disable-thread-safety, but
> it didn't occur to me that the AC_REPLACE_FUNCS search for
> pthread_barrier_wait should also be conditional on that; I will now go
> and try to figure out how to do that.

OK, cool.  I don't think it's hard, just do

if test "$enable_thread_safety" = yes; then
  AC_REPLACE_FUNCS(pthread_barrier_wait)
fi

Probably this check should be likewise conditional:

AC_SEARCH_LIBS(pthread_barrier_wait, pthread)

			regards, tom lane

  45. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-13T03:37:11Z

    On Sat, Mar 13, 2021 at 4:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> OK, cool.  I don't think it's hard, just do
>
> if test "$enable_thread_safety" = yes; then
>   AC_REPLACE_FUNCS(pthread_barrier_wait)
> fi
>
> Probably this check should be likewise conditional:
>
> AC_SEARCH_LIBS(pthread_barrier_wait, pthread)

Thanks.  This seems to work for me on a Mac.  I confirmed with nm that
we don't define or reference any pthread_XXX symbols with
--disable-thread-safety, and we do otherwise, and the pgbench tests
pass either way.

  46. Re: pgbench: option delaying queries till connections establishment?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-13T03:58:56Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> Thanks.  This seems to work for me on a Mac.  I confirmed with nm that
> we don't define or reference any pthread_XXX symbols with
> --disable-thread-safety, and we do otherwise, and the pgbench tests
> pass either way.

Looks reasonable by eyeball.  If you'd push it, I can launch
a gaur run right away.

			regards, tom lane

  47. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-13T04:23:22Z

    On Sat, Mar 13, 2021 at 4:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Thanks.  This seems to work for me on a Mac.  I confirmed with nm that
> > we don't define or reference any pthread_XXX symbols with
> > --disable-thread-safety, and we do otherwise, and the pgbench tests
> > pass either way.
>
> Looks reasonable by eyeball.  If you'd push it, I can launch
> a gaur run right away.

Done.

  48. Re: pgbench: option delaying queries till connections establishment?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-13T06:31:39Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Sat, Mar 13, 2021 at 4:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Looks reasonable by eyeball.  If you'd push it, I can launch
>> a gaur run right away.

> Done.

gaur's gotten through "make" and "make check" cleanly.  Unfortunately
I expect it will fail at the pg_amcheck test before it reaches pgbench.
But for the moment it's reasonable to assume we're good here.  Thanks!

			regards, tom lane

  49. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2021-03-13T08:08:31Z

    Hello Thomas,

>> David Rowley kindly tested this for me on Windows and told me how to
>> fix one of the macros that had incorrect error checking on that OS.
>> So here's a new version.  I'm planning to commit 0001 and 0002 soon,
>> if there are no objections.  0003 needs some more review.
>
> I made a few mostly cosmetic changes, pgindented and pushed all these patches.

Thanks a lot for pushing all that, and fixing issues raised by buildfarm 
animals pretty unexpected and strange failures…

I must say that I'm not a big fan of the macro-based all-in-capitals API 
for threads because it exposes some platform specific uglyness (eg 
THREAD_FUNC_CC) and it does not look much like clean C code when used. I 
liked the previous partial pthread implementation better, even if it was 
not the real thing, obviously.

ISTM that with the current approach threads are always used on Windows, 
i.e. pgbench does not comply to "ENABLE_THREAD_SAFETY" configuration on 
that platform. Not sure whether this is an issue that need to be 
addressed, though.

-- 
Fabien.

  50. Re: pgbench: option delaying queries till connections establishment?

    Thomas Munro <thomas.munro@gmail.com> — 2021-03-13T09:54:24Z

    On Sat, Mar 13, 2021 at 9:08 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> I must say that I'm not a big fan of the macro-based all-in-capitals API
> for threads because it exposes some platform specific uglyness (eg
> THREAD_FUNC_CC) and it does not look much like clean C code when used. I
> liked the previous partial pthread implementation better, even if it was
> not the real thing, obviously.

But we were using macros already, to support --disable-thread-safety
builds.  I just changed them to upper case and dropped the 'p',
because I didn't like to pretend to do POSIX threads, but do it so
badly.  Perhaps we should drop --disable-thread-safety soon, and
perhaps it is nearly time to create a good thread abstraction in clean
C code, for use in the server and here?  Then we won't need any ugly
macros.

> ISTM that with the current approach threads are always used on Windows,
> i.e. pgbench does not comply to "ENABLE_THREAD_SAFETY" configuration on
> that platform. Not sure whether this is an issue that need to be
> addressed, though.

The idea of that option, as I understand it, is that in ancient times
there were Unix systems with no threads (that's of course the reason
PostgreSQL is the way it is).  I don't think that was ever the case
for Windows NT, and we have no build option for that on Windows
AFAICS.

  51. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2021-03-13T11:09:37Z

    Hello Thomas,

>> I must say that I'm not a big fan of the macro-based all-in-capitals API
>> for threads because it exposes some platform specific uglyness (eg
>> THREAD_FUNC_CC) and it does not look much like clean C code when used. I
>> liked the previous partial pthread implementation better, even if it was
>> not the real thing, obviously.
>
> But we were using macros already, to support --disable-thread-safety
> builds.

Yep, but the look and feel was still C code.

> I just changed them to upper case and dropped the 'p', because I didn't 
> like to pretend to do POSIX threads, but do it so badly.

Hmmm. From the source code point of view it was just like actually using 
posix threads, even if the underlying machinery was not quite that on some 
systems. I value looking at "beautiful" and "standard" code if possible, 
even if there is some cheating involved, compared to exposing macros. I 
made some effort to remove the pretty ugly and inefficient INSTR_TIME 
macros from pgbench, replaced with straightforward arithmetic and inlined 
functions. Now some other macros just crept back in:-) Anyway, this is 
just "les goûts et les couleurs" (just a matter of taste), as we say here.

> Perhaps we should drop --disable-thread-safety soon, and perhaps it is 
> nearly time to create a good thread abstraction in clean C code, for use 
> in the server and here?  Then we won't need any ugly macros.

+1.

>> ISTM that with the current approach threads are always used on Windows,
>> i.e. pgbench does not comply to "ENABLE_THREAD_SAFETY" configuration on
>> that platform. Not sure whether this is an issue that need to be
>> addressed, though.
>
> The idea of that option, as I understand it, is that in ancient times
> there were Unix systems with no threads (that's of course the reason
> PostgreSQL is the way it is).  I don't think that was ever the case
> for Windows NT, and we have no build option for that on Windows
> AFAICS.

Ok, fine with me.

-- 
Fabien.

  52. Re: pgbench: option delaying queries till connections establishment?

    Dave Cramer <davecramer@postgres.rocks> — 2023-05-16T12:54:43Z

    Dave Cramer
www.postgres.rocks


On Tue, 16 May 2023 at 07:27, Andres Freund <andres@anarazel.de> wrote:

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

I've recently run into something I am having difficulty understanding.

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 ?

Dave

  53. Re: pgbench: option delaying queries till connections establishment?

    Dave Cramer <davecramer@postgres.rocks> — 2023-05-16T14:25:29Z

    >
> I've recently run into something I am having difficulty understanding.
>
> 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 ?
>
> Dave
>

Turns out that for this specific test, pg is faster with a pooler.

Dave Cramer, [May 16, 2023 at 9:49:24 AM]:

turns out having a connection pool helps. First run is without a pool,
second with


pgbench=# select mean_exec_time, stddev_exec_time, calls, total_exec_time,
min_exec_time, max_exec_time from pg_stat_statements where
queryid=-531095336438083412;

   mean_exec_time   |  stddev_exec_time  | calls |  total_exec_time  |
   min_exec_time
    | max_exec_time

--------------------+--------------------+-------+-------------------+----------------------+---------------

 0.4672699999999998 | 2.2758508661446535 |   200 | 93.45399999999997 |
0.046616000000000005 |     17.434766

(1 row)


pgbench=# select pg_stat_statements_reset();

 pg_stat_statements_reset

--------------------------


(1 row)


pgbench=# select mean_exec_time, stddev_exec_time, calls, total_exec_time,
min_exec_time, max_exec_time from pg_stat_statements where
queryid=-531095336438083412;

   mean_exec_time    |   stddev_exec_time   | calls |  total_exec_time   |
min_exec_time | max_exec_time

---------------------+----------------------+-------+--------------------+---------------+---------------

 0.06640186499999999 | 0.021800404695481574 |   200 | 13.280373000000004 |
    0.034006 |      0.226696
(1 row)

  54. Re: pgbench: option delaying queries till connections establishment?

    Fabien COELHO <coelho@cri.ensmp.fr> — 2023-07-03T07:24:17Z

    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.