On Tue, Dec 14, 2010 at 9:29 AM, Alvaro Herrera <alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
