Fix logic to skip checkpoint if no records have been inserted.

After the WAL format changes, the calculation of the size of a checkpoint
record became incorrect. Instead of trying to fix the math, check that the
previous record, i.e. the xl_prev value that we'd write for the next
record, matches the last checkpoint's redo pointer. That way it's not
dependent on the size of the checkpoint record at all.

The old logic was actually slightly wrong all along: if the previous
checkpoint record crossed a page boundary, the page headers threw off the
record size calculation, and the checkpoint was not skipped. The new
checkpoint would not cross a page boundary, so this only resulted in at
most one extra checkpoint after the system became idle. The new logic fixes
that. (It's not worth fixing in backbranches).

However, it makes some sense to try to keep the latest checkpoint contained
fully in a page, or at least in a single WAL segment, just on general
robustness grounds. If something goes awfully wrong, it's more likely that
you can recover the latest WAL segment, than the last two WAL segments. So
I added an extra check that the checkpoint is not skipped if the previous
checkpoint crossed a WAL segment.

Reported by Jeff Janes.

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 975eac0c22bb92e68ecae444b7a87e2dc43ab770..25809961028f0b7676c53d30579d0d5e07496f04 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7903,6 +7903,7 @@ CreateCheckPoint(int flags)
        uint32          freespace;
        XLogRecPtr      PriorRedoPtr;
        XLogRecPtr      curInsert;
+       XLogRecPtr      prevPtr;
        VirtualTransactionId *vxids;
        int                     nvxids;
 
@@ -7988,6 +7989,7 @@ CreateCheckPoint(int flags)
         */
        WALInsertLockAcquireExclusive();
        curInsert = XLogBytePosToRecPtr(Insert->CurrBytePos);
+       prevPtr = XLogBytePosToRecPtr(Insert->PrevBytePos);
 
        /*
         * If this isn't a shutdown or forced checkpoint, and we have not inserted
@@ -7999,17 +8001,17 @@ CreateCheckPoint(int flags)
         * (Perhaps it'd make even more sense to checkpoint only when the previous
         * checkpoint record is in a different xlog page?)
         *
-        * We have to make two tests to determine that nothing has happened since
-        * the start of the last checkpoint: current insertion point must match
-        * the end of the last checkpoint record, and its redo pointer must point
-        * to itself.
+        * If the previous checkpoint crossed a WAL segment, however, we create
+        * the checkpoint anyway, to have the latest checkpoint fully contained in
+        * the new segment. This is for a little bit of extra robustness: it's
+        * better if you don't need to keep two WAL segments around to recover the
+        * checkpoint.
         */
        if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY |
                                  CHECKPOINT_FORCE)) == 0)
        {
-               if (curInsert == ControlFile->checkPoint +
-                       MAXALIGN(SizeOfXLogRecord + sizeof(CheckPoint)) &&
-                       ControlFile->checkPoint == ControlFile->checkPointCopy.redo)
+               if (prevPtr == ControlFile->checkPointCopy.redo &&
+                       prevPtr / XLOG_SEG_SIZE == curInsert / XLOG_SEG_SIZE)
                {
                        WALInsertLockRelease();
                        LWLockRelease(CheckpointLock);