Re: Instrument checkpoint sync calls

On Tue, Dec 14, 2010 at 9:29 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)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