Alvaro Herrera wrote:
I gave this patch a look and it seems pretty good to me, except 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?
That was originally how I planned to write this bit of code. When I realized that the CheckpointStats structure was already visible there and stuffed with details that ultimately go into the same output line at the end, it just didn't seem worth the extra code complexity. The abstraction layer around md.c was not exactly airtight before I poked that extra little hole in there, and I was aiming via the principal of a smaller diff usually being the better patch . If you feel strongly that the result led to a bad abstraction violation, I'll submit a patch to refactor it to pass a structure instead before the next CF. I appreciate your concern, I'm just not sure it's worth spending time on. What I'd really like to do is refactor out major parts of the leaky md/smgr layers altogether instead, but that's obviously a bigger project.
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.)
And now you're just getting picky! This is a useful observation though, and I'll try to include that fix along with the next general checkpoint overhaul patch I submit. Doesn't seem worth going through the trouble of committing that minor rework on its own, I'll slip it into the next useful thing that touches this area I do. Thanks for the hint, this would work better than what I did.
-- Greg Smith 2ndQuadrant US g...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services and Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
