Date: Wed, 27 Jan 2016 10:03:45 +0100 (CET)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: pgbench stats per script & other stuff
In-Reply-To: <20160126144226.GA557305@alvherre.pgsql>
Message-ID: <alpine.DEB.2.10.1601270942310.12620@sto>
References: <20160126144226.GA557305@alvherre.pgsql>
User-Agent: Alpine 2.10 (DEB 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers