Re: pgbench: option delaying queries till connections establishment?

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.