Fabien COELHO wrote:
> You know how delighted I am to split patches...
Yes, of course, it's the most interesting task in the world. I'm fully
aware of that.
FWIW I'm going to apply a preliminary commit to pgindent-clean the file
before your patches, then apply each patch as pgindent-clean. Otherwise
your whitespace style was getting too much on my nerves.
> b) refactor statistics collections (per thread, per command, per whatever)
> so as to use the same structure everywhere, reducing the CLOC by 115.
> this enables the next small patch which can reuse the new functions.
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?
Why is this function defined like this?
/*
* Initialize a StatsData struct to all zeroes, but the given
* start_time, except that if it's exactly zero don't change it.
*/
static void
initStats(StatsData *sd, double start_time)
{
sd->cnt = 0;
sd->skipped = 0;
initSimpleStats(&sd->latency);
initSimpleStats(&sd->lag);
/* not necessarily overriden? */
if (start_time)
sd->start_time = start_time;
}
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;
}
I renamed a couple of your functionettes, for instance doSimpleStats to
addToSimpleStats and appendSimpleStats to mergeSimpleStats.
Haven't looked at patches c or d yet. I'm tempted to thrown patch e in
with the initial pgindent run.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
