Re: Remove page-read callback from XLogReaderState. - Mailing list pgsql-hackers

From Kyotaro Horiguchi
Subject Re: Remove page-read callback from XLogReaderState.
Date
Msg-id 20190710.131810.229433693.horikyota.ntt@gmail.com
Whole thread Raw
In response to Re: Remove page-read callback from XLogReaderState.  (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>)
List pgsql-hackers
Hello. The patch gets disliked by my tool chain. Fixed the usage
of PG_USED_FOR_ASSERTS_ONLY and rebased to bd56cd75d2.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 1dabdce6993b73408b950cb8c348c4999178b9a0 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 10:22:49 +0900
Subject: [PATCH 01/10] Define macros to make XLogReadRecord a state machine

To minimize apparent impact on code, use some macros as syntax
sugar. This is a similar stuff with ExecInterpExpr but a bit
different. The most significant difference is that this stuff allows
some functions are leaved midst of their work then continue. Roughly
speaking this is used as the follows.

enum retval
some_func()
{
  static .. internal_variables;

  XLR_SWITCH(INITIAL_STATUS);
  ...
  XLR_LEAVE(STATUS1, RETVAL_CONTINUE);
  ...
  XLR_LEAVE(STATUS2, RETVAL_CONTINUE2);
  ...
  XLR_SWITCH_END();

  XLR_RETURN(RETVAL_FINISH);
}

The caller uses the function as follows:

  while (some_func() != RETVAL_FINISH)
  {
    <do some work>;
  }
---
 src/backend/access/transam/xlogreader.c | 83 +++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 41dae916b4..69d20e1f2f 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -29,6 +29,89 @@
 #include "utils/memutils.h"
 #endif
 
+/*
+ * Use computed-goto-based state dispatch when computed gotos are available.
+ * But use a separate symbol so that it's easy to adjust locally in this file
+ * for development and testing.
+ */
+#ifdef HAVE_COMPUTED_GOTO
+#define XLR_USE_COMPUTED_GOTO
+#endif                            /* HAVE_COMPUTED_GOTO */
+
+/*
+ * The state machine functions relies on static local variables. They cannot
+ * be reentered after non-local exit using ereport/elog for consistency. The
+ * assertion macros protect the functions from reenter after non-local exit.
+ */
+#ifdef USE_ASSERT_CHECKING
+#define XLR_REENT_PROTECT_ENTER() \
+    do { Assert(!__xlr_running); __xlr_running = true; } while (0)
+#define XLR_REENT_PROTECT_LEAVE()\
+    do { __xlr_running = false; } while (0)
+#else
+#define XLR_REENT_PROTECT_ENTER()
+#define XLR_REENT_PROTECT_LEAVE()
+#endif
+
+/*
+ * Macros for state dispatch.
+ *
+ * XLR_SWITCH - prologue code for state machine including switch itself.
+ * XLR_CASE - labels the implementation of named state.
+ * XLR_LEAVE - leave the function and return here at the next call.
+ * XLR_RETURN - return from the function and set state to initial state.
+ * XLR_END - just hides the closing brace if not in use.
+ */
+#if defined(XLR_USE_COMPUTED_GOTO)
+#define XLR_SWITCH(name)                                        \
+    static bool __xlr_running PG_USED_FOR_ASSERTS_ONLY = false; \
+    static void *__xlr_init_state = &&name;                                \
+    static void *__xlr_state = &&name;                                    \
+    do {                                                                \
+        XLR_REENT_PROTECT_ENTER();                                        \
+        goto *__xlr_state;                                                \
+        XLR_CASE(name);                                                    \
+    } while (0)
+#define XLR_CASE(name)        name:
+#define XLR_LEAVE(name, code)                                            \
+    do {                                                                \
+        __xlr_state = (&&name);                                            \
+        XLR_REENT_PROTECT_LEAVE();                                        \
+        return (code);                                                    \
+        XLR_CASE(name);                                                    \
+    } while (0)
+#define XLR_RETURN(code)                                        \
+    do {                                                        \
+        __xlr_state = __xlr_init_state;                            \
+        XLR_REENT_PROTECT_LEAVE();                                \
+        return (code);                                            \
+    } while (0)
+#define XLR_SWITCH_END()
+#else                            /* !XLR_USE_COMPUTED_GOTO */
+#define XLR_SWITCH(name)                                                \
+    static bool __xlr_running = false PG_USED_FOR_ASSERTS_ONLY;            \
+    static int __xlr_init_state = name;                                    \
+    static int __xlr_state = name;                                        \
+    XLR_REENT_PROTECT_ENTER();                                            \
+    switch (__xlr_state) {                                                \
+    XLR_CASE(name)
+#define XLR_CASE(name)        case name:
+#define XLR_LEAVE(name, code)                                            \
+    do {                                                                \
+        __xlr_state = (name);                                            \
+        XLR_REENT_PROTECT_LEAVE();                                        \
+        return (code);                                                    \
+        XLR_CASE(name);                                                    \
+    } while (0)
+#define XLR_RETURN(code)                        \
+    do {                                        \
+        __xlr_state = __xlr_init_state;                                    \
+        XLR_REENT_PROTECT_LEAVE();                                        \
+        return (code);                                                    \
+    } while (0)
+#define XLR_SWITCH_END()    }
+#endif                            /* XLR_USE_COMPUTED_GOTO */
+
 static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
 
 static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
-- 
2.16.3

From 6c36b3c428ac1160b4211710331c1cfee5f83ab9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 17 Apr 2019 14:15:16 +0900
Subject: [PATCH 02/10] Make ReadPageInternal a state machine

This patch set aims to remove read_page call back from
XLogReaderState. This is done in two steps. This patch does the first
step. Makes ReadPageInternal, which currently calling read_page
callback, a state machine and move the caller sites to XLogReadRecord
and other direct callers of the callback.
---
 src/backend/access/transam/xlog.c              |  15 ++-
 src/backend/access/transam/xlogreader.c        | 180 +++++++++++++++----------
 src/backend/access/transam/xlogutils.c         |   8 +-
 src/backend/replication/logical/logicalfuncs.c |   6 +-
 src/backend/replication/walsender.c            |  10 +-
 src/bin/pg_rewind/parsexlog.c                  |  17 ++-
 src/bin/pg_waldump/pg_waldump.c                |   8 +-
 src/include/access/xlogreader.h                |  39 ++++--
 src/include/access/xlogutils.h                 |   2 +-
 src/include/replication/logicalfuncs.h         |   2 +-
 10 files changed, 177 insertions(+), 110 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b6c9353cbd..f517dec16c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -884,7 +884,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
 static int    XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
                          int source, bool notfoundOk);
 static int    XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source);
-static int    XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static void    XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
                          int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
                          TimeLineID *readTLI);
 static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
@@ -11518,7 +11518,7 @@ CancelBackup(void)
  * XLogPageRead() to try fetching the record from another source, or to
  * sleep and retry.
  */
-static int
+static void
 XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
              XLogRecPtr targetRecPtr, char *readBuf, TimeLineID *readTLI)
 {
@@ -11577,7 +11577,8 @@ retry:
             readLen = 0;
             readSource = 0;
 
-            return -1;
+            xlogreader->readLen = -1;
+            return;
         }
     }
 
@@ -11672,7 +11673,8 @@ retry:
         goto next_record_is_invalid;
     }
 
-    return readLen;
+    xlogreader->readLen = readLen;
+    return;
 
 next_record_is_invalid:
     lastSourceFailed = true;
@@ -11686,8 +11688,9 @@ next_record_is_invalid:
     /* In standby-mode, keep trying */
     if (StandbyMode)
         goto retry;
-    else
-        return -1;
+
+    xlogreader->readLen = -1;
+    return;
 }
 
 /*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 69d20e1f2f..45d201fc23 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -118,8 +118,8 @@ static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
                                   XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
 static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record,
                             XLogRecPtr recptr);
-static int    ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
-                             int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+                         int reqLen);
 static void report_invalid_record(XLogReaderState *state, const char *fmt,...) pg_attribute_printf(2, 3);
 
 static void ResetDecoder(XLogReaderState *state);
@@ -306,7 +306,6 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
     uint32        targetRecOff;
     uint32        pageHeaderSize;
     bool        gotheader;
-    int            readOff;
 
     /*
      * randAccess indicates whether to verify the previous-record pointer of
@@ -358,15 +357,17 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
      * byte to cover the whole record header, or at least the part of it that
      * fits on the same page.
      */
-    readOff = ReadPageInternal(state,
-                               targetPagePtr,
-                               Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ));
-    if (readOff < 0)
+    while (XLogNeedData(state, targetPagePtr,
+                        Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ)))
+        state->read_page(state, state->loadPagePtr, state->loadLen,
+                         state->currRecPtr, state->readBuf,
+                         &state->readPageTLI);
+    
+    if (state->readLen < 0)
         goto err;
 
     /*
-     * ReadPageInternal always returns at least the page header, so we can
-     * examine it now.
+     * We have loaded at least the page header, so we can examine it now.
      */
     pageHeaderSize = XLogPageHeaderSize((XLogPageHeader) state->readBuf);
     if (targetRecOff == 0)
@@ -392,8 +393,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
         goto err;
     }
 
-    /* ReadPageInternal has verified the page header */
-    Assert(pageHeaderSize <= readOff);
+    /* XLogNeedData has verified the page header */
+    Assert(pageHeaderSize <= state->readLen);
 
     /*
      * Read the record length.
@@ -470,14 +471,17 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
             targetPagePtr += XLOG_BLCKSZ;
 
             /* Wait for the next page to become available */
-            readOff = ReadPageInternal(state, targetPagePtr,
-                                       Min(total_len - gotlen + SizeOfXLogShortPHD,
-                                           XLOG_BLCKSZ));
+            while (XLogNeedData(state, targetPagePtr,
+                                Min(total_len - gotlen + SizeOfXLogShortPHD,
+                                    XLOG_BLCKSZ)))
+                state->read_page(state, state->loadPagePtr, state->loadLen,
+                                 state->currRecPtr, state->readBuf,
+                                 &state->readPageTLI);
 
-            if (readOff < 0)
+            if (state->readLen < 0)
                 goto err;
 
-            Assert(SizeOfXLogShortPHD <= readOff);
+            Assert(SizeOfXLogShortPHD <= state->readLen);
 
             /* Check that the continuation on next page looks valid */
             pageHeader = (XLogPageHeader) state->readBuf;
@@ -506,20 +510,28 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
             /* Append the continuation from this page to the buffer */
             pageHeaderSize = XLogPageHeaderSize(pageHeader);
 
-            if (readOff < pageHeaderSize)
-                readOff = ReadPageInternal(state, targetPagePtr,
-                                           pageHeaderSize);
+            if (state->readLen < pageHeaderSize)
+            {
+                while (XLogNeedData(state, targetPagePtr, pageHeaderSize))
+                    state->read_page(state, state->loadPagePtr, state->loadLen,
+                                     state->currRecPtr, state->readBuf,
+                                     &state->readPageTLI);
+            }
 
-            Assert(pageHeaderSize <= readOff);
+            Assert(pageHeaderSize <= state->readLen);
 
             contdata = (char *) state->readBuf + pageHeaderSize;
             len = XLOG_BLCKSZ - pageHeaderSize;
             if (pageHeader->xlp_rem_len < len)
                 len = pageHeader->xlp_rem_len;
 
-            if (readOff < pageHeaderSize + len)
-                readOff = ReadPageInternal(state, targetPagePtr,
-                                           pageHeaderSize + len);
+            if (state->readLen < pageHeaderSize + len)
+            {
+                if (XLogNeedData(state, targetPagePtr, pageHeaderSize + len))
+                    state->read_page(state, state->loadPagePtr, state->loadLen,
+                                     state->currRecPtr, state->readBuf,
+                                     &state->readPageTLI);
+            }
 
             memcpy(buffer, (char *) contdata, len);
             buffer += len;
@@ -550,9 +562,13 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
     else
     {
         /* Wait for the record data to become available */
-        readOff = ReadPageInternal(state, targetPagePtr,
-                                   Min(targetRecOff + total_len, XLOG_BLCKSZ));
-        if (readOff < 0)
+        while (XLogNeedData(state, targetPagePtr,
+                            Min(targetRecOff + total_len, XLOG_BLCKSZ)))
+            state->read_page(state, state->loadPagePtr, state->loadLen,
+                             state->currRecPtr, state->readBuf,
+                             &state->readPageTLI);
+
+        if (state->readLen < 0)
             goto err;
 
         /* Record does not cross a page boundary */
@@ -595,39 +611,48 @@ err:
 }
 
 /*
- * Read a single xlog page including at least [pageptr, reqLen] of valid data
- * via the read_page() callback.
+ * Checks that an xlog page loaded in state->readBuf is including at least
+ * [pageptr, reqLen] and the page is valid.
  *
- * Returns -1 if the required page cannot be read for some reason; errormsg_buf
- * is set in that case (unless the error occurs in the read_page callback).
+ * Returns false if the required data is fully loaded. state->readLen is set to
+ * -1 when the loaded data is found to be invalid.
  *
- * We fetch the page from a reader-local cache if we know we have the required
- * data and if there hasn't been any error since caching the data.
+ * Otherwise, returns true and requests data using state->loadPagePtr and
+ * state->loadLen. The caller should load the region to state->readBuf and
+ * call this function again.
+ *
+ * Note: This function is not reentrant. The state is maintained internally in
+ * the function. DO NOT ereport(ERROR)-out from inside of this function.
  */
-static int
-ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
+static bool
+XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
 {
-    int            readLen;
-    uint32        targetPageOff;
-    XLogSegNo    targetSegNo;
-    XLogPageHeader hdr;
+    /*
+     * This function is a state machine that can exit and reenter at any place
+     * marked as XLR_LEAVE. All internal state is preserved through multiple
+     * calls.
+     */
+    static uint32        targetPageOff;
+    static XLogSegNo    targetSegNo;
+    static XLogPageHeader hdr;
 
-    Assert((pageptr % XLOG_BLCKSZ) == 0);
+    XLR_SWITCH (XLND_STATE_INIT);
 
     XLByteToSeg(pageptr, targetSegNo, state->wal_segment_size);
     targetPageOff = XLogSegmentOffset(pageptr, state->wal_segment_size);
+    Assert((pageptr % XLOG_BLCKSZ) == 0);
 
     /* check whether we have all the requested data already */
     if (targetSegNo == state->readSegNo && targetPageOff == state->readOff &&
         reqLen <= state->readLen)
-        return state->readLen;
+        XLR_RETURN(false);
 
     /*
      * Data is not in our buffer.
      *
      * Every time we actually read the page, even if we looked at parts of it
-     * before, we need to do verification as the read_page callback might now
-     * be rereading data from a different source.
+     * before, we need to do verification as the caller might now be rereading
+     * data from a different source.
      *
      * Whenever switching to a new WAL segment, we read the first page of the
      * file and validate its header, even if that's not where the target
@@ -636,18 +661,17 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
      */
     if (targetSegNo != state->readSegNo && targetPageOff != 0)
     {
-        XLogRecPtr    targetSegmentPtr = pageptr - targetPageOff;
+        state->loadPagePtr = pageptr - targetPageOff;
+        state->loadLen = XLOG_BLCKSZ;
+        XLR_LEAVE(XLND_STATE_SEGHEADER, true);
 
-        readLen = state->read_page(state, targetSegmentPtr, XLOG_BLCKSZ,
-                                   state->currRecPtr,
-                                   state->readBuf, &state->readPageTLI);
-        if (readLen < 0)
+        if (state->readLen < 0)
             goto err;
 
         /* we can be sure to have enough WAL available, we scrolled back */
-        Assert(readLen == XLOG_BLCKSZ);
+        Assert(state->readLen == XLOG_BLCKSZ);
 
-        if (!XLogReaderValidatePageHeader(state, targetSegmentPtr,
+        if (!XLogReaderValidatePageHeader(state, state->loadPagePtr,
                                           state->readBuf))
             goto err;
     }
@@ -656,48 +680,53 @@ ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen)
      * First, read the requested data length, but at least a short page header
      * so that we can validate it.
      */
-    readLen = state->read_page(state, pageptr, Max(reqLen, SizeOfXLogShortPHD),
-                               state->currRecPtr,
-                               state->readBuf, &state->readPageTLI);
-    if (readLen < 0)
+    state->loadPagePtr = pageptr;
+    state->loadLen = Max(reqLen, SizeOfXLogShortPHD);
+    XLR_LEAVE(XLND_STATE_PAGEHEADER, true);
+
+    if (state->readLen < 0)
         goto err;
 
-    Assert(readLen <= XLOG_BLCKSZ);
+    Assert(state->readLen <= XLOG_BLCKSZ);
 
     /* Do we have enough data to check the header length? */
-    if (readLen <= SizeOfXLogShortPHD)
+    if (state->readLen <= SizeOfXLogShortPHD)
         goto err;
 
-    Assert(readLen >= reqLen);
+    Assert(state->readLen >= state->loadLen);
 
     hdr = (XLogPageHeader) state->readBuf;
 
     /* still not enough */
-    if (readLen < XLogPageHeaderSize(hdr))
+    if (state->readLen < XLogPageHeaderSize(hdr))
     {
-        readLen = state->read_page(state, pageptr, XLogPageHeaderSize(hdr),
-                                   state->currRecPtr,
-                                   state->readBuf, &state->readPageTLI);
-        if (readLen < 0)
+        state->loadPagePtr = pageptr;
+        state->loadLen = XLogPageHeaderSize(hdr);
+        XLR_LEAVE(XLND_STATE_PAGELONGHEADER, true);
+
+        if (state->readLen < 0)
             goto err;
     }
 
+    XLR_SWITCH_END();
+
     /*
      * Now that we know we have the full header, validate it.
      */
-    if (!XLogReaderValidatePageHeader(state, pageptr, (char *) hdr))
-        goto err;
+    if (!XLogReaderValidatePageHeader(state, pageptr, (char *) state->readBuf))
+            goto err;
 
     /* update read state information */
     state->readSegNo = targetSegNo;
     state->readOff = targetPageOff;
-    state->readLen = readLen;
 
-    return readLen;
+    XLR_RETURN(false);
 
 err:
     XLogReaderInvalReadState(state);
-    return -1;
+    state->readLen = -1;
+
+    XLR_RETURN(false);
 }
 
 /*
@@ -986,7 +1015,6 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
         XLogRecPtr    targetPagePtr;
         int            targetRecOff;
         uint32        pageHeaderSize;
-        int            readLen;
 
         /*
          * Compute targetRecOff. It should typically be equal or greater than
@@ -994,7 +1022,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
          * that, except when caller has explicitly specified the offset that
          * falls somewhere there or when we are skipping multi-page
          * continuation record. It doesn't matter though because
-         * ReadPageInternal() is prepared to handle that and will read at
+         * CheckPage() is prepared to handle that and will read at
          * least short page-header worth of data
          */
         targetRecOff = tmpRecPtr % XLOG_BLCKSZ;
@@ -1003,8 +1031,12 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
         targetPagePtr = tmpRecPtr - targetRecOff;
 
         /* Read the page containing the record */
-        readLen = ReadPageInternal(state, targetPagePtr, targetRecOff);
-        if (readLen < 0)
+        while(XLogNeedData(state, targetPagePtr, targetRecOff))
+            state->read_page(state, state->loadPagePtr, state->loadLen,
+                             state->currRecPtr, state->readBuf,
+                             &state->readPageTLI);
+
+        if (state->readLen < 0)
             goto err;
 
         header = (XLogPageHeader) state->readBuf;
@@ -1012,8 +1044,12 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
         pageHeaderSize = XLogPageHeaderSize(header);
 
         /* make sure we have enough data for the page header */
-        readLen = ReadPageInternal(state, targetPagePtr, pageHeaderSize);
-        if (readLen < 0)
+        while (XLogNeedData(state, targetPagePtr, pageHeaderSize))
+            state->read_page(state, state->loadPagePtr, state->loadLen,
+                             state->currRecPtr, state->readBuf,
+                             &state->readPageTLI);
+
+        if (state->readLen < 0)
             goto err;
 
         /* skip over potential continuation data */
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 10a663bae6..c853e1f0e3 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -907,7 +907,7 @@ XLogReadDetermineTimeline(XLogReaderState *state, XLogRecPtr wantPage, uint32 wa
  * exists for normal backends, so we have to do a check/sleep/repeat style of
  * loop for now.
  */
-int
+void
 read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
                      int reqLen, XLogRecPtr targetRecPtr, char *cur_page,
                      TimeLineID *pageTLI)
@@ -1009,7 +1009,8 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
     else if (targetPagePtr + reqLen > read_upto)
     {
         /* not enough data there */
-        return -1;
+        state->readLen = -1;
+        return;
     }
     else
     {
@@ -1026,5 +1027,6 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
              XLOG_BLCKSZ);
 
     /* number of valid bytes in the buffer */
-    return count;
+    state->readLen = count;
+    return;
 }
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index d974400d6e..16c6095179 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -114,12 +114,12 @@ check_permissions(void)
                  (errmsg("must be superuser or replication role to use replication slots"))));
 }
 
-int
+void
 logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
                              int reqLen, XLogRecPtr targetRecPtr, char *cur_page, TimeLineID *pageTLI)
 {
-    return read_local_xlog_page(state, targetPagePtr, reqLen,
-                                targetRecPtr, cur_page, pageTLI);
+    read_local_xlog_page(state, targetPagePtr, reqLen,
+                         targetRecPtr, cur_page, pageTLI);
 }
 
 /*
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e7a59b0a92..130656e5b1 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -761,7 +761,7 @@ StartReplication(StartReplicationCmd *cmd)
  * which has to do a plain sleep/busy loop, because the walsender's latch gets
  * set every time WAL is flushed.
  */
-static int
+static void
 logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
                        XLogRecPtr targetRecPtr, char *cur_page, TimeLineID *pageTLI)
 {
@@ -779,7 +779,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
 
     /* fail if not (implies we are going to shut down) */
     if (flushptr < targetPagePtr + reqLen)
-        return -1;
+    {
+        state->readLen = -1;
+        return;
+    }
 
     if (targetPagePtr + XLOG_BLCKSZ <= flushptr)
         count = XLOG_BLCKSZ;    /* more than one block available */
@@ -789,7 +792,8 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
     /* now actually read the data, we know it's there */
     XLogRead(cur_page, targetPagePtr, XLOG_BLCKSZ);
 
-    return count;
+    state->readLen = count;
+    return;
 }
 
 /*
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 287af60c4e..0bfbea8f09 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -47,7 +47,7 @@ typedef struct XLogPageReadPrivate
     int            tliIndex;
 } XLogPageReadPrivate;
 
-static int    SimpleXLogPageRead(XLogReaderState *xlogreader,
+static void    SimpleXLogPageRead(XLogReaderState *xlogreader,
                                XLogRecPtr targetPagePtr,
                                int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
                                TimeLineID *pageTLI);
@@ -236,7 +236,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
 }
 
 /* XLogreader callback function, to read a WAL page */
-static int
+static void
 SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
                    int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
                    TimeLineID *pageTLI)
@@ -291,7 +291,8 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
         if (xlogreadfd < 0)
         {
             pg_log_error("could not open file \"%s\": %m", xlogfpath);
-            return -1;
+            xlogreader->readLen = -1;
+            return;
         }
     }
 
@@ -304,7 +305,8 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
     if (lseek(xlogreadfd, (off_t) targetPageOff, SEEK_SET) < 0)
     {
         pg_log_error("could not seek in file \"%s\": %m", xlogfpath);
-        return -1;
+        xlogreader->readLen = -1;
+        return;
     }
 
 
@@ -317,13 +319,16 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
             pg_log_error("could not read file \"%s\": read %d of %zu",
                          xlogfpath, r, (Size) XLOG_BLCKSZ);
 
-        return -1;
+        xlogreader->readLen = -1;
+        return;
     }
 
     Assert(targetSegNo == xlogreadsegno);
 
     *pageTLI = targetHistory[private->tliIndex].tli;
-    return XLOG_BLCKSZ;
+
+    xlogreader->readLen = XLOG_BLCKSZ;
+    return;
 }
 
 /*
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index b95d467805..f4dfdd12ca 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -421,7 +421,7 @@ XLogDumpXLogRead(const char *directory, TimeLineID timeline_id,
 /*
  * XLogReader read_page callback
  */
-static int
+static void
 XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
                  XLogRecPtr targetPtr, char *readBuff, TimeLineID *curFileTLI)
 {
@@ -437,14 +437,16 @@ XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
         else
         {
             private->endptr_reached = true;
-            return -1;
+            state->readLen = -1;
+            return;
         }
     }
 
     XLogDumpXLogRead(private->inpath, private->timeline, targetPagePtr,
                      readBuff, count);
 
-    return count;
+    state->readLen = count;
+    return;
 }
 
 /*
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 04228e2a87..d3b3e4b7ba 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -30,7 +30,7 @@
 typedef struct XLogReaderState XLogReaderState;
 
 /* Function type definition for the read_page callback */
-typedef int (*XLogPageReadCB) (XLogReaderState *xlogreader,
+typedef void (*XLogPageReadCB) (XLogReaderState *xlogreader,
                                XLogRecPtr targetPagePtr,
                                int reqLen,
                                XLogRecPtr targetRecPtr,
@@ -66,6 +66,15 @@ typedef struct
     uint16        data_bufsz;
 } DecodedBkpBlock;
 
+/* internal state of XLogNeedData() */
+typedef enum xlnd_stateid
+{
+    XLND_STATE_INIT,
+    XLND_STATE_SEGHEADER,
+    XLND_STATE_PAGEHEADER,
+    XLND_STATE_PAGELONGHEADER
+} xlnd_stateid;
+
 struct XLogReaderState
 {
     /* ----------------------------------------
@@ -120,6 +129,22 @@ struct XLogReaderState
     XLogRecPtr    EndRecPtr;        /* end+1 of last record read */
 
 
+    /* ----------------------------------------
+     * Communication with page reader
+     * readBuf is XLOG_BLCKSZ bytes, valid up to at least readLen bytes.
+     *  ----------------------------------------
+     */
+    /* parameters to page reader */
+    XLogRecPtr    loadPagePtr;    /* Pointer to the page  */
+    int            loadLen;        /* wanted length in bytes */
+    char       *readBuf;        /* buffer to store data */
+    XLogRecPtr    currRecPtr;        /* beginning of the WAL record being read */
+
+    /* return from page reader */
+    int32        readLen;        /* bytes actually read, must be at least
+                                 * loadLen. -1 on error. */
+    TimeLineID    readPageTLI;    /* TLI for data currently in readBuf */
+
     /* ----------------------------------------
      * Decoded representation of current record
      *
@@ -145,17 +170,9 @@ struct XLogReaderState
      * ----------------------------------------
      */
 
-    /*
-     * Buffer for currently read page (XLOG_BLCKSZ bytes, valid up to at least
-     * readLen bytes)
-     */
-    char       *readBuf;
-    uint32        readLen;
-
-    /* last read segment, segment offset, TLI for data currently in readBuf */
+    /* last read segment and segment offset for data currently in readBuf */
     XLogSegNo    readSegNo;
     uint32        readOff;
-    TimeLineID    readPageTLI;
 
     /*
      * beginning of prior page read, and its TLI.  Doesn't necessarily
@@ -164,8 +181,6 @@ struct XLogReaderState
     XLogRecPtr    latestPagePtr;
     TimeLineID    latestPageTLI;
 
-    /* beginning of the WAL record being read. */
-    XLogRecPtr    currRecPtr;
     /* timeline to read it from, 0 if a lookup is required */
     TimeLineID    currTLI;
 
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index 4105b59904..be0c79d18c 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -47,7 +47,7 @@ extern Buffer XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
 extern Relation CreateFakeRelcacheEntry(RelFileNode rnode);
 extern void FreeFakeRelcacheEntry(Relation fakerel);

-extern int    read_local_xlog_page(XLogReaderState *state,
+extern void    read_local_xlog_page(XLogReaderState *state,
                                  XLogRecPtr targetPagePtr, int reqLen,
                                  XLogRecPtr targetRecPtr, char *cur_page,
                                  TimeLineID *pageTLI);
diff --git a/src/include/replication/logicalfuncs.h b/src/include/replication/logicalfuncs.h
index a9c178a9e6..b0306c54ab 100644
--- a/src/include/replication/logicalfuncs.h
+++ b/src/include/replication/logicalfuncs.h
@@ -11,7 +11,7 @@
 
 #include "replication/logical.h"
 
-extern int    logical_read_local_xlog_page(XLogReaderState *state,
+extern void    logical_read_local_xlog_page(XLogReaderState *state,
                                          XLogRecPtr targetPagePtr,
                                          int reqLen, XLogRecPtr targetRecPtr,
                                          char *cur_page, TimeLineID *pageTLI);
-- 
2.16.3

From 949d47fe75f33a2da7cbd44c6fd3e7116381e0a6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 24 May 2019 09:22:44 +0900
Subject: [PATCH 03/10] Change interface of XLogReadRecord

As a preparation to the second step, this patch changes the interface
of XLogReadRecord so that the function can request the callers for
reading more data.
---
 src/backend/access/transam/twophase.c          |  2 +-
 src/backend/access/transam/xlog.c              |  2 +-
 src/backend/access/transam/xlogreader.c        | 52 ++++++++++++++------------
 src/backend/replication/logical/logical.c      |  2 +-
 src/backend/replication/logical/logicalfuncs.c |  2 +-
 src/backend/replication/slotfuncs.c            |  2 +-
 src/backend/replication/walsender.c            |  2 +-
 src/bin/pg_rewind/parsexlog.c                  |  6 +--
 src/bin/pg_waldump/pg_waldump.c                |  2 +-
 src/include/access/xlogreader.h                | 13 ++++++-
 10 files changed, 49 insertions(+), 36 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 477709bbc2..653f685870 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1394,7 +1394,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
                  errmsg("out of memory"),
                  errdetail("Failed while allocating a WAL reading processor.")));
 
-    record = XLogReadRecord(xlogreader, lsn, &errormsg);
+    XLogReadRecord(xlogreader, lsn, &record, &errormsg);
     if (record == NULL)
         ereport(ERROR,
                 (errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f517dec16c..27ab6cc815 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4261,7 +4261,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
     {
         char       *errormsg;
 
-        record = XLogReadRecord(xlogreader, RecPtr, &errormsg);
+        XLogReadRecord(xlogreader, RecPtr, &record, &errormsg);
         ReadRecPtr = xlogreader->ReadRecPtr;
         EndRecPtr = xlogreader->EndRecPtr;
         if (record == NULL)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 45d201fc23..8bd0e2925d 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -285,20 +285,21 @@ allocate_recordbuf(XLogReaderState *state, uint32 reclength)
  * If RecPtr is valid, try to read a record at that position.  Otherwise
  * try to read a record just after the last one previously read.
  *
- * If the read_page callback fails to read the requested data, NULL is
- * returned.  The callback is expected to have reported the error; errormsg
- * is set to NULL.
+ * If the read_page callback fails to read the requested data, *record is set
+ * to NULL and XLREAD_FAIL is returned.  The callback is expected to have
+ * reported the error; errormsg is set to NULL.
  *
- * If the reading fails for some other reason, NULL is also returned, and
- * *errormsg is set to a string with details of the failure.
+ * If the reading fails for some other reason, *record is also set to NULL and
+ * XLREAD_FAIL is returned. *errormsg is set to a string with details of the
+ * failure.
  *
  * The returned pointer (or *errormsg) points to an internal buffer that's
  * valid until the next call to XLogReadRecord.
  */
-XLogRecord *
-XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
+XLogReadRecordResult
+XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
+               XLogRecord **record, char **errormsg)
 {
-    XLogRecord *record;
     XLogRecPtr    targetPagePtr;
     bool        randAccess;
     uint32        len,
@@ -405,8 +406,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
      * cannot access any other fields until we've verified that we got the
      * whole header.
      */
-    record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
-    total_len = record->xl_tot_len;
+    *record = (XLogRecord *) (state->readBuf + RecPtr % XLOG_BLCKSZ);
+    total_len = (*record)->xl_tot_len;
 
     /*
      * If the whole record header is on this page, validate it immediately.
@@ -418,7 +419,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
      */
     if (targetRecOff <= XLOG_BLCKSZ - SizeOfXLogRecord)
     {
-        if (!ValidXLogRecordHeader(state, RecPtr, state->ReadRecPtr, record,
+        if (!ValidXLogRecordHeader(state, RecPtr, state->ReadRecPtr, *record,
                                    randAccess))
             goto err;
         gotheader = true;
@@ -540,9 +541,9 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
             /* If we just reassembled the record header, validate it. */
             if (!gotheader)
             {
-                record = (XLogRecord *) state->readRecordBuf;
+                *record = (XLogRecord *) state->readRecordBuf;
                 if (!ValidXLogRecordHeader(state, RecPtr, state->ReadRecPtr,
-                                           record, randAccess))
+                                           *record, randAccess))
                     goto err;
                 gotheader = true;
             }
@@ -550,8 +551,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
 
         Assert(gotheader);
 
-        record = (XLogRecord *) state->readRecordBuf;
-        if (!ValidXLogRecord(state, record, RecPtr))
+        *record = (XLogRecord *) state->readRecordBuf;
+        if (!ValidXLogRecord(state, *record, RecPtr))
             goto err;
 
         pageHeaderSize = XLogPageHeaderSize((XLogPageHeader) state->readBuf);
@@ -572,7 +573,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
             goto err;
 
         /* Record does not cross a page boundary */
-        if (!ValidXLogRecord(state, record, RecPtr))
+        if (!ValidXLogRecord(state, *record, RecPtr))
             goto err;
 
         state->EndRecPtr = RecPtr + MAXALIGN(total_len);
@@ -583,18 +584,19 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
     /*
      * Special processing if it's an XLOG SWITCH record
      */
-    if (record->xl_rmid == RM_XLOG_ID &&
-        (record->xl_info & ~XLR_INFO_MASK) == XLOG_SWITCH)
+    if ((*record)->xl_rmid == RM_XLOG_ID &&
+        ((*record)->xl_info & ~XLR_INFO_MASK) == XLOG_SWITCH)
     {
         /* Pretend it extends to end of segment */
         state->EndRecPtr += state->wal_segment_size - 1;
         state->EndRecPtr -= XLogSegmentOffset(state->EndRecPtr, state->wal_segment_size);
     }
 
-    if (DecodeXLogRecord(state, record, errormsg))
-        return record;
-    else
-        return NULL;
+    if (DecodeXLogRecord(state, *record, errormsg))
+        return XLREAD_SUCCESS;
+
+    *record = NULL;
+    return XLREAD_FAIL;
 
 err:
 
@@ -607,7 +609,8 @@ err:
     if (state->errormsg_buf[0] != '\0')
         *errormsg = state->errormsg_buf;
 
-    return NULL;
+    *record = NULL;
+    return XLREAD_FAIL;
 }
 
 /*
@@ -1001,6 +1004,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
     XLogRecPtr    tmpRecPtr;
     XLogRecPtr    found = InvalidXLogRecPtr;
     XLogPageHeader header;
+    XLogRecord *record;
     char       *errormsg;
 
     Assert(!XLogRecPtrIsInvalid(RecPtr));
@@ -1089,7 +1093,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
      * because either we're at the first record after the beginning of a page
      * or we just jumped over the remaining data of a continuation.
      */
-    while (XLogReadRecord(state, tmpRecPtr, &errormsg) != NULL)
+    while (XLogReadRecord(state, tmpRecPtr, &record, &errormsg) == XLREAD_SUCCESS)
     {
         /* continue after the record */
         tmpRecPtr = InvalidXLogRecPtr;
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 9853be6d1c..4f383721eb 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -481,7 +481,7 @@ DecodingContextFindStartpoint(LogicalDecodingContext *ctx)
         char       *err = NULL;
 
         /* the read_page callback waits for new WAL */
-        record = XLogReadRecord(ctx->reader, startptr, &err);
+        XLogReadRecord(ctx->reader, startptr, &record, &err);
         if (err)
             elog(ERROR, "%s", err);
         if (!record)
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 16c6095179..6fc78d7445 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -289,7 +289,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
             XLogRecord *record;
             char       *errm = NULL;
 
-            record = XLogReadRecord(ctx->reader, startptr, &errm);
+            XLogReadRecord(ctx->reader, startptr, &record, &errm);
             if (errm)
                 elog(ERROR, "%s", errm);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 808a6f5b83..7db8e0d044 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -433,7 +433,7 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto)
              * Read records.  No changes are generated in fast_forward mode,
              * but snapbuilder/slot statuses are updated properly.
              */
-            record = XLogReadRecord(ctx->reader, startlsn, &errm);
+            XLogReadRecord(ctx->reader, startlsn, &record, &errm);
             if (errm)
                 elog(ERROR, "%s", errm);
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 130656e5b1..4ba43592ca 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2827,7 +2827,7 @@ XLogSendLogical(void)
      */
     WalSndCaughtUp = false;
 
-    record = XLogReadRecord(logical_decoding_ctx->reader, logical_startptr, &errm);
+    XLogReadRecord(logical_decoding_ctx->reader, logical_startptr, &record, &errm);
     logical_startptr = InvalidXLogRecPtr;
 
     /* xlog record was invalid */
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 0bfbea8f09..512005de1c 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -75,7 +75,7 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
 
     do
     {
-        record = XLogReadRecord(xlogreader, startpoint, &errormsg);
+        XLogReadRecord(xlogreader, startpoint, &record, &errormsg);
 
         if (record == NULL)
         {
@@ -127,7 +127,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
-    record = XLogReadRecord(xlogreader, ptr, &errormsg);
+    XLogReadRecord(xlogreader, ptr, &record, &errormsg);
     if (record == NULL)
     {
         if (errormsg)
@@ -190,7 +190,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
     {
         uint8        info;
 
-        record = XLogReadRecord(xlogreader, searchptr, &errormsg);
+        XLogReadRecord(xlogreader, searchptr, &record, &errormsg);
 
         if (record == NULL)
         {
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index f4dfdd12ca..41aa108215 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -1132,7 +1132,7 @@ main(int argc, char **argv)
     for (;;)
     {
         /* try to read the next record */
-        record = XLogReadRecord(xlogreader_state, first_record, &errormsg);
+        XLogReadRecord(xlogreader_state, first_record, &record, &errormsg);
         if (!record)
         {
             if (!config.follow || private.endptr_reached)
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index d3b3e4b7ba..0e734d27f1 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -66,6 +66,13 @@ typedef struct
     uint16        data_bufsz;
 } DecodedBkpBlock;
 
+/* Return code from XLogReadRecord */
+typedef enum XLogReadRecordResult
+{
+    XLREAD_SUCCESS,        /* record is successfully read */
+    XLREAD_FAIL            /* failed during reading a record */
+} XLogReadRecordResult;
+
 /* internal state of XLogNeedData() */
 typedef enum xlnd_stateid
 {
@@ -220,8 +227,10 @@ extern XLogReaderState *XLogReaderAllocate(int wal_segment_size,
 extern void XLogReaderFree(XLogReaderState *state);
 
 /* Read the next XLog record. Returns NULL on end-of-WAL or failure */
-extern struct XLogRecord *XLogReadRecord(XLogReaderState *state,
-                                         XLogRecPtr recptr, char **errormsg);
+extern XLogReadRecordResult XLogReadRecord(XLogReaderState *state,
+                                           XLogRecPtr recptr,
+                                           XLogRecord **record,
+                                           char **errormsg);
 
 /* Validate a page */
 extern bool XLogReaderValidatePageHeader(XLogReaderState *state,
-- 
2.16.3

From c3ac6cb8c4d4478053686d9485b74e912949c391 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Fri, 24 May 2019 09:33:29 +0900
Subject: [PATCH 04/10] Make XLogReadRecord a state machine

This patch moves the caller sites of the callback above XLogReadRecord
by making XLogReadRecord a state machine.
---
 src/backend/access/transam/twophase.c          |   8 +-
 src/backend/access/transam/xlog.c              |   8 +-
 src/backend/access/transam/xlogreader.c        | 108 ++++++++++++++++---------
 src/backend/replication/logical/logical.c      |  10 ++-
 src/backend/replication/logical/logicalfuncs.c |  10 ++-
 src/backend/replication/slotfuncs.c            |  10 ++-
 src/backend/replication/walsender.c            |  10 ++-
 src/bin/pg_rewind/parsexlog.c                  |  28 ++++++-
 src/bin/pg_waldump/pg_waldump.c                |  11 ++-
 src/include/access/xlogreader.h                |  12 +++
 10 files changed, 165 insertions(+), 50 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 653f685870..6feca69126 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1394,7 +1394,13 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
                  errmsg("out of memory"),
                  errdetail("Failed while allocating a WAL reading processor.")));
 
-    XLogReadRecord(xlogreader, lsn, &record, &errormsg);
+    while (XLogReadRecord(xlogreader, lsn, &record, &errormsg) ==
+           XLREAD_NEED_DATA)
+            xlogreader->read_page(xlogreader,
+                                  xlogreader->loadPagePtr, xlogreader->loadLen,
+                                  xlogreader->currRecPtr, xlogreader->readBuf,
+                                  &xlogreader->readPageTLI);
+
     if (record == NULL)
         ereport(ERROR,
                 (errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 27ab6cc815..573b49050d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4261,7 +4261,13 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
     {
         char       *errormsg;
 
-        XLogReadRecord(xlogreader, RecPtr, &record, &errormsg);
+        while (XLogReadRecord(xlogreader, RecPtr, &record, &errormsg)
+               == XLREAD_NEED_DATA)
+            xlogreader->read_page(xlogreader,
+                                  xlogreader->loadPagePtr, xlogreader->loadLen,
+                                  xlogreader->currRecPtr, xlogreader->readBuf,
+                                  &xlogreader->readPageTLI);
+
         ReadRecPtr = xlogreader->ReadRecPtr;
         EndRecPtr = xlogreader->EndRecPtr;
         if (record == NULL)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 8bd0e2925d..4ab0655af5 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -282,31 +282,57 @@ allocate_recordbuf(XLogReaderState *state, uint32 reclength)
 /*
  * Attempt to read an XLOG record.
  *
- * If RecPtr is valid, try to read a record at that position.  Otherwise
- * try to read a record just after the last one previously read.
+ * This function runs a state machine and may need to call several times until
+ * a record is read.
  *
- * If the read_page callback fails to read the requested data, *record is set
- * to NULL and XLREAD_FAIL is returned.  The callback is expected to have
- * reported the error; errormsg is set to NULL.
+ * At the initial state, if called with valid pRecPtr, try to read a record at
+ * that position.  If invalid pRecPtr is given try to read a record just after
+ * the last one previously read.
  *
- * If the reading fails for some other reason, *record is also set to NULL and
- * XLREAD_FAIL is returned. *errormsg is set to a string with details of the
- * failure.
+ * When a record is successfully read, returns XLREAD_SUCCESS with result
+ * record being stored in *record then the state machine is reset to initial
+ * state.
+ *
+ * Returns XLREAD_NEED_DATA if needs more data fed.  In that case loadPagePtr
+ * and loadLen in state is set to inform the required WAL data. The caller
+ * shall read in the requested data into readBuf and set readLen and
+ * readPageTLI to the length of the data actually read and the TLI for the
+ * data read in respectively. In case of failure the caller shall call the
+ * function setting readLen to -1 and storing error message in errormsg_buf to
+ * inform error.
+ *
+ * If the reading fails for some reasons including caller-side error mentioned
+ * above, returns XLREAD_FAIL with *record being set to NULL. *errormsg is set
+ * to a string with details of the failure. The state machine is reset to
+ * initial state.
  *
  * The returned pointer (or *errormsg) points to an internal buffer that's
  * valid until the next call to XLogReadRecord.
+ *
+ * Note: This function is not reentrant. The state is maintained internally in
+ * the function. DO NOT non-local exit (ereport) from inside of this function.
  */
 XLogReadRecordResult
-XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
+XLogReadRecord(XLogReaderState *state, XLogRecPtr pRecPtr,
                XLogRecord **record, char **errormsg)
 {
-    XLogRecPtr    targetPagePtr;
-    bool        randAccess;
-    uint32        len,
-                total_len;
-    uint32        targetRecOff;
-    uint32        pageHeaderSize;
-    bool        gotheader;
+    /*
+     * This function is a state machine that can exit and reenter at any place
+     * marked as XLR_LEAVE. All internal state needs to be preserved through
+     * multiple calls.
+     */
+    static XLogRecPtr    targetPagePtr;
+    static bool            randAccess;
+    static uint32        len,
+                        total_len;
+    static uint32        targetRecOff;
+    static uint32        pageHeaderSize;
+    static bool            gotheader;
+    static XLogRecPtr    RecPtr;
+
+    XLR_SWITCH(XLREAD_STATE_INIT);
+
+    RecPtr = pRecPtr;
 
     /*
      * randAccess indicates whether to verify the previous-record pointer of
@@ -360,10 +386,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
      */
     while (XLogNeedData(state, targetPagePtr,
                         Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ)))
-        state->read_page(state, state->loadPagePtr, state->loadLen,
-                         state->currRecPtr, state->readBuf,
-                         &state->readPageTLI);
-    
+        XLR_LEAVE(XLREAD_STATE_PAGE, XLREAD_NEED_DATA);
+
     if (state->readLen < 0)
         goto err;
 
@@ -442,10 +466,10 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
     if (total_len > len)
     {
         /* Need to reassemble record */
-        char       *contdata;
-        XLogPageHeader pageHeader;
-        char       *buffer;
-        uint32        gotlen;
+        static char       *contdata;
+        static XLogPageHeader pageHeader;
+        static char       *buffer;
+        static uint32    gotlen;
 
         /*
          * Enlarge readRecordBuf as needed.
@@ -475,9 +499,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
             while (XLogNeedData(state, targetPagePtr,
                                 Min(total_len - gotlen + SizeOfXLogShortPHD,
                                     XLOG_BLCKSZ)))
-                state->read_page(state, state->loadPagePtr, state->loadLen,
-                                 state->currRecPtr, state->readBuf,
-                                 &state->readPageTLI);
+                XLR_LEAVE(XLREAD_STATE_CONTPAGE, XLREAD_NEED_DATA);
 
             if (state->readLen < 0)
                 goto err;
@@ -514,9 +536,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
             if (state->readLen < pageHeaderSize)
             {
                 while (XLogNeedData(state, targetPagePtr, pageHeaderSize))
-                    state->read_page(state, state->loadPagePtr, state->loadLen,
-                                     state->currRecPtr, state->readBuf,
-                                     &state->readPageTLI);
+                    XLR_LEAVE(XLREAD_STATE_CONTPAGE_HEADER, XLREAD_NEED_DATA);
             }
 
             Assert(pageHeaderSize <= state->readLen);
@@ -529,9 +549,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
             if (state->readLen < pageHeaderSize + len)
             {
                 if (XLogNeedData(state, targetPagePtr, pageHeaderSize + len))
-                    state->read_page(state, state->loadPagePtr, state->loadLen,
-                                     state->currRecPtr, state->readBuf,
-                                     &state->readPageTLI);
+                    XLR_LEAVE(XLREAD_STATE_CONTRECORD, XLREAD_NEED_DATA);
             }
 
             memcpy(buffer, (char *) contdata, len);
@@ -565,9 +583,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
         /* Wait for the record data to become available */
         while (XLogNeedData(state, targetPagePtr,
                             Min(targetRecOff + total_len, XLOG_BLCKSZ)))
-            state->read_page(state, state->loadPagePtr, state->loadLen,
-                             state->currRecPtr, state->readBuf,
-                             &state->readPageTLI);
+            XLR_LEAVE(XLREAD_STATE_RECORD, XLREAD_NEED_DATA);
 
         if (state->readLen < 0)
             goto err;
@@ -581,6 +597,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
         state->ReadRecPtr = RecPtr;
     }
 
+    XLR_SWITCH_END();
+
     /*
      * Special processing if it's an XLOG SWITCH record
      */
@@ -593,10 +611,10 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr,
     }
 
     if (DecodeXLogRecord(state, *record, errormsg))
-        return XLREAD_SUCCESS;
+        XLR_RETURN(XLREAD_SUCCESS);
 
     *record = NULL;
-    return XLREAD_FAIL;
+    XLR_RETURN(XLREAD_FAIL);
 
 err:
 
@@ -610,7 +628,7 @@ err:
         *errormsg = state->errormsg_buf;
 
     *record = NULL;
-    return XLREAD_FAIL;
+    XLR_RETURN(XLREAD_FAIL);
 }
 
 /*
@@ -1005,6 +1023,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
     XLogRecPtr    found = InvalidXLogRecPtr;
     XLogPageHeader header;
     XLogRecord *record;
+    XLogReadRecordResult result;
     char       *errormsg;
 
     Assert(!XLogRecPtrIsInvalid(RecPtr));
@@ -1093,8 +1112,17 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
      * because either we're at the first record after the beginning of a page
      * or we just jumped over the remaining data of a continuation.
      */
-    while (XLogReadRecord(state, tmpRecPtr, &record, &errormsg) == XLREAD_SUCCESS)
+    while ((result = XLogReadRecord(state, tmpRecPtr, &record, &errormsg)) !=
+           XLREAD_FAIL)
     {
+        if (result == XLREAD_NEED_DATA)
+        {
+            state->read_page(state, state->loadPagePtr, state->loadLen,
+                             state->currRecPtr,    state->readBuf,
+                             &state->readPageTLI);
+            continue;
+        }
+
         /* continue after the record */
         tmpRecPtr = InvalidXLogRecPtr;
 
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 4f383721eb..06200ea2e9 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -481,7 +481,15 @@ DecodingContextFindStartpoint(LogicalDecodingContext *ctx)
         char       *err = NULL;
 
         /* the read_page callback waits for new WAL */
-        XLogReadRecord(ctx->reader, startptr, &record, &err);
+        while (XLogReadRecord(ctx->reader, startptr, &record, &err) ==
+               XLREAD_NEED_DATA)
+            ctx->reader->read_page(ctx->reader,
+                                   ctx->reader->loadPagePtr,
+                                   ctx->reader->loadLen,
+                                   ctx->reader->currRecPtr,
+                                   ctx->reader->readBuf,
+                                   &ctx->reader->readPageTLI);
+
         if (err)
             elog(ERROR, "%s", err);
         if (!record)
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 6fc78d7445..4d09255504 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -289,7 +289,15 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
             XLogRecord *record;
             char       *errm = NULL;
 
-            XLogReadRecord(ctx->reader, startptr, &record, &errm);
+            while (XLogReadRecord(ctx->reader, startptr, &record, &errm) ==
+                   XLREAD_NEED_DATA)
+                ctx->reader->read_page(ctx->reader,
+                                       ctx->reader->loadPagePtr,
+                                       ctx->reader->loadLen,
+                                       ctx->reader->currRecPtr,
+                                       ctx->reader->readBuf,
+                                       &ctx->reader->readPageTLI);
+
             if (errm)
                 elog(ERROR, "%s", errm);
 
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 7db8e0d044..f4f4a907ad 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -433,7 +433,15 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto)
              * Read records.  No changes are generated in fast_forward mode,
              * but snapbuilder/slot statuses are updated properly.
              */
-            XLogReadRecord(ctx->reader, startlsn, &record, &errm);
+            while (XLogReadRecord(ctx->reader, startlsn, &record, &errm) ==
+                   XLREAD_NEED_DATA)
+                ctx->reader->read_page(ctx->reader,
+                                       ctx->reader->loadPagePtr,
+                                       ctx->reader->loadLen,
+                                       ctx->reader->currRecPtr,
+                                       ctx->reader->readBuf,
+                                       &ctx->reader->readPageTLI);
+
             if (errm)
                 elog(ERROR, "%s", errm);
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 4ba43592ca..36e14ab822 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2827,7 +2827,15 @@ XLogSendLogical(void)
      */
     WalSndCaughtUp = false;
 
-    XLogReadRecord(logical_decoding_ctx->reader, logical_startptr, &record, &errm);
+    while (XLogReadRecord(logical_decoding_ctx->reader,
+                          logical_startptr, &record, &errm) == XLREAD_NEED_DATA)
+        logical_decoding_ctx->reader->read_page(logical_decoding_ctx->reader,
+                                   logical_decoding_ctx->reader->loadPagePtr,
+                                   logical_decoding_ctx->reader->loadLen,
+                                   logical_decoding_ctx->reader->currRecPtr,
+                                   logical_decoding_ctx->reader->readBuf,
+                                   &logical_decoding_ctx->reader->readPageTLI);
+
     logical_startptr = InvalidXLogRecPtr;
 
     /* xlog record was invalid */
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 512005de1c..e26127206c 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -75,7 +75,14 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
 
     do
     {
-        XLogReadRecord(xlogreader, startpoint, &record, &errormsg);
+        while (XLogReadRecord(xlogreader, startpoint, &record, &errormsg) ==
+               XLREAD_NEED_DATA)
+            xlogreader->read_page(xlogreader,
+                                  xlogreader->loadPagePtr,
+                                  xlogreader->loadLen,
+                                  xlogreader->currRecPtr,
+                                  xlogreader->readBuf,
+                                  &xlogreader->readPageTLI);
 
         if (record == NULL)
         {
@@ -127,7 +134,15 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
-    XLogReadRecord(xlogreader, ptr, &record, &errormsg);
+    while (XLogReadRecord(xlogreader, ptr, &record, &errormsg) ==
+           XLREAD_NEED_DATA)
+                xlogreader->read_page(xlogreader,
+                                      xlogreader->loadPagePtr,
+                                      xlogreader->loadLen,
+                                      xlogreader->currRecPtr,
+                                      xlogreader->readBuf,
+                                      &xlogreader->readPageTLI);
+
     if (record == NULL)
     {
         if (errormsg)
@@ -190,7 +205,14 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
     {
         uint8        info;
 
-        XLogReadRecord(xlogreader, searchptr, &record, &errormsg);
+        while (XLogReadRecord(xlogreader, searchptr, &record, &errormsg) ==
+               XLREAD_NEED_DATA)
+            xlogreader->read_page(xlogreader,
+                                  xlogreader->loadPagePtr,
+                                  xlogreader->loadLen,
+                                  xlogreader->currRecPtr,
+                                  xlogreader->readBuf,
+                                  &xlogreader->readPageTLI);
 
         if (record == NULL)
         {
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 41aa108215..e2e93f144a 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -1132,7 +1132,16 @@ main(int argc, char **argv)
     for (;;)
     {
         /* try to read the next record */
-        XLogReadRecord(xlogreader_state, first_record, &record, &errormsg);
+        while (XLogReadRecord(xlogreader_state,
+                              first_record, &record, &errormsg) ==
+               XLREAD_NEED_DATA)
+            xlogreader_state->read_page(xlogreader_state,
+                                        xlogreader_state->loadPagePtr,
+                                        xlogreader_state->loadLen,
+                                        xlogreader_state->currRecPtr,
+                                        xlogreader_state->readBuf,
+                                        &xlogreader_state->readPageTLI);
+
         if (!record)
         {
             if (!config.follow || private.endptr_reached)
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 0e734d27f1..bc0c642906 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -70,6 +70,7 @@ typedef struct
 typedef enum XLogReadRecordResult
 {
     XLREAD_SUCCESS,        /* record is successfully read */
+    XLREAD_NEED_DATA,    /* need more data. see XLogReadRecord. */
     XLREAD_FAIL            /* failed during reading a record */
 } XLogReadRecordResult;
 
@@ -82,6 +83,17 @@ typedef enum xlnd_stateid
     XLND_STATE_PAGELONGHEADER
 } xlnd_stateid;
 
+/* internal state of XLogReadRecord() */
+typedef enum xlread_stateid
+{
+    XLREAD_STATE_INIT,
+    XLREAD_STATE_PAGE,
+    XLREAD_STATE_CONTPAGE,
+    XLREAD_STATE_CONTPAGE_HEADER,
+    XLREAD_STATE_CONTRECORD,
+    XLREAD_STATE_RECORD
+} xlread_stateid;
+
 struct XLogReaderState
 {
     /* ----------------------------------------
-- 
2.16.3

From a6862ebfdc4a82b489c95ba40f89e680d26fac51 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 13:48:25 +0900
Subject: [PATCH 05/10] Make XLogPageRead not use callback but call the
 function directly

This patch replaces the call to the callback in XLogReadRecord with
direct call to the original function. Then invalidate the parameters
callback and private for XLogReaderAllocate.
---
 src/backend/access/transam/xlog.c | 44 +++++++++++++--------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 573b49050d..f7b2528f26 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -803,13 +803,6 @@ static XLogSource readSource = 0;    /* XLOG_FROM_* code */
 static XLogSource currentSource = 0;    /* XLOG_FROM_* code */
 static bool lastSourceFailed = false;
 
-typedef struct XLogPageReadPrivate
-{
-    int            emode;
-    bool        fetching_ckpt;    /* are we fetching a checkpoint record? */
-    bool        randAccess;
-} XLogPageReadPrivate;
-
 /*
  * These variables track when we last obtained some WAL data to process,
  * and where we got it from.  (XLogReceiptSource is initially the same as
@@ -884,9 +877,8 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
 static int    XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
                          int source, bool notfoundOk);
 static int    XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source);
-static void    XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
-                         int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
-                         TimeLineID *readTLI);
+static void XLogPageRead(XLogReaderState *xlogreader,
+                         bool fetching_ckpt, int emode, bool randAccess);
 static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
                                         bool fetching_ckpt, XLogRecPtr tliRecPtr);
 static int    emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
@@ -4247,12 +4239,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
            bool fetching_ckpt)
 {
     XLogRecord *record;
-    XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
-
-    /* Pass through parameters to XLogPageRead */
-    private->fetching_ckpt = fetching_ckpt;
-    private->emode = emode;
-    private->randAccess = (RecPtr != InvalidXLogRecPtr);
+    bool        randAccess = (RecPtr != InvalidXLogRecPtr);
 
     /* This is the first attempt to read this page. */
     lastSourceFailed = false;
@@ -4263,10 +4250,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
 
         while (XLogReadRecord(xlogreader, RecPtr, &record, &errormsg)
                == XLREAD_NEED_DATA)
-            xlogreader->read_page(xlogreader,
-                                  xlogreader->loadPagePtr, xlogreader->loadLen,
-                                  xlogreader->currRecPtr, xlogreader->readBuf,
-                                  &xlogreader->readPageTLI);
+            XLogPageRead(xlogreader, fetching_ckpt, emode, randAccess);
 
         ReadRecPtr = xlogreader->ReadRecPtr;
         EndRecPtr = xlogreader->EndRecPtr;
@@ -6214,7 +6198,6 @@ StartupXLOG(void)
     bool        backupFromStandby = false;
     DBState        dbstate_at_startup;
     XLogReaderState *xlogreader;
-    XLogPageReadPrivate private;
     bool        fast_promoted = false;
     struct stat st;
 
@@ -6355,8 +6338,7 @@ StartupXLOG(void)
         OwnLatch(&XLogCtl->recoveryWakeupLatch);
 
     /* Set up XLOG reader facility */
-    MemSet(&private, 0, sizeof(XLogPageReadPrivate));
-    xlogreader = XLogReaderAllocate(wal_segment_size, &XLogPageRead, &private);
+    xlogreader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
     if (!xlogreader)
         ereport(ERROR,
                 (errcode(ERRCODE_OUT_OF_MEMORY),
@@ -11525,12 +11507,14 @@ CancelBackup(void)
  * sleep and retry.
  */
 static void
-XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
-             XLogRecPtr targetRecPtr, char *readBuf, TimeLineID *readTLI)
+XLogPageRead(XLogReaderState *xlogreader,
+             bool fetching_ckpt, int emode, bool randAccess)
 {
-    XLogPageReadPrivate *private =
-    (XLogPageReadPrivate *) xlogreader->private_data;
-    int            emode = private->emode;
+    XLogRecPtr targetPagePtr    = xlogreader->loadPagePtr;
+    int reqLen                    = xlogreader->loadLen;
+    XLogRecPtr targetRecPtr        = xlogreader->currRecPtr;
+    char *readBuf                = xlogreader->readBuf;
+    TimeLineID *readTLI            = &xlogreader->readPageTLI;
     uint32        targetPageOff;
     XLogSegNo    targetSegNo PG_USED_FOR_ASSERTS_ONLY;
     int            r;
@@ -11573,8 +11557,8 @@ retry:
          receivedUpto < targetPagePtr + reqLen))
     {
         if (!WaitForWALToBecomeAvailable(targetPagePtr + reqLen,
-                                         private->randAccess,
-                                         private->fetching_ckpt,
+                                         randAccess,
+                                         fetching_ckpt,
                                          targetRecPtr))
         {
             if (readFile >= 0)
-- 
2.16.3

From b233f84e3ce28b6634ae6f301088b54e6cfd0e4c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 15:02:19 +0900
Subject: [PATCH 06/10] Make XlogReadTwoPhaseData not use callback but call the
 function directly

This patch replaces the call to the callback in XlogReadTwoPhaseData
with direct call to the original function. Then invalidate the
parameters callback and private for XLogReaderAllocate.
---
 src/backend/access/transam/twophase.c          | 8 ++------
 src/backend/access/transam/xlogutils.c         | 8 +++++---
 src/backend/replication/logical/logicalfuncs.c | 8 ++++++--
 src/include/access/xlogutils.h                 | 5 +----
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6feca69126..f6d48368fe 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1386,8 +1386,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
     XLogReaderState *xlogreader;
     char       *errormsg;
 
-    xlogreader = XLogReaderAllocate(wal_segment_size, &read_local_xlog_page,
-                                    NULL);
+    xlogreader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
     if (!xlogreader)
         ereport(ERROR,
                 (errcode(ERRCODE_OUT_OF_MEMORY),
@@ -1396,10 +1395,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
 
     while (XLogReadRecord(xlogreader, lsn, &record, &errormsg) ==
            XLREAD_NEED_DATA)
-            xlogreader->read_page(xlogreader,
-                                  xlogreader->loadPagePtr, xlogreader->loadLen,
-                                  xlogreader->currRecPtr, xlogreader->readBuf,
-                                  &xlogreader->readPageTLI);
+        read_local_xlog_page(xlogreader);
 
     if (record == NULL)
         ereport(ERROR,
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index c853e1f0e3..fd461f16fc 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -908,10 +908,12 @@ XLogReadDetermineTimeline(XLogReaderState *state, XLogRecPtr wantPage, uint32 wa
  * loop for now.
  */
 void
-read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
-                     int reqLen, XLogRecPtr targetRecPtr, char *cur_page,
-                     TimeLineID *pageTLI)
+read_local_xlog_page(XLogReaderState *state)
 {
+    XLogRecPtr    targetPagePtr = state->loadPagePtr;
+    int            reqLen          = state->loadLen;
+    char       *cur_page      = state->readBuf;
+    TimeLineID *pageTLI          = &state->readPageTLI;
     XLogRecPtr    read_upto,
                 loc;
     int            count;
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 4d09255504..240a375d8f 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -118,8 +118,12 @@ void
 logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
                              int reqLen, XLogRecPtr targetRecPtr, char *cur_page, TimeLineID *pageTLI)
 {
-    read_local_xlog_page(state, targetPagePtr, reqLen,
-                         targetRecPtr, cur_page, pageTLI);
+    Assert(targetPagePtr == state->loadPagePtr &&
+           reqLen == state->loadLen &&
+           targetRecPtr == state->currRecPtr &&
+           cur_page == state->readBuf &&
+           pageTLI == &state->readPageTLI);
+    read_local_xlog_page(state);
 }
 
 /*
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index be0c79d18c..9724ce20b8 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -47,10 +47,7 @@ extern Buffer XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
 extern Relation CreateFakeRelcacheEntry(RelFileNode rnode);
 extern void FreeFakeRelcacheEntry(Relation fakerel);
 
-extern void    read_local_xlog_page(XLogReaderState *state,
-                                 XLogRecPtr targetPagePtr, int reqLen,
-                                 XLogRecPtr targetRecPtr, char *cur_page,
-                                 TimeLineID *pageTLI);
+extern void read_local_xlog_page(XLogReaderState *state);
 
 extern void XLogReadDetermineTimeline(XLogReaderState *state,
                                       XLogRecPtr wantPage, uint32 wantLength);
-- 
2.16.3

From 0682f7f9472f443f77505ce17745f98acf7f5a7c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 15:35:10 +0900
Subject: [PATCH 07/10] Make logical rep stuff not use callback but call the
 function directly

This is a bit different from the two before. This patch moves the
callback from XLogReaderState to LogicalDecodingContext. Then
invalidate the parameters callback and private for XLogReaderAllocate.
---
 src/backend/replication/logical/logical.c      | 17 +++++++----------
 src/backend/replication/logical/logicalfuncs.c | 17 +++--------------
 src/backend/replication/slotfuncs.c            |  7 +------
 src/backend/replication/walsender.c            | 15 +++++++--------
 src/include/replication/logical.h              | 13 +++++++++----
 src/include/replication/logicalfuncs.h         |  5 +----
 6 files changed, 28 insertions(+), 46 deletions(-)

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 06200ea2e9..dacd73e031 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -123,7 +123,7 @@ StartupDecodingContext(List *output_plugin_options,
                        TransactionId xmin_horizon,
                        bool need_full_snapshot,
                        bool fast_forward,
-                       XLogPageReadCB read_page,
+                       LogicalDecodingXLogReadPageCB read_page,
                        LogicalOutputPluginWriterPrepareWrite prepare_write,
                        LogicalOutputPluginWriterWrite do_write,
                        LogicalOutputPluginWriterUpdateProgress update_progress)
@@ -172,12 +172,14 @@ StartupDecodingContext(List *output_plugin_options,
 
     ctx->slot = slot;
 
-    ctx->reader = XLogReaderAllocate(wal_segment_size, read_page, ctx);
+    ctx->reader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
     if (!ctx->reader)
         ereport(ERROR,
                 (errcode(ERRCODE_OUT_OF_MEMORY),
                  errmsg("out of memory")));
 
+    ctx->read_page = read_page;
+
     ctx->reorder = ReorderBufferAllocate();
     ctx->snapshot_builder =
         AllocateSnapshotBuilder(ctx->reorder, xmin_horizon, start_lsn,
@@ -231,7 +233,7 @@ CreateInitDecodingContext(char *plugin,
                           List *output_plugin_options,
                           bool need_full_snapshot,
                           XLogRecPtr restart_lsn,
-                          XLogPageReadCB read_page,
+                          LogicalDecodingXLogReadPageCB read_page,
                           LogicalOutputPluginWriterPrepareWrite prepare_write,
                           LogicalOutputPluginWriterWrite do_write,
                           LogicalOutputPluginWriterUpdateProgress update_progress)
@@ -373,7 +375,7 @@ LogicalDecodingContext *
 CreateDecodingContext(XLogRecPtr start_lsn,
                       List *output_plugin_options,
                       bool fast_forward,
-                      XLogPageReadCB read_page,
+                      LogicalDecodingXLogReadPageCB read_page,
                       LogicalOutputPluginWriterPrepareWrite prepare_write,
                       LogicalOutputPluginWriterWrite do_write,
                       LogicalOutputPluginWriterUpdateProgress update_progress)
@@ -483,12 +485,7 @@ DecodingContextFindStartpoint(LogicalDecodingContext *ctx)
         /* the read_page callback waits for new WAL */
         while (XLogReadRecord(ctx->reader, startptr, &record, &err) ==
                XLREAD_NEED_DATA)
-            ctx->reader->read_page(ctx->reader,
-                                   ctx->reader->loadPagePtr,
-                                   ctx->reader->loadLen,
-                                   ctx->reader->currRecPtr,
-                                   ctx->reader->readBuf,
-                                   &ctx->reader->readPageTLI);
+            ctx->read_page(ctx);
 
         if (err)
             elog(ERROR, "%s", err);
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 240a375d8f..2a3f6d3cde 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -115,15 +115,9 @@ check_permissions(void)
 }
 
 void
-logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
-                             int reqLen, XLogRecPtr targetRecPtr, char *cur_page, TimeLineID *pageTLI)
+logical_read_local_xlog_page(LogicalDecodingContext *ctx)
 {
-    Assert(targetPagePtr == state->loadPagePtr &&
-           reqLen == state->loadLen &&
-           targetRecPtr == state->currRecPtr &&
-           cur_page == state->readBuf &&
-           pageTLI == &state->readPageTLI);
-    read_local_xlog_page(state);
+    read_local_xlog_page(ctx->reader);
 }
 
 /*
@@ -295,12 +289,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
 
             while (XLogReadRecord(ctx->reader, startptr, &record, &errm) ==
                    XLREAD_NEED_DATA)
-                ctx->reader->read_page(ctx->reader,
-                                       ctx->reader->loadPagePtr,
-                                       ctx->reader->loadLen,
-                                       ctx->reader->currRecPtr,
-                                       ctx->reader->readBuf,
-                                       &ctx->reader->readPageTLI);
+                ctx->read_page(ctx);
 
             if (errm)
                 elog(ERROR, "%s", errm);
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index f4f4a907ad..8675c9203e 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -435,12 +435,7 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto)
              */
             while (XLogReadRecord(ctx->reader, startlsn, &record, &errm) ==
                    XLREAD_NEED_DATA)
-                ctx->reader->read_page(ctx->reader,
-                                       ctx->reader->loadPagePtr,
-                                       ctx->reader->loadLen,
-                                       ctx->reader->currRecPtr,
-                                       ctx->reader->readBuf,
-                                       &ctx->reader->readPageTLI);
+                ctx->read_page(ctx);
 
             if (errm)
                 elog(ERROR, "%s", errm);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 36e14ab822..5bf8646c80 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -762,9 +762,13 @@ StartReplication(StartReplicationCmd *cmd)
  * set every time WAL is flushed.
  */
 static void
-logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
-                       XLogRecPtr targetRecPtr, char *cur_page, TimeLineID *pageTLI)
+logical_read_xlog_page(LogicalDecodingContext *ctx)
 {
+    XLogReaderState *state = ctx->reader;
+    XLogRecPtr        targetPagePtr = state->loadPagePtr;
+    int                reqLen          = state->loadLen;
+    char           *cur_page      = state->readBuf;
+
     XLogRecPtr    flushptr;
     int            count;
 
@@ -2829,12 +2833,7 @@ XLogSendLogical(void)
 
     while (XLogReadRecord(logical_decoding_ctx->reader,
                           logical_startptr, &record, &errm) == XLREAD_NEED_DATA)
-        logical_decoding_ctx->reader->read_page(logical_decoding_ctx->reader,
-                                   logical_decoding_ctx->reader->loadPagePtr,
-                                   logical_decoding_ctx->reader->loadLen,
-                                   logical_decoding_ctx->reader->currRecPtr,
-                                   logical_decoding_ctx->reader->readBuf,
-                                   &logical_decoding_ctx->reader->readPageTLI);
+        logical_decoding_ctx->read_page(logical_decoding_ctx);
 
     logical_startptr = InvalidXLogRecPtr;
 
diff --git a/src/include/replication/logical.h b/src/include/replication/logical.h
index f76b9eb4df..f0bec63241 100644
--- a/src/include/replication/logical.h
+++ b/src/include/replication/logical.h
@@ -32,7 +32,11 @@ typedef void (*LogicalOutputPluginWriterUpdateProgress) (
                                                          TransactionId xid
 );
 
-typedef struct LogicalDecodingContext
+typedef struct LogicalDecodingContext LogicalDecodingContext;
+
+typedef void (*LogicalDecodingXLogReadPageCB)(LogicalDecodingContext *ctx);
+
+struct LogicalDecodingContext
 {
     /* memory context this is all allocated in */
     MemoryContext context;
@@ -42,6 +46,7 @@ typedef struct LogicalDecodingContext
 
     /* infrastructure pieces for decoding */
     XLogReaderState *reader;
+    LogicalDecodingXLogReadPageCB read_page;
     struct ReorderBuffer *reorder;
     struct SnapBuild *snapshot_builder;
 
@@ -89,7 +94,7 @@ typedef struct LogicalDecodingContext
     bool        prepared_write;
     XLogRecPtr    write_location;
     TransactionId write_xid;
-} LogicalDecodingContext;
+};
 
 
 extern void CheckLogicalDecodingRequirements(void);
@@ -98,7 +103,7 @@ extern LogicalDecodingContext *CreateInitDecodingContext(char *plugin,
                                                          List *output_plugin_options,
                                                          bool need_full_snapshot,
                                                          XLogRecPtr restart_lsn,
-                                                         XLogPageReadCB read_page,
+                                                         LogicalDecodingXLogReadPageCB read_page,
                                                          LogicalOutputPluginWriterPrepareWrite prepare_write,
                                                          LogicalOutputPluginWriterWrite do_write,
                                                          LogicalOutputPluginWriterUpdateProgress update_progress);
@@ -106,7 +111,7 @@ extern LogicalDecodingContext *CreateDecodingContext(
                                                      XLogRecPtr start_lsn,
                                                      List *output_plugin_options,
                                                      bool fast_forward,
-                                                     XLogPageReadCB read_page,
+                                                     LogicalDecodingXLogReadPageCB read_page,
                                                      LogicalOutputPluginWriterPrepareWrite prepare_write,
                                                      LogicalOutputPluginWriterWrite do_write,
                                                      LogicalOutputPluginWriterUpdateProgress update_progress);
diff --git a/src/include/replication/logicalfuncs.h b/src/include/replication/logicalfuncs.h
index b0306c54ab..04a9fe10fa 100644
--- a/src/include/replication/logicalfuncs.h
+++ b/src/include/replication/logicalfuncs.h
@@ -11,9 +11,6 @@
 
 #include "replication/logical.h"
 
-extern void    logical_read_local_xlog_page(XLogReaderState *state,
-                                         XLogRecPtr targetPagePtr,
-                                         int reqLen, XLogRecPtr targetRecPtr,
-                                         char *cur_page, TimeLineID *pageTLI);
+extern void logical_read_local_xlog_page(LogicalDecodingContext *ctx);
 
 #endif
-- 
2.16.3

From 1f1ca75055bb17a1e9e877527f2094f12ac41ca6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 15:50:51 +0900
Subject: [PATCH 08/10] Make pg_waldump not use callback but call the function
 directly

This patch does the similar thing to the change in logical rep. Moves
callback from XLogReaderState from the parameter of
XLogFindNextRecord. Then invalidate the parameters callback and
private for XLogReaderAllocate.
---
 src/backend/access/transam/xlogreader.c | 15 +++++----------
 src/bin/pg_waldump/pg_waldump.c         | 21 +++++++++------------
 src/include/access/xlogreader.h         | 14 +++++---------
 3 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 4ab0655af5..004eaac021 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1016,7 +1016,8 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
  * debugging purposes.
  */
 XLogRecPtr
-XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
+XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr,
+                   XLogFindNextRecordCB read_page, void *private)
 {
     XLogReaderState saved_state = *state;
     XLogRecPtr    tmpRecPtr;
@@ -1055,9 +1056,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
 
         /* Read the page containing the record */
         while(XLogNeedData(state, targetPagePtr, targetRecOff))
-            state->read_page(state, state->loadPagePtr, state->loadLen,
-                             state->currRecPtr, state->readBuf,
-                             &state->readPageTLI);
+            read_page(state, private);
 
         if (state->readLen < 0)
             goto err;
@@ -1068,9 +1067,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
 
         /* make sure we have enough data for the page header */
         while (XLogNeedData(state, targetPagePtr, pageHeaderSize))
-            state->read_page(state, state->loadPagePtr, state->loadLen,
-                             state->currRecPtr, state->readBuf,
-                             &state->readPageTLI);
+            read_page(state, private);
 
         if (state->readLen < 0)
             goto err;
@@ -1117,9 +1114,7 @@ XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr)
     {
         if (result == XLREAD_NEED_DATA)
         {
-            state->read_page(state, state->loadPagePtr, state->loadLen,
-                             state->currRecPtr,    state->readBuf,
-                             &state->readPageTLI);
+            read_page(state, private);
             continue;
         }
 
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index e2e93f144a..8fe6823b32 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -422,10 +422,12 @@ XLogDumpXLogRead(const char *directory, TimeLineID timeline_id,
  * XLogReader read_page callback
  */
 static void
-XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
-                 XLogRecPtr targetPtr, char *readBuff, TimeLineID *curFileTLI)
+XLogDumpReadPage(XLogReaderState *state, void *priv)
 {
-    XLogDumpPrivate *private = state->private_data;
+    XLogRecPtr    targetPagePtr = state->loadPagePtr;
+    int            reqLen          = state->loadLen;
+    char       *readBuff      = state->readBuf;
+    XLogDumpPrivate *private  = (XLogDumpPrivate *) priv;
     int            count = XLOG_BLCKSZ;
 
     if (private->endptr != InvalidXLogRecPtr)
@@ -1102,13 +1104,13 @@ main(int argc, char **argv)
     /* done with argument parsing, do the actual work */
 
     /* we have everything we need, start reading */
-    xlogreader_state = XLogReaderAllocate(WalSegSz, XLogDumpReadPage,
-                                          &private);
+    xlogreader_state = XLogReaderAllocate(WalSegSz, NULL, NULL);
     if (!xlogreader_state)
         fatal_error("out of memory");
 
     /* first find a valid recptr to start from */
-    first_record = XLogFindNextRecord(xlogreader_state, private.startptr);
+    first_record = XLogFindNextRecord(xlogreader_state, private.startptr,
+                                      &XLogDumpReadPage, (void*) &private);
 
     if (first_record == InvalidXLogRecPtr)
         fatal_error("could not find a valid record after %X/%X",
@@ -1135,12 +1137,7 @@ main(int argc, char **argv)
         while (XLogReadRecord(xlogreader_state,
                               first_record, &record, &errormsg) ==
                XLREAD_NEED_DATA)
-            xlogreader_state->read_page(xlogreader_state,
-                                        xlogreader_state->loadPagePtr,
-                                        xlogreader_state->loadLen,
-                                        xlogreader_state->currRecPtr,
-                                        xlogreader_state->readBuf,
-                                        &xlogreader_state->readPageTLI);
+            XLogDumpReadPage(xlogreader_state, (void *) &private);
 
         if (!record)
         {
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index bc0c642906..b4ace71a75 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -29,14 +29,6 @@
 
 typedef struct XLogReaderState XLogReaderState;
 
-/* Function type definition for the read_page callback */
-typedef void (*XLogPageReadCB) (XLogReaderState *xlogreader,
-                               XLogRecPtr targetPagePtr,
-                               int reqLen,
-                               XLogRecPtr targetRecPtr,
-                               char *readBuf,
-                               TimeLineID *pageTLI);
-
 typedef struct
 {
     /* Is this block ref in use? */
@@ -252,7 +244,11 @@ extern bool XLogReaderValidatePageHeader(XLogReaderState *state,
 extern void XLogReaderInvalReadState(XLogReaderState *state);
 
 #ifdef FRONTEND
-extern XLogRecPtr XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr);
+/* Function type definition for the read_page callback */
+typedef void (*XLogFindNextRecordCB) (XLogReaderState *xlogreader,
+                                      void *private);
+extern XLogRecPtr XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr,
+                                     XLogFindNextRecordCB read_page, void *private);
 #endif                            /* FRONTEND */
 
 /* Functions for decoding an XLogRecord */
-- 
2.16.3

From 598c7adabcb9c0063e74632ae4486c83c4ff474e Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 16:00:57 +0900
Subject: [PATCH 09/10] Make pg_rewind not use callback but call the function
 directly

This patch replaces the call to the callback in pg_rewind with direct
call to the original function. Then invalidate the parameters callback
and private for XLogReaderAllocate.
---
 src/bin/pg_rewind/parsexlog.c | 78 ++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 56 deletions(-)

diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index e26127206c..26446027ab 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -41,16 +41,8 @@ static int    xlogreadfd = -1;
 static XLogSegNo xlogreadsegno = -1;
 static char xlogfpath[MAXPGPATH];
 
-typedef struct XLogPageReadPrivate
-{
-    const char *datadir;
-    int            tliIndex;
-} XLogPageReadPrivate;
-
-static void    SimpleXLogPageRead(XLogReaderState *xlogreader,
-                               XLogRecPtr targetPagePtr,
-                               int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
-                               TimeLineID *pageTLI);
+static void SimpleXLogPageRead(XLogReaderState *xlogreader,
+                               const char *datadir, int *tliIndex);
 
 /*
  * Read WAL from the datadir/pg_wal, starting from 'startpoint' on timeline
@@ -64,12 +56,8 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
     XLogRecord *record;
     XLogReaderState *xlogreader;
     char       *errormsg;
-    XLogPageReadPrivate private;
 
-    private.datadir = datadir;
-    private.tliIndex = tliIndex;
-    xlogreader = XLogReaderAllocate(WalSegSz, &SimpleXLogPageRead,
-                                    &private);
+    xlogreader = XLogReaderAllocate(WalSegSz, NULL, NULL);
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
@@ -77,12 +65,7 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
     {
         while (XLogReadRecord(xlogreader, startpoint, &record, &errormsg) ==
                XLREAD_NEED_DATA)
-            xlogreader->read_page(xlogreader,
-                                  xlogreader->loadPagePtr,
-                                  xlogreader->loadLen,
-                                  xlogreader->currRecPtr,
-                                  xlogreader->readBuf,
-                                  &xlogreader->readPageTLI);
+            SimpleXLogPageRead(xlogreader, datadir, &tliIndex);
 
         if (record == NULL)
         {
@@ -124,24 +107,15 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
     XLogRecord *record;
     XLogReaderState *xlogreader;
     char       *errormsg;
-    XLogPageReadPrivate private;
     XLogRecPtr    endptr;
 
-    private.datadir = datadir;
-    private.tliIndex = tliIndex;
-    xlogreader = XLogReaderAllocate(WalSegSz, &SimpleXLogPageRead,
-                                    &private);
+    xlogreader = XLogReaderAllocate(WalSegSz, NULL, NULL);
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
     while (XLogReadRecord(xlogreader, ptr, &record, &errormsg) ==
            XLREAD_NEED_DATA)
-                xlogreader->read_page(xlogreader,
-                                      xlogreader->loadPagePtr,
-                                      xlogreader->loadLen,
-                                      xlogreader->currRecPtr,
-                                      xlogreader->readBuf,
-                                      &xlogreader->readPageTLI);
+        SimpleXLogPageRead(xlogreader, datadir, &tliIndex);
 
     if (record == NULL)
     {
@@ -177,7 +151,6 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
     XLogRecPtr    searchptr;
     XLogReaderState *xlogreader;
     char       *errormsg;
-    XLogPageReadPrivate private;
 
     /*
      * The given fork pointer points to the end of the last common record,
@@ -193,10 +166,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
             forkptr += SizeOfXLogShortPHD;
     }
 
-    private.datadir = datadir;
-    private.tliIndex = tliIndex;
-    xlogreader = XLogReaderAllocate(WalSegSz, &SimpleXLogPageRead,
-                                    &private);
+    xlogreader = XLogReaderAllocate(WalSegSz, NULL, NULL);
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
@@ -207,12 +177,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
 
         while (XLogReadRecord(xlogreader, searchptr, &record, &errormsg) ==
                XLREAD_NEED_DATA)
-            xlogreader->read_page(xlogreader,
-                                  xlogreader->loadPagePtr,
-                                  xlogreader->loadLen,
-                                  xlogreader->currRecPtr,
-                                  xlogreader->readBuf,
-                                  &xlogreader->readPageTLI);
+            SimpleXLogPageRead(xlogreader, datadir, &tliIndex);
 
         if (record == NULL)
         {
@@ -259,11 +224,12 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
 
 /* XLogreader callback function, to read a WAL page */
 static void
-SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
-                   int reqLen, XLogRecPtr targetRecPtr, char *readBuf,
-                   TimeLineID *pageTLI)
+SimpleXLogPageRead(XLogReaderState *xlogreader,
+                   const char*datadir, int *tliIndex)
 {
-    XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
+    XLogRecPtr    targetPagePtr = xlogreader->loadPagePtr;
+    char       *readBuf          = xlogreader->readBuf;
+    TimeLineID *pageTLI          = &xlogreader->readPageTLI;
     uint32        targetPageOff;
     XLogRecPtr    targetSegEnd;
     XLogSegNo    targetSegNo;
@@ -296,17 +262,17 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
          * be done both forward and backward, consider also switching timeline
          * accordingly.
          */
-        while (private->tliIndex < targetNentries - 1 &&
-               targetHistory[private->tliIndex].end < targetSegEnd)
-            private->tliIndex++;
-        while (private->tliIndex > 0 &&
-               targetHistory[private->tliIndex].begin >= targetSegEnd)
-            private->tliIndex--;
+        while (*tliIndex < targetNentries - 1 &&
+               targetHistory[*tliIndex].end < targetSegEnd)
+            (*tliIndex)++;
+        while (*tliIndex > 0 &&
+               targetHistory[*tliIndex].begin >= targetSegEnd)
+            (*tliIndex)--;
 
-        XLogFileName(xlogfname, targetHistory[private->tliIndex].tli,
+        XLogFileName(xlogfname, targetHistory[*tliIndex].tli,
                      xlogreadsegno, WalSegSz);
 
-        snprintf(xlogfpath, MAXPGPATH, "%s/" XLOGDIR "/%s", private->datadir, xlogfname);
+        snprintf(xlogfpath, MAXPGPATH, "%s/" XLOGDIR "/%s", datadir, xlogfname);
 
         xlogreadfd = open(xlogfpath, O_RDONLY | PG_BINARY, 0);
 
@@ -347,7 +313,7 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
 
     Assert(targetSegNo == xlogreadsegno);
 
-    *pageTLI = targetHistory[private->tliIndex].tli;
+    *pageTLI = targetHistory[*tliIndex].tli;
 
     xlogreader->readLen = XLOG_BLCKSZ;
     return;
-- 
2.16.3

From 38ceaa8ada19b5fd34af14500d2f2307489b75da Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Apr 2019 16:15:18 +0900
Subject: [PATCH 10/10] Remove callback entry from XLogReaderState

This is the third (3/2?) step. Remove no-longer-useful members
read_page and private_data from XLogReaderState. Then change
XLogReaderAllocate not to take the parameters.
---
 src/backend/access/transam/twophase.c     |  2 +-
 src/backend/access/transam/xlog.c         |  4 ++--
 src/backend/access/transam/xlogreader.c   |  9 ++-------
 src/backend/replication/logical/logical.c |  2 +-
 src/bin/pg_rewind/parsexlog.c             |  6 +++---
 src/bin/pg_waldump/pg_waldump.c           |  2 +-
 src/include/access/xlogreader.h           | 32 +------------------------------
 7 files changed, 11 insertions(+), 46 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index f6d48368fe..e7a086b71e 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1386,7 +1386,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
     XLogReaderState *xlogreader;
     char       *errormsg;
 
-    xlogreader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
+    xlogreader = XLogReaderAllocate(wal_segment_size);
     if (!xlogreader)
         ereport(ERROR,
                 (errcode(ERRCODE_OUT_OF_MEMORY),
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f7b2528f26..546ea09996 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1187,7 +1187,7 @@ XLogInsertRecord(XLogRecData *rdata,
             appendBinaryStringInfo(&recordBuf, rdata->data, rdata->len);
 
         if (!debug_reader)
-            debug_reader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
+            debug_reader = XLogReaderAllocate(wal_segment_size);
 
         if (!debug_reader)
         {
@@ -6338,7 +6338,7 @@ StartupXLOG(void)
         OwnLatch(&XLogCtl->recoveryWakeupLatch);
 
     /* Set up XLOG reader facility */
-    xlogreader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
+    xlogreader = XLogReaderAllocate(wal_segment_size);
     if (!xlogreader)
         ereport(ERROR,
                 (errcode(ERRCODE_OUT_OF_MEMORY),
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 004eaac021..443a7b608f 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -149,8 +149,7 @@ report_invalid_record(XLogReaderState *state, const char *fmt,...)
  * Returns NULL if the xlogreader couldn't be allocated.
  */
 XLogReaderState *
-XLogReaderAllocate(int wal_segment_size, XLogPageReadCB pagereadfunc,
-                   void *private_data)
+XLogReaderAllocate(int wal_segment_size)
 {
     XLogReaderState *state;
 
@@ -178,11 +177,7 @@ XLogReaderAllocate(int wal_segment_size, XLogPageReadCB pagereadfunc,
     }
 
     state->wal_segment_size = wal_segment_size;
-    state->read_page = pagereadfunc;
-    /* system_identifier initialized to zeroes above */
-    state->private_data = private_data;
-    /* ReadRecPtr and EndRecPtr initialized to zeroes above */
-    /* readSegNo, readOff, readLen, readPageTLI initialized to zeroes above */
+    /* All members are initialized to zeroes above */
     state->errormsg_buf = palloc_extended(MAX_ERRORMSG_LEN + 1,
                                           MCXT_ALLOC_NO_OOM);
     if (!state->errormsg_buf)
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index dacd73e031..3724cff0fc 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -172,7 +172,7 @@ StartupDecodingContext(List *output_plugin_options,
 
     ctx->slot = slot;
 
-    ctx->reader = XLogReaderAllocate(wal_segment_size, NULL, NULL);
+    ctx->reader = XLogReaderAllocate(wal_segment_size);
     if (!ctx->reader)
         ereport(ERROR,
                 (errcode(ERRCODE_OUT_OF_MEMORY),
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 26446027ab..65420d0e4a 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -57,7 +57,7 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
     XLogReaderState *xlogreader;
     char       *errormsg;
 
-    xlogreader = XLogReaderAllocate(WalSegSz, NULL, NULL);
+    xlogreader = XLogReaderAllocate(WalSegSz);
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
@@ -109,7 +109,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
     char       *errormsg;
     XLogRecPtr    endptr;
 
-    xlogreader = XLogReaderAllocate(WalSegSz, NULL, NULL);
+    xlogreader = XLogReaderAllocate(WalSegSz);
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
@@ -166,7 +166,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
             forkptr += SizeOfXLogShortPHD;
     }
 
-    xlogreader = XLogReaderAllocate(WalSegSz, NULL, NULL);
+    xlogreader = XLogReaderAllocate(WalSegSz);
     if (xlogreader == NULL)
         pg_fatal("out of memory");
 
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 8fe6823b32..54fff797fc 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -1104,7 +1104,7 @@ main(int argc, char **argv)
     /* done with argument parsing, do the actual work */
 
     /* we have everything we need, start reading */
-    xlogreader_state = XLogReaderAllocate(WalSegSz, NULL, NULL);
+    xlogreader_state = XLogReaderAllocate(WalSegSz);
     if (!xlogreader_state)
         fatal_error("out of memory");
 
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index b4ace71a75..e83fc4da0e 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -98,40 +98,12 @@ struct XLogReaderState
      */
     int            wal_segment_size;
 
-    /*
-     * Data input callback (mandatory).
-     *
-     * This callback shall read at least reqLen valid bytes of the xlog page
-     * starting at targetPagePtr, and store them in readBuf.  The callback
-     * shall return the number of bytes read (never more than XLOG_BLCKSZ), or
-     * -1 on failure.  The callback shall sleep, if necessary, to wait for the
-     * requested bytes to become available.  The callback will not be invoked
-     * again for the same page unless more than the returned number of bytes
-     * are needed.
-     *
-     * targetRecPtr is the position of the WAL record we're reading.  Usually
-     * it is equal to targetPagePtr + reqLen, but sometimes xlogreader needs
-     * to read and verify the page or segment header, before it reads the
-     * actual WAL record it's interested in.  In that case, targetRecPtr can
-     * be used to determine which timeline to read the page from.
-     *
-     * The callback shall set *pageTLI to the TLI of the file the page was
-     * read from.  It is currently used only for error reporting purposes, to
-     * reconstruct the name of the WAL file where an error occurred.
-     */
-    XLogPageReadCB read_page;
-
     /*
      * System identifier of the xlog files we're about to read.  Set to zero
      * (the default value) if unknown or unimportant.
      */
     uint64        system_identifier;
 
-    /*
-     * Opaque data for callbacks to use.  Not used by XLogReader.
-     */
-    void       *private_data;
-
     /*
      * Start and end point of last record read.  EndRecPtr is also used as the
      * position to read next, if XLogReadRecord receives an invalid recptr.
@@ -223,9 +195,7 @@ struct XLogReaderState
 };
 
 /* Get a new XLogReader */
-extern XLogReaderState *XLogReaderAllocate(int wal_segment_size,
-                                           XLogPageReadCB pagereadfunc,
-                                           void *private_data);
+extern XLogReaderState *XLogReaderAllocate(int wal_segment_size);
 
 /* Free an XLogReader */
 extern void XLogReaderFree(XLogReaderState *state);
-- 
2.16.3


pgsql-hackers by date:

Previous
From: Dilip Kumar
Date:
Subject: Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?
Next
From: Paul A Jungwirth
Date:
Subject: Re: range_agg