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.
> 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...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company