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.