Re: pgbench stats per script & other stuff - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: pgbench stats per script & other stuff
Date
Msg-id alpine.DEB.2.10.1601270942310.12620@sto
Whole thread Raw
In response to Re: pgbench stats per script & other stuff  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Responses Re: pgbench stats per script & other stuff  (Alvaro Herrera <alvherre@2ndquadrant.com>)
List pgsql-hackers
Hello Alvaro,

> I'm not really sure about the fact that we operate on those Stats
> structs without locking.  I see upthread you convinced Michael that it
> was okay, but is it really?  How severe is the damage if two threads
> happen to collide?

For stats shared among threads, when it occurs one data about one 
transaction is not counted.

On the risk side: the collision probability is pretty low because the time 
to update a value is a "few" cycles, and the time to execute a transaction 
is typically in ms: I think under 1/10,000,000 data could be lost.

On the advantageous side: locking costs significant time thus would impact 
performance, I think that the measured performance loss because the 
occasional transaction data is not counted is lower that the performance 
loss due to systematically locking.

So for me this is really a low risk trade-off.

> [...]
> It seems a bit funny to have the start_time not be reset when 0.0 is
> passed, which is almost all the callers.  Using a float as a boolean
> looks pretty odd; is that kosher?  Maybe it'd be a good idea to have a
> separate boolean flag instead?  Something like this
>
> /*
> * Initialize a StatsData struct to all zeroes.  Use the given
> * start_time only if reset_start_time, otherwise keep the original
> * value.
> */
> static void
> initStats(StatsData *sd, double start_time, bool reset_start_time)
> {
>     sd->cnt = 0;
>     sd->skipped = 0;
>     initSimpleStats(&sd->latency);
>     initSimpleStats(&sd->lag);
>
>     /* not necessarily overriden? */
>     if (reset_start_time)
>         sd->start_time = start_time;
> }

Obviously this would work. I did not think the special case was worth the 
extra argument. This one has some oddity too, because the second argument 
is ignored depending on the third. Do as you feel.

> I renamed a couple of your functionettes, for instance doSimpleStats to
> addToSimpleStats and appendSimpleStats to mergeSimpleStats.

Fine with me.

-- 
Fabien.



pgsql-hackers by date:

Previous
From: Aleksander Alekseev
Date:
Subject: Re: Patch: ResourceOwner optimization for tables with many partitions
Next
From: Fabien COELHO
Date:
Subject: Re: pgbench stats per script & other stuff