Excerpts from Robert Haas's message of mar dic 14 11:34:55 -0300 2010:
> On Tue, Dec 14, 2010 at 9:29 AM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
> > I gave this patch a look and it seems pretty good to me, except
>
> Err, woops. I just committed this as-is. Sorry.
I noticed :-)
> > that I'm
> > uncomfortable with the idea of mdsync filling in the details for
> > CheckpointStats fields directly. Would it work to pass a struct (say
> > SmgrSyncStats) from CheckPointBuffers to smgrsync and from there to
> > mdsync, have this function fill it, and return it back so that
> > CheckPointBuffers copies the data from this struct into CheckpointStats?
> >
> > Another minor nitpick: inside the block when you call FileSync, why
> > check for log_checkpoints at all? Seems to me that just checking for
> > zero of sync_start should be enough. Alternatively, seems simpler to
> > just have a local var with the value of log_checkpoints at the start of
> > mdsync and use that throughout the function. (Surely if someone turns
> > off log_checkpoints in the middle of a checkpoint, it's not really a
> > problem that we collect and report stats during that checkpoint.)
>
> Neither of these things bothers me, but we can certainly discuss...
Well, the second one was just about simplifying it, so never mind that.
But referring to CheckpointStats in md.c seems to me to be a violation
of modularity that ought to be fixed.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
