Re: Instrument checkpoint sync calls

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 greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books