Re: Crash by targetted recovery - Mailing list pgsql-hackers

At Sat, 7 Mar 2020 01:46:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > (It seems retroverting to the first patch when I started this...)
> > The second place covers wider cases so I reverted the first place.
> 
> Thanks for updating the patch that way.
> Not sure which patch you're mentioning, though.

That meant 0003.

> Regarding 0003 patch, I added a bit more detail comments into
> the patch so that we can understand the code more easily.
> Updated version of 0003 patch attached. Barring any objection,
> at first, I plan to commit this patch.

Looks good to me. Thanks for writing the detailed comments.

> >> + lastSourceFailed = false; /* We haven't failed on the new source */
> >>
> >> Is this really necessary? Since ReadRecord() always reset
> >> lastSourceFailed to false, it seems not necessary.
> > It's just to make sure.  Actually lastSourceFailed is always false
> > when we get there.  But when the source is switched, lastSourceFailed
> > should be changed to false as a matter of design.  I'd like to do that
> > unless that harms.
> 
> OK.

Thanks.

> >> -    else if (currentSource == 0)
> >> +    else if (currentSource == 0 ||
> >>
> >> Though this is a *separate topic*, 0 should be XLOG_FROM_ANY?
> >> There are some places where 0 is used as the value of currentSource.
> >> IMO they should be updated so that XLOG_FROM_ANY is used instead of 0.
> > Yeah, I've thought that many times but have neglected since it is not
> > critical and trivial as a separate patch.  I'd take the chance to do
> > that now.  Another minor glitch is "int oldSource = currentSource;" it
> > is not debugger-friendly so I changed it to XLogSource.  It is added
> > as a new patch file before the main patch.
> 
> There seems to be more other places where XLogSource and
> XLOG_FROM_XXX are not used yet. For example, the initial values
> of readSource and XLogReceiptSource, the type of argument
> "source" in XLogFileReadAnyTLI() and XLogFileRead(), etc.
> These also should be updated?

Right. I checked through the file and AFAICS that's all.  The attachec
v5-0001-Tidy...patch is the fix on top of the v4-0003 on the current
master.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 358f37899a02a8ae6f803fe32b97c2cbe302786f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Fri, 6 Mar 2020 10:11:59 +0900
Subject: [PATCH v5] Tidy up XLogSource usage.

We used interger 0 instead of XLOG_FROM_ANY and defined a variable to
store the type with int. Tidy them up for readability and
debugger-friendliness.
---
 src/backend/access/transam/xlog.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 21c0acb740..9ebfbf31c5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -795,7 +795,7 @@ static int    readFile = -1;
 static XLogSegNo readSegNo = 0;
 static uint32 readOff = 0;
 static uint32 readLen = 0;
-static XLogSource readSource = 0;    /* XLOG_FROM_* code */
+static XLogSource readSource = XLOG_FROM_ANY;
 
 /*
  * Keeps track of which source we're currently reading from. This is
@@ -804,7 +804,7 @@ static XLogSource readSource = 0;    /* XLOG_FROM_* code */
  * attempt to read from currentSource failed, and we should try another source
  * next.
  */
-static XLogSource currentSource = 0;    /* XLOG_FROM_* code */
+static XLogSource currentSource = XLOG_FROM_ANY;
 static bool lastSourceFailed = false;
 
 typedef struct XLogPageReadPrivate
@@ -823,7 +823,7 @@ typedef struct XLogPageReadPrivate
  * XLogReceiptSource tracks where we last successfully read some WAL.)
  */
 static TimestampTz XLogReceiptTime = 0;
-static XLogSource XLogReceiptSource = 0;    /* XLOG_FROM_* code */
+static XLogSource XLogReceiptSource = XLOG_FROM_ANY;
 
 /* State information for XLOG reading */
 static XLogRecPtr ReadRecPtr;    /* start of last record read */
@@ -886,8 +886,8 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
                                    bool find_free, XLogSegNo max_segno,
                                    bool use_lock);
 static int    XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
-                         int source, bool notfoundOk);
-static int    XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source);
+                         XLogSource source, bool notfoundOk);
+static int    XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
 static int    XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
                          int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
 static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
@@ -3633,7 +3633,7 @@ XLogFileOpen(XLogSegNo segno)
  */
 static int
 XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
-             int source, bool notfoundOk)
+             XLogSource source, bool notfoundOk)
 {
     char        xlogfname[MAXFNAMELEN];
     char        activitymsg[MAXFNAMELEN + 16];
@@ -3715,7 +3715,7 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
  * This version searches for the segment with any TLI listed in expectedTLEs.
  */
 static int
-XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source)
+XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source)
 {
     char        path[MAXPGPATH];
     ListCell   *cell;
@@ -4387,7 +4387,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
                  * so that we will check the archive next.
                  */
                 lastSourceFailed = false;
-                currentSource = 0;
+                currentSource = XLOG_FROM_ANY;
 
                 continue;
             }
@@ -11669,7 +11669,7 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
 
         close(readFile);
         readFile = -1;
-        readSource = 0;
+        readSource = XLOG_FROM_ANY;
     }
 
     XLByteToSeg(targetPagePtr, readSegNo, wal_segment_size);
@@ -11689,7 +11689,7 @@ retry:
                 close(readFile);
             readFile = -1;
             readLen = 0;
-            readSource = 0;
+            readSource = XLOG_FROM_ANY;
 
             return -1;
         }
@@ -11795,7 +11795,7 @@ next_record_is_invalid:
         close(readFile);
     readFile = -1;
     readLen = 0;
-    readSource = 0;
+    readSource = XLOG_FROM_ANY;
 
     /* In standby-mode, keep trying */
     if (StandbyMode)
@@ -11860,7 +11860,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
      */
     if (!InArchiveRecovery)
         currentSource = XLOG_FROM_PG_WAL;
-    else if (currentSource == 0 ||
+    else if (currentSource == XLOG_FROM_ANY ||
              (!StandbyMode && currentSource == XLOG_FROM_STREAM))
     {
         lastSourceFailed = false; /* We haven't failed on the new source */
@@ -11869,7 +11869,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 
     for (;;)
     {
-        int            oldSource = currentSource;
+        XLogSource    oldSource = currentSource;
 
         /*
          * First check if we failed to read from the current source, and
-- 
2.18.2