Re: Fix checkpoint skip logic on idle systems by tracking LSN progress

On 10/5/16 7:18 AM, Michael Paquier wrote:

>> Note: I am moving this patch to next CF.
>
> And I am back on it more seriously... And I am taking back what I said upthread.
>
> I looked at the v12 that Horiguchi-san has written, and that seems
> correct to me. So I have squashed everything into a single patch,
> including the log entry that gets logged with log_checkpoints. Testing
> with archive_timeout to 10s, checkpoint_timeout to 30s, sometimes
> triggering manual activity with CREATE TABLE/whatever and manual
> pg_switch_xlog(), I am able to see that checkpoints can be correctly
> skipped or generated.
>
> There was as well a compilation error with ereport(). Not sure how I
> missed that... Likely too many patches handled these days.
>
> I have also updated the description of archive_timeout that increasing
> checkpoint_timeout would reduce unnecessary checkpoints on a idle
> system. With this patch, on an idle system checkpoints are just
> skipped as they should.
>
> How does that look?

This looks much better now and exhibits exactly the behavior that I 
expect.

In theory it would be nice if the checkpoint records did not cause 
rotation, but this can be mitigated in the way you have described and 
perhaps for safety's sake it's best.

I had a bit of trouble parsing this paragraph:

+    /*
+     * Update the progress LSN positions. At least one WAL insertion lock
+     * is already taken appropriately before doing that, and it is just more
+     * simple to do that here where WAL record data and type is at hand.
+     * The progress is set at the start position of the record tracked that
+     * is being added, making easier checkpoint progress tracking as the
+     * control file already saves the start LSN position of the last
+     * checkpoint run. If an exclusive lock is taken for WAL insertion,
+     * there is actually no need to update all the progression fields, so

So I did a little reworking:

Update the LSN progress positions. At least one WAL insertion lock is 
already taken appropriately before doing that, and it is simpler to do 
that here when the WAL record data and type are at hand. Progress is set 
at the start position of the tracked record that is being added, making 
checkpoint progress tracking easier as the control file already saves 
the start LSN position of the last checkpoint. If an exclusive lock is 
taken for WAL insertion there is no need to update all the progress 
fields, only the first one.

If that still says what you think it should, then I believe it is 
clearer.  Also:

+         * last time a segment has switched because of a timeout. Segment
+         * switching because of other reasons, like manual trigerring of

typo, should be "triggering".

I don't see any further issues with this patch unless there are 
performance concerns about the locks taken in GetProgressRecPtr().  The 
locks seem reasonable to me but I'd like to see this committed so 
there's plenty of time to detect any regression before 10.0.

As such, my vote is to mark this "Ready for Committer."  I'm fine with 
waiting a few days as Kyotaro suggested, or we can consider my review 
"additional comments" and do it now.

-- 
-David
david@pgmasters.net