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
Date: Wed, 27 Jan 2016 10:03:45 +0100 (CET)
Message-ID: <alpine.DEB.2.10.1601270942310.12620@sto> (raw)
In-Reply-To: <20160126144226.GA557305@alvherre.pgsql>
References: <20160126144226.GA557305@alvherre.pgsql>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: coelho@cri.ensmp.fr, alvherre@2ndquadrant.com
Subject: Re: pgbench stats per script & other stuff
In-Reply-To: <alpine.DEB.2.10.1601270942310.12620@sto>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox