Re: Fix calculations on WAL recycling.

At Mon, 23 Jul 2018 15:59:16 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180723065916.GI2854@paquier.xyz>
> On Mon, Jul 23, 2018 at 01:57:48PM +0900, Kyotaro HORIGUCHI wrote:
> > I'll register this to the next commitfest.
> > 
> > https://www.postgresql.org/message-id/20180719.125117.155470938.horiguchi.kyotaro@lab.ntt.co.jp
> 
> This is an open item for v11.

Mmm. Thanks. I wrongly thought this was v10 item. Removed this
from the next CF.

Thank you for taking a look.

> >> While considering this, I found a bug in 4b0d28de06, which
> >> removed prior checkpoint from control file. It actually trims the
> >> segments before the last checkpoint's redo segment but recycling
> >> is still considered based on the *prevous* checkpoint. As the
> >> result min_wal_size doesn't work as told.  Specifically, setting
> >> min/max_wal_size to 48MB and advance four or more segments then
> >> two checkpoints leaves just one segment, which is less than
> >> min_wal_size.
> >> 
> >> The attached patch fixes that. One arguable point on this would
> >> be the removal of the behavior when RemoveXLogFile(name,
> >> InvalidXLogRecPtr, ..).
> >> 
> >> The only place calling the function with the parameter is
> >> timeline switching. Previously unconditionally 10 segments are
> >> recycled after switchpoint but the reason for the behavior is we
> >> didn't have the information on previous checkpoint at hand at the
> >> time. But now we can use the timeline switch point as the
> >> approximate of the last checkpoint's redo point and this allows
> >> us to use min/max_wal_size properly at the time.
> 
> I have been looking at that, and tested with this simple scenario:
> create table aa (a int);
> 
> Then just repeat the following SQLs: 
> insert into aa values (1);
> select pg_switch_wal();
> checkpoint;
> select pg_walfile_name(pg_current_wal_lsn());
> 
> By doing so, you would notice that the oldest WAL segment does not get
> recycled after the checkpoint, while it should as the redo pointer is
> always checkpoint generated.  What happens is that this oldest segment
> gets recycled every two checkpoints.

(I'm not sure I'm getting it correctly..) I see the old segments
recycled. When I see pg_switch_wal() returns 0/12..,
pg_walfile_name is ...13 and segment files for 13 and 14 are
found in pg_wal directory. That is, seg 12 was recycled as seg
14. log_checkpoint=on shows every checkpoint recycles 1 segment
in the case.

> Then I had a look at the proposed patch, with a couple of comments.
> 
> -   if (PriorRedoPtr == InvalidXLogRecPtr)
> -       recycleSegNo = endlogSegNo + 10;
> -   else
> -       recycleSegNo = XLOGfileslop(PriorRedoPtr);
> +   recycleSegNo = XLOGfileslop(RedoRecPtr);
> I think that this is a new behavior, and should not be changed, see
> point 3 below.
> 
> In CreateCheckPoint(), the removal of past WAL segments is always going
> to happen as RedoRecPtr is never InvalidXLogRecPtr, so the recycling
> should happen also when PriorRedoPtr is InvalidXLogRecPtr, no?

Yes. The reason for the change was the change of
RemoveNonParentXlogFiles that I made and it is irrelevant to this
bug fix (and rather breaking as you pointed..). So, reverted it.


> /* Trim from the last checkpoint, not the last - 1 */
> This comment could be adjusted, let's remove "not the last - 1" .

Oops! Thanks. The comment has finally vanished and melded into
another comment just above.

| * Delete old log files not required by the last checkpoint and recycle
| * them


> The calculation of _logSegNo in CreateRestartPoint is visibly incorrect,
> this still uses PriorRedoPtr so the bug is not fixed for standbys.  The
> tweaks for ThisTimeLineID also need to be out of the loop where
> PriorRedoPtr is InvalidXLogRecPtr, and only the distance calculation
> should be kept.

Agreed.  While I reconsidered this, I noticed that the estimated
checkpoint distance is 0 when PriorRedoPtr is invalid. This means
that the first checkpoint/restartpoint tries to reduce WAL
segments to min_wal_size. However, it happens only at initdb time
and makes no substantial behavior change so I made the change
ignoring the difference.

> Finally, in summary, this patch is doing actually three things:
> 1) Rename a couple of variables which refer incorrectly to the prior
> checkpoint so as they refer to the last checkpoint generated.
> 2) Make sure that WAL recycling/removal happens based on the last
> checkpoint generated, which is the one just generated when past WAL
> segments are cleaned up as post-operation actions.
> 3) Enforce the recycling point to be the switch point instead of
> arbitrarily recycle 10 segments, which is what b2a5545b has introduced.
> Recycling at exactly the switch point is wrong, as the redo LSN of the
> previous checkpoint may not be at the same segment when a timeline has
> switched, so you would finish with recycling segments which are still
> needed if an instance needs be crash-recovered post-promotion without
> a completed post-recovery checkpoint.  In short this is dangerous.
> I'll let Heikki comment on that, but I think that you should fetch the
> last redo LSN instead, still you need to be worried about checkpoints
> running in parallel of the startup process, so I would stick with the
> current logic.

Thank you for the detail. I was coufused a bit there. I agree to
preserve the logic, too.

> 1) and 2) are in my opinion clearly oversights from 4b0d28d, but 3) is
> not.

Thank you for the comments and suggestions. After all, I did the
following things in the attached patch.

- Reverted the change on timeline switching. (Removed the (3))

- Fixed CreateRestartPoint to do the same thing with CreateCheckPoint.

- Both CreateRestart/CheckPoint now tries trimming of WAL
  segments even for the first time.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 662ecab9c2d0efc41756d98e18dbabe060da8d96 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 19 Jul 2018 12:13:56 +0900
Subject: [PATCH] Fix calculation base of WAL recycling

The commit 4b0d28de06 removed the prior checkpoint and related things
but that leaves WAL recycling based on the prior checkpoint. This
makes max_wal_size and min_wal_size work incorrectly. This patch makes
WAL recycling be based on the last checkpoint.
---
 src/backend/access/transam/xlog.c | 159 ++++++++++++++++++--------------------
 1 file changed, 75 insertions(+), 84 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 335b4a573d..05f750253f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2287,7 +2287,7 @@ assign_checkpoint_completion_target(double newval, void *extra)
  * XLOG segments? Returns the highest segment that should be preallocated.
  */
 static XLogSegNo
-XLOGfileslop(XLogRecPtr PriorRedoPtr)
+XLOGfileslop(XLogRecPtr RedoRecPtr)
 {
     XLogSegNo    minSegNo;
     XLogSegNo    maxSegNo;
@@ -2299,9 +2299,9 @@ XLOGfileslop(XLogRecPtr PriorRedoPtr)
      * correspond to. Always recycle enough segments to meet the minimum, and
      * remove enough segments to stay below the maximum.
      */
-    minSegNo = PriorRedoPtr / wal_segment_size +
+    minSegNo = RedoRecPtr / wal_segment_size +
         ConvertToXSegs(min_wal_size_mb, wal_segment_size) - 1;
-    maxSegNo = PriorRedoPtr / wal_segment_size +
+    maxSegNo = RedoRecPtr / wal_segment_size +
         ConvertToXSegs(max_wal_size_mb, wal_segment_size) - 1;
 
     /*
@@ -2316,7 +2316,7 @@ XLOGfileslop(XLogRecPtr PriorRedoPtr)
     /* add 10% for good measure. */
     distance *= 1.10;
 
-    recycleSegNo = (XLogSegNo) ceil(((double) PriorRedoPtr + distance) /
+    recycleSegNo = (XLogSegNo) ceil(((double) RedoRecPtr + distance) /
                                     wal_segment_size);
 
     if (recycleSegNo < minSegNo)
@@ -3899,12 +3899,12 @@ RemoveTempXlogFiles(void)
 /*
  * Recycle or remove all log files older or equal to passed segno.
  *
- * endptr is current (or recent) end of xlog, and PriorRedoRecPtr is the
- * redo pointer of the previous checkpoint. These are used to determine
+ * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycle rather than delete no-longer-wanted log files.
  */
 static void
-RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
+RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
 {
     DIR           *xldir;
     struct dirent *xlde;
@@ -3947,7 +3947,7 @@ RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
                 /* Update the last removed location in shared memory first */
                 UpdateLastRemovedPtr(xlde->d_name);
 
-                RemoveXlogFile(xlde->d_name, PriorRedoPtr, endptr);
+                RemoveXlogFile(xlde->d_name, RedoRecPtr, endptr);
             }
         }
     }
@@ -4021,14 +4021,14 @@ RemoveNonParentXlogFiles(XLogRecPtr switchpoint, TimeLineID newTLI)
 /*
  * Recycle or remove a log file that's no longer needed.
  *
- * endptr is current (or recent) end of xlog, and PriorRedoRecPtr is the
- * redo pointer of the previous checkpoint. These are used to determine
+ * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycle rather than delete no-longer-wanted log files.
- * If PriorRedoRecPtr is not known, pass invalid, and the function will
- * recycle, somewhat arbitrarily, 10 future segments.
+ * If RedoRecPtr is not known, pass invalid, and the function will recycle,
+ * somewhat arbitrarily, 10 future segments.
  */
 static void
-RemoveXlogFile(const char *segname, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
+RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
 {
     char        path[MAXPGPATH];
 #ifdef WIN32
@@ -4042,10 +4042,10 @@ RemoveXlogFile(const char *segname, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
      * Initialize info about where to try to recycle to.
      */
     XLByteToSeg(endptr, endlogSegNo, wal_segment_size);
-    if (PriorRedoPtr == InvalidXLogRecPtr)
+    if (RedoRecPtr == InvalidXLogRecPtr)
         recycleSegNo = endlogSegNo + 10;
     else
-        recycleSegNo = XLOGfileslop(PriorRedoPtr);
+        recycleSegNo = XLOGfileslop(RedoRecPtr);
 
     snprintf(path, MAXPGPATH, XLOGDIR "/%s", segname);
 
@@ -8706,6 +8706,7 @@ CreateCheckPoint(int flags)
     bool        shutdown;
     CheckPoint    checkPoint;
     XLogRecPtr    recptr;
+    XLogSegNo    _logSegNo;
     XLogCtlInsert *Insert = &XLogCtl->Insert;
     uint32        freespace;
     XLogRecPtr    PriorRedoPtr;
@@ -9072,22 +9073,18 @@ CreateCheckPoint(int flags)
      */
     smgrpostckpt();
 
-    /*
-     * Delete old log files and recycle them
-     */
+    /* Update the average distance between checkpoints/restartpoints. */
     if (PriorRedoPtr != InvalidXLogRecPtr)
-    {
-        XLogSegNo    _logSegNo;
-
-        /* Update the average distance between checkpoints. */
         UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr);
 
-        /* Trim from the last checkpoint, not the last - 1 */
-        XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
-        KeepLogSeg(recptr, &_logSegNo);
-        _logSegNo--;
-        RemoveOldXlogFiles(_logSegNo, PriorRedoPtr, recptr);
-    }
+    /*
+     * Delete old log files (those no longer needed for last checkpoint to
+     * prevent the disk holding the xlog from growing full.
+     */
+    XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
+    KeepLogSeg(recptr, &_logSegNo);
+    _logSegNo--;
+    RemoveOldXlogFiles(_logSegNo, RedoRecPtr, recptr);
 
     /*
      * Make more log segments if needed.  (Do this after recycling old log
@@ -9253,6 +9250,11 @@ CreateRestartPoint(int flags)
     XLogRecPtr    lastCheckPointEndPtr;
     CheckPoint    lastCheckPoint;
     XLogRecPtr    PriorRedoPtr;
+    XLogRecPtr    receivePtr;
+    XLogRecPtr    replayPtr;
+    TimeLineID    replayTLI;
+    XLogRecPtr    endptr;
+    XLogSegNo    _logSegNo;
     TimestampTz xtime;
 
     /*
@@ -9394,69 +9396,58 @@ CreateRestartPoint(int flags)
     }
     LWLockRelease(ControlFileLock);
 
-    /*
-     * Delete old log files (those no longer needed even for previous
-     * checkpoint/restartpoint) to prevent the disk holding the xlog from
-     * growing full.
-     */
+    /* Update the average distance between checkpoints/restartpoints. */
     if (PriorRedoPtr != InvalidXLogRecPtr)
-    {
-        XLogRecPtr    receivePtr;
-        XLogRecPtr    replayPtr;
-        TimeLineID    replayTLI;
-        XLogRecPtr    endptr;
-        XLogSegNo    _logSegNo;
-
-        /* Update the average distance between checkpoints/restartpoints. */
         UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr);
 
-        XLByteToSeg(PriorRedoPtr, _logSegNo, wal_segment_size);
+    /*
+     * Delete old log files (those no longer needed for last restartpoint to
+     * prevent the disk holding the xlog from growing full.
+     */
+    XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
 
-        /*
-         * Get the current end of xlog replayed or received, whichever is
-         * later.
-         */
-        receivePtr = GetWalRcvWriteRecPtr(NULL, NULL);
-        replayPtr = GetXLogReplayRecPtr(&replayTLI);
-        endptr = (receivePtr < replayPtr) ? replayPtr : receivePtr;
+    /*
+     * Retreat _logSegNo using the current end of xlog replayed or received,
+     * whichever is later.
+     */
+    receivePtr = GetWalRcvWriteRecPtr(NULL, NULL);
+    replayPtr = GetXLogReplayRecPtr(&replayTLI);
+    endptr = (receivePtr < replayPtr) ? replayPtr : receivePtr;
+    KeepLogSeg(endptr, &_logSegNo);
+    _logSegNo--;
+    
+    /*
+     * Try to recycle segments on a useful timeline. If we've been promoted
+     * since the beginning of this restartpoint, use the new timeline chosen
+     * at end of recovery (RecoveryInProgress() sets ThisTimeLineID in that
+     * case). If we're still in recovery, use the timeline we're currently
+     * replaying.
+     *
+     * There is no guarantee that the WAL segments will be useful on the
+     * current timeline; if recovery proceeds to a new timeline right after
+     * this, the pre-allocated WAL segments on this timeline will not be used,
+     * and will go wasted until recycled on the next restartpoint. We'll live
+     * with that.
+     */
+    if (RecoveryInProgress())
+        ThisTimeLineID = replayTLI;
 
-        KeepLogSeg(endptr, &_logSegNo);
-        _logSegNo--;
+    RemoveOldXlogFiles(_logSegNo, RedoRecPtr, endptr);
 
-        /*
-         * Try to recycle segments on a useful timeline. If we've been
-         * promoted since the beginning of this restartpoint, use the new
-         * timeline chosen at end of recovery (RecoveryInProgress() sets
-         * ThisTimeLineID in that case). If we're still in recovery, use the
-         * timeline we're currently replaying.
-         *
-         * There is no guarantee that the WAL segments will be useful on the
-         * current timeline; if recovery proceeds to a new timeline right
-         * after this, the pre-allocated WAL segments on this timeline will
-         * not be used, and will go wasted until recycled on the next
-         * restartpoint. We'll live with that.
-         */
-        if (RecoveryInProgress())
-            ThisTimeLineID = replayTLI;
+    /*
+     * Make more log segments if needed.  (Do this after recycling old log
+     * segments, since that may supply some of the needed files.)
+     */
+    PreallocXlogFiles(endptr);
 
-        RemoveOldXlogFiles(_logSegNo, PriorRedoPtr, endptr);
-
-        /*
-         * Make more log segments if needed.  (Do this after recycling old log
-         * segments, since that may supply some of the needed files.)
-         */
-        PreallocXlogFiles(endptr);
-
-        /*
-         * ThisTimeLineID is normally not set when we're still in recovery.
-         * However, recycling/preallocating segments above needed
-         * ThisTimeLineID to determine which timeline to install the segments
-         * on. Reset it now, to restore the normal state of affairs for
-         * debugging purposes.
-         */
-        if (RecoveryInProgress())
-            ThisTimeLineID = 0;
-    }
+    /*
+     * ThisTimeLineID is normally not set when we're still in recovery.
+     * However, recycling/preallocating segments above needed ThisTimeLineID
+     * to determine which timeline to install the segments on. Reset it now,
+     * to restore the normal state of affairs for debugging purposes.
+     */
+    if (RecoveryInProgress())
+        ThisTimeLineID = 0;
 
     /*
      * Truncate pg_subtrans if possible.  We can throw away all data before
-- 
2.16.3