Re: pgbench: option delaying queries till connectionsestablishment? - Mailing list pgsql-hackers

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.