Re: Remove page-read callback from XLogReaderState.
At Thu, 30 Sep 2021 09:40:06 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Mon, 27 Sep 2021 17:31:03 +1300, Thomas Munro
> wrote in
> > On Thu, Jul 15, 2021 at 4:48 PM Kyotaro Horiguchi
> > wrote:
> > > Gah... Thank you for noticing me. I thought that I have sent the
> > > rebased version. This is the rebased version on the current master.
> >
> > Hi Kyotaro,
> >
> > Did you see this?
> >
> > https://www.postgresql.org/message-id/20210429022553.4h5qii5jb5eclu4i%40alap3.anarazel.de
>
> Thank you for pinging me. I haven't noticed of that.
> I'll check on that line.
It looks like the XLogFindNextRecord was not finished. It should have
been turned into a state machine.
In this version (v18),
This contains only page-reader refactoring stuff.
- Rebased to the current master, including additional change for
XLOG_OVERWRITE_CONTRECORD stuff. (This needed the new function
XLogTerminateRead.)
- Finished XLogFindNextRecord including the fixup from Thomas' v17.
- Added a test for XLogFindNextRecord, on the behavior that
page-skipping on seeking for the first record.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 2da06a9de78e4f2125bcc889d12e7e3ffdf1d4d4 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 30 Sep 2021 11:48:40 +0900
Subject: [PATCH v18 1/5] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Redesign the interface so that it asks the caller for more
data when required. This model works better for proposed projects that
encryption, prefetching and other new features that would require
extending the callback interface for each case.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal(), then the remaining tasks of the
function are taken over by the new function XLogNeedData().
Author: Kyotaro HORIGUCHI
Author: Heikki Linnakangas
Reviewed-by: Antonin Houska
Reviewed-by: Alvaro Herrera
Reviewed-by: Takashi Menjo
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/20190418.210257.43726183.horiguchi.kyotaro%40lab.ntt.co.jp
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 325 +++-
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 31 ++-
src/include/access/xlogutils.h | 2 +-
8 files changed, 266 insertions(+), 159 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 26dcc00ac0..1557ceb8c1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -918,7 +918,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4401,7 +4401,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -12300,7 +12299,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -12359,7 +12358,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -12469,7 +12469,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -12483,8 +12484,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 4b03577dcc..1d9976ecf4 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -39,8 +39,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
Re: Remove page-read callback from XLogReaderState.
At Mon, 27 Sep 2021 17:31:03 +1300, Thomas Munro wrote in > On Thu, Jul 15, 2021 at 4:48 PM Kyotaro Horiguchi > wrote: > > Gah... Thank you for noticing me. I thought that I have sent the > > rebased version. This is the rebased version on the current master. > > Hi Kyotaro, > > Did you see this? > > https://www.postgresql.org/message-id/20210429022553.4h5qii5jb5eclu4i%40alap3.anarazel.de Thank you for pinging me. I haven't noticed of that. I'll check on that line. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On Thu, Jul 15, 2021 at 4:48 PM Kyotaro Horiguchi wrote: > Gah... Thank you for noticing me. I thought that I have sent the > rebased version. This is the rebased version on the current master. Hi Kyotaro, Did you see this? https://www.postgresql.org/message-id/20210429022553.4h5qii5jb5eclu4i%40alap3.anarazel.de
Re: Remove page-read callback from XLogReaderState.
At Thu, 15 Jul 2021 00:39:52 +0500, Ibrar Ahmed wrote
in
> On Wed, Jun 30, 2021 at 12:54 PM Kyotaro Horiguchi
> wrote:
> > Maybe I will rebase it soon.
> >
> > Yes, rebase is required, therefore I am changing the status to "Waiting On
> Author"
> http://cfbot.cputube.org/patch_33_2113.log
Gah... Thank you for noticing me. I thought that I have sent the
rebased version. This is the rebased version on the current master.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From cc36aefa3943e67d357be989044c951b5e6c0702 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Wed, 7 Jul 2021 13:10:34 +0900
Subject: [PATCH v21] Remove read_page callback from XLogReadRecord.
Previously, the XLogReader module would fetch new input data using a
callback function. Redesign the interface so that it tells the caller
to insert more data with a return value instead. This API suits later
patches for prefetching, encryption and probably other future features
that would otherwise require extending the callback interface for new
projects.
As incidental cleanup work, move global variables readOff, readLen and
readSegNo inside XlogReaderState.
Author: Kyotaro HORIGUCHI
Author: Heikki Linnakangas (earlier version)
Reviewed-by: Antonin Houska
Reviewed-by: Alvaro Herrera
Reviewed-by: Takashi Menjo
Reviewed-by: Andres Freund
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/20190418.210257.43726183.horiguchi.kyotaro%40lab.ntt.co.jp
---
src/backend/access/transam/twophase.c | 14 +-
src/backend/access/transam/xlog.c | 125 +--
src/backend/access/transam/xlogreader.c | 968 +++---
src/backend/access/transam/xlogutils.c| 25 +-
src/backend/replication/logical/logical.c | 26 +-
.../replication/logical/logicalfuncs.c| 13 +-
src/backend/replication/slotfuncs.c | 18 +-
src/backend/replication/walsender.c | 42 +-
src/bin/pg_rewind/parsexlog.c | 105 +-
src/bin/pg_waldump/pg_waldump.c | 141 +--
src/include/access/xlogreader.h | 157 +--
src/include/access/xlogutils.h| 4 +-
src/include/pg_config_manual.h| 2 +-
src/include/replication/logical.h | 11 +-
14 files changed, 955 insertions(+), 696 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6d3efb49a4..e0c1e8f334 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1330,11 +1330,8 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
char *errormsg;
TimeLineID save_currtli = ThisTimeLineID;
- xlogreader = XLogReaderAllocate(wal_segment_size, NULL,
- XL_ROUTINE(.page_read = _local_xlog_page,
- .segment_open = _segment_open,
- .segment_close = _segment_close),
- NULL);
+ xlogreader = XLogReaderAllocate(wal_segment_size, NULL, wal_segment_close);
+
if (!xlogreader)
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -1342,7 +1339,12 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
errdetail("Failed while allocating a WAL reading processor.")));
XLogBeginRead(xlogreader, lsn);
- record = XLogReadRecord(xlogreader, );
+ while (XLogReadRecord(xlogreader, , ) ==
+ XLREAD_NEED_DATA)
+ {
+ if (!read_local_xlog_page(xlogreader))
+ break;
+ }
/*
* Restore immediately the timeline where it was previously, as
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c7c928f50b..f9aea2a689 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -819,17 +819,13 @@ static XLogSegNo openLogSegNo = 0;
/*
* These variables are used similarly to the ones above, but for reading
- * the XLOG. readOff is the offset of the page just read, readLen
- * indicates how much of it has been read into readBuf, and readSource
+ * the XLOG. readOff is the offset of the page just read, and readSource
* indicates where we got the currently open file from.
* Note: we could use Reserve/ReleaseExternalFD to track consumption of
* this FD too; but it doesn't currently seem worthwhile, since the XLOG is
* not read by general-purpose sessions.
*/
static int readFile = -1;
-static XLogSegNo readSegNo = 0;
-static uint32 readOff = 0;
-static uint32 readLen = 0;
static XLogSource readSource = XLOG_FROM_ANY;
/*
@@ -846,13 +842,6 @@ static XLogSource currentSource = XLOG_FROM_ANY;
static bool lastSourceFailed = false;
static bool pendingWalRcvRestart = 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
@@ -927,10 +916,12 @@ static bool
Re: Remove page-read callback from XLogReaderState.
On Wed, Jun 30, 2021 at 12:54 PM Kyotaro Horiguchi wrote: > At Fri, 09 Apr 2021 09:36:59 +0900 (JST), Kyotaro Horiguchi < > horikyota@gmail.com> wrote in > > I'm surprised to see this pushed this soon. Thanks for pushing this! > > Then this has been reverted. I'm not sure how to check for the > possible defect happens on that platform, but, anyways I reverted the > CF item to "Needs Review" then moved to the next CF. > > Maybe I will rebase it soon. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > > > Yes, rebase is required, therefore I am changing the status to "Waiting On Author" http://cfbot.cputube.org/patch_33_2113.log -- Ibrar Ahmed
Re: Remove page-read callback from XLogReaderState.
At Fri, 09 Apr 2021 09:36:59 +0900 (JST), Kyotaro Horiguchi wrote in > I'm surprised to see this pushed this soon. Thanks for pushing this! Then this has been reverted. I'm not sure how to check for the possible defect happens on that platform, but, anyways I reverted the CF item to "Needs Review" then moved to the next CF. Maybe I will rebase it soon. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
At Thu, 8 Apr 2021 23:51:34 +1200, Thomas Munro wrote in > On Thu, Apr 8, 2021 at 9:46 PM Thomas Munro wrote: > > I squashed the patch set into one because half of them were fixups, > > and the two main patches were really parts of the same change and > > should go in together. > > > > I fixed a few compiler warnings (GCC 10.2 reported several > > uninitialised variables, comparisons that are always true, etc) and > > some comments. You can see these in the fixup patch. > > Pushed. Luckily there are plenty more improvements possible for > XLogReader/XLogDecoder in the next cycle. I'm surprised to see this pushed this soon. Thanks for pushing this! And thanks for fixing the remaining mistakes including some stupid ones.. At Thu, 8 Apr 2021 10:04:26 +1200, Thomas Munro wrote in > There is a stray elog(HOGE) :-) U! This looks like getting slipped-in while investigating another issue.. Thanks for preventing the repository from being contaminated by such a thing.. At Thu, 8 Apr 2021 21:46:06 +1200, Thomas Munro wrote in > I think maybe it it should really be XLogReaderSetInputData(state, > tli, data, size) in a later release. In the meantime, I changed it to > XLogReaderSetInputData(state, size), hope that name make sense... Sounds better. I didn't like that page-readers are allowed to touch XLogReaderStats.seg directly. Anyway it would be a small change. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On Thu, Apr 8, 2021 at 9:46 PM Thomas Munro wrote: > I squashed the patch set into one because half of them were fixups, > and the two main patches were really parts of the same change and > should go in together. > > I fixed a few compiler warnings (GCC 10.2 reported several > uninitialised variables, comparisons that are always true, etc) and > some comments. You can see these in the fixup patch. Pushed. Luckily there are plenty more improvements possible for XLogReader/XLogDecoder in the next cycle.
Re: Remove page-read callback from XLogReaderState.
I squashed the patch set into one because half of them were fixups,
and the two main patches were really parts of the same change and
should go in together.
I fixed a few compiler warnings (GCC 10.2 reported several
uninitialised variables, comparisons that are always true, etc) and
some comments. You can see these in the fixup patch.
+static inline void
+XLogReaderNotifySize(XLogReaderState *state, int32 len)
I think maybe it it should really be XLogReaderSetInputData(state,
tli, data, size) in a later release. In the meantime, I changed it to
XLogReaderSetInputData(state, size), hope that name make sense...
From aa5ed37bef02bdbee2e11ad54f028e6bac54816a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v20 1/5] Remove read_page callback from XLogReadRecord.
Previously, the XLogReader module would fetch new input data using a
callback function. Redesign the interface so that it tells the caller
to insert more data with a return value instead. This API suits later
patches for prefetching, encryption and probably other future features
that would otherwise require extending the callback interface for new
projects.
As incidental cleanup work, move global variables readOff, readLen and
readSegNo inside XlogReaderState.
Author: Kyotaro HORIGUCHI
Author: Heikki Linnakangas (earlier version)
Reviewed-by: Antonin Houska
Reviewed-by: Alvaro Herrera
Reviewed-by: Takashi Menjo
Reviewed-by: Andres Freund
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/20190418.210257.43726183.horiguchi.kyotaro%40lab.ntt.co.jp
---
src/backend/access/transam/twophase.c | 14 +-
src/backend/access/transam/xlog.c | 129 ++-
src/backend/access/transam/xlogreader.c | 932 +++---
src/backend/access/transam/xlogutils.c| 25 +-
src/backend/replication/logical/logical.c | 26 +-
.../replication/logical/logicalfuncs.c| 13 +-
src/backend/replication/slotfuncs.c | 18 +-
src/backend/replication/walsender.c | 42 +-
src/bin/pg_rewind/parsexlog.c | 107 +-
src/bin/pg_waldump/pg_waldump.c | 141 +--
src/include/access/xlogreader.h | 157 +--
src/include/access/xlogutils.h| 4 +-
src/include/pg_config_manual.h| 2 +-
src/include/replication/logical.h | 11 +-
14 files changed, 940 insertions(+), 681 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 89335b64a2..3137cb3ecc 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1330,11 +1330,8 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
char *errormsg;
TimeLineID save_currtli = ThisTimeLineID;
- xlogreader = XLogReaderAllocate(wal_segment_size, NULL,
- XL_ROUTINE(.page_read = _local_xlog_page,
- .segment_open = _segment_open,
- .segment_close = _segment_close),
- NULL);
+ xlogreader = XLogReaderAllocate(wal_segment_size, NULL, wal_segment_close);
+
if (!xlogreader)
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
@@ -1342,7 +1339,12 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
errdetail("Failed while allocating a WAL reading processor.")));
XLogBeginRead(xlogreader, lsn);
- record = XLogReadRecord(xlogreader, );
+ while (XLogReadRecord(xlogreader, , ) ==
+ XLREAD_NEED_DATA)
+ {
+ if (!read_local_xlog_page(xlogreader))
+ break;
+ }
/*
* Restore immediately the timeline where it was previously, as
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c1d4415a43..d3d6fb4643 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -811,17 +811,13 @@ static XLogSegNo openLogSegNo = 0;
* These variables are used similarly to the ones above, but for reading
* the XLOG. Note, however, that readOff generally represents the offset
* of the page just read, not the seek position of the FD itself, which
- * will be just past that page. readLen indicates how much of the current
- * page has been read into readBuf, and readSource indicates where we got
- * the currently open file from.
+ * will be just past that page. readSource indicates where we got the
+ * currently open file from.
* Note: we could use Reserve/ReleaseExternalFD to track consumption of
* this FD too; but it doesn't currently seem worthwhile, since the XLOG is
* not read by general-purpose sessions.
*/
static int readFile = -1;
-static XLogSegNo readSegNo = 0;
-static uint32 readOff = 0;
-static uint32 readLen = 0;
static XLogSource readSource = XLOG_FROM_ANY;
/*
@@ -838,13 +834,6 @@ static XLogSource currentSource = XLOG_FROM_ANY;
static bool lastSourceFailed = false;
static bool pendingWalRcvRestart = false;
-typedef struct XLogPageReadPrivate
-{
- int emode;
- bool fetching_ckpt;
Re: Remove page-read callback from XLogReaderState.
On Wed, Apr 7, 2021 at 8:50 PM Kyotaro Horiguchi
wrote:
> I haven't changed the name "XLog reader" to "XLog decoder". I'm doing
> that but it affects somewhat wide range of code.
Thanks for the new patch set! Let's not worry about renaming it for now.
This fails in check-world as seen on cfbot; I am not 100% sure but
this change fixes it:
@@ -1231,7 +1231,7 @@ XLogFindNextRecord(XLogFindNextRecordState *state)
{
/* Rewind the reader to the beginning of the
last record. */
state->currRecPtr = state->reader_state->ReadRecPtr;
- XLogBeginRead(state->reader_state, found);
+ XLogBeginRead(state->reader_state, state->currRecPtr);
The variable "found" seem to be useless.
I still see the 3 warnings mentioned earlier when compiling without
--enable-cassert.
There is a stray elog(HOGE) :-)
Re: Remove page-read callback from XLogReaderState.
At Tue, 6 Apr 2021 16:09:55 -0700, Andres Freund wrote in
> Hi,
>
> > XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int
> > reqLen,
> > XLogRecPtr targetRecPtr, char *readBuf)
> > {
> > @@ -12170,7 +12169,8 @@ retry:
> > readLen = 0;
> > readSource = XLOG_FROM_ANY;
> >
> > - return -1;
> > + xlogreader->readLen = -1;
> > + return false;
> > }
> > }
>
> It seems a bit weird to assign to XlogReaderState->readLen inside the
> callbacks. I first thought it was just a transient state, but it's
> not. I think it'd be good to wrap the xlogreader->readLen assignment an
> an inline function. That we can add more asserts etc over time.
Sounds reasonable. The variable is split up into request/result
variables and setting the result variable is wrapped by a
function. (0005).
> > -/* pg_waldump's XLogReaderRoutine->page_read callback */
> > +/*
> > + * pg_waldump's WAL page rader, also used as page_read callback for
> > + * XLogFindNextRecord
> > + */
> > static bool
> > -WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int
> > reqLen,
> > - XLogRecPtr targetPtr, char *readBuff)
> > +WALDumpReadPage(XLogReaderState *state, void *priv)
> > {
> > - XLogDumpPrivate *private = state->private_data;
> > + XLogRecPtr targetPagePtr = state->readPagePtr;
> > + int reqLen= state->readLen;
> > + char *readBuff = state->readBuf;
> > + XLogDumpPrivate *private = (XLogDumpPrivate *) priv;
>
> It seems weird to pass a void *priv to a function that now doesn't at
> all need the type punning anymore.
Mmm. I omitted it since client code was somewhat out-of-scope. In the
attached 0004 WALDumpReadPage() is no longer used as the callback of
XLogFindNextRecord.
On the way fixing them, I found that XLogReaderState.readPageTLI has
been moved to XLogReaderState.seg.ws_tli so I removed it from 0001.
I haven't changed the name "XLog reader" to "XLog decoder". I'm doing
that but it affects somewhat wide range of code.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From bda7ea561de0a8aec75eb50e8f07daaf96509583 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v19 1/5] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Redesign the interface so that it asks the caller for more
data when required. This model works better for proposed projects that
encryption, prefetching and other new features that would require
extending the callback interface for each case.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal(), then the remaining tasks of the
function are taken over by the new function XLogNeedData().
Author: Kyotaro HORIGUCHI
Author: Heikki Linnakangas
Reviewed-by: Antonin Houska
Reviewed-by: Alvaro Herrera
Reviewed-by: Takashi Menjo
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/20190418.210257.43726183.horiguchi.kyotaro%40lab.ntt.co.jp
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 325 +++-
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 32 ++-
src/include/access/xlogutils.h | 2 +-
8 files changed, 267 insertions(+), 159 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c1d4415a43..8085ca1117 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -920,7 +920,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4375,7 +4375,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -12107,7 +12106,7 @@ CancelBackup(void)
* XLogPageRead() to try
Re: Remove page-read callback from XLogReaderState.
On 2021-Apr-07, Thomas Munro wrote: > On Wed, Apr 7, 2021 at 11:18 AM Alvaro Herrera > wrote: > > BTRW it's funny that after these patches, "xlogreader" no longer reads > > anything. It's more an "xlog interpreter" -- the piece of code that > > splits individual WAL records from a stream of WAL bytes that's caller's > > responsibility to obtain somehow. But (and, again, I haven't read this > > patch recently) it still offers pieces that support a reader, in > > addition to its main interface as the interpreter. Maybe it's not a > > totally stupid idea to split it in even more different files. > > Yeah, I like "decoder", and it's already called that in some places... Yeah, that works ... -- Álvaro Herrera39°49'30"S 73°17'W "Los trabajadores menos efectivos son sistematicamente llevados al lugar donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)
Re: Remove page-read callback from XLogReaderState.
On Wed, Apr 7, 2021 at 11:18 AM Alvaro Herrera wrote: > BTRW it's funny that after these patches, "xlogreader" no longer reads > anything. It's more an "xlog interpreter" -- the piece of code that > splits individual WAL records from a stream of WAL bytes that's caller's > responsibility to obtain somehow. But (and, again, I haven't read this > patch recently) it still offers pieces that support a reader, in > addition to its main interface as the interpreter. Maybe it's not a > totally stupid idea to split it in even more different files. Yeah, I like "decoder", and it's already called that in some places...
Re: Remove page-read callback from XLogReaderState.
On 2021-Apr-07, Thomas Munro wrote: > I wonder if it would be better to have the client code access these > values through functions (even if they just access the variables in a > static inline function), to create a bit more separation? Something > like XLogReaderGetWanted(_lsn, _wanted), and then > XLogReaderSetAvailable(state, 42)? Just an idea. I think more opacity is good in this area, generally speaking. There are way too many globals, and they interact in nontrivial ways across the codebase. Just look at the ThisTimeLineID recent disaster. I don't have this patch sufficiently paged-in to say that bytes_wanted/ bytes_available is precisely the thing we need, but if it makes for a cleaner interface, I'm for it. This module keeps some state inside itself, and others part of the state is in its users; that's not good, and any cleanup on that is welcome. BTRW it's funny that after these patches, "xlogreader" no longer reads anything. It's more an "xlog interpreter" -- the piece of code that splits individual WAL records from a stream of WAL bytes that's caller's responsibility to obtain somehow. But (and, again, I haven't read this patch recently) it still offers pieces that support a reader, in addition to its main interface as the interpreter. Maybe it's not a totally stupid idea to split it in even more different files. -- Álvaro Herrera39°49'30"S 73°17'W
Re: Remove page-read callback from XLogReaderState.
Hi,
On 2021-04-07 05:09:53 +1200, Thomas Munro wrote:
> From 560cdfa444a3b05a0e6b8054f3cfeadf56e059fc Mon Sep 17 00:00:00 2001
> From: Kyotaro Horiguchi
> Date: Thu, 5 Sep 2019 20:21:55 +0900
> Subject: [PATCH v18 1/3] Move callback-call from ReadPageInternal to
> XLogReadRecord.
>
> The current WAL record reader reads page data using a call back
> function. Redesign the interface so that it asks the caller for more
> data when required. This model works better for proposed projects that
> encryption, prefetching and other new features that would require
> extending the callback interface for each case.
>
> As the first step of that change, this patch moves the page reader
> function out of ReadPageInternal(), then the remaining tasks of the
> function are taken over by the new function XLogNeedData().
> -static int
> +static bool
> XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int
> reqLen,
>XLogRecPtr targetRecPtr, char *readBuf)
> {
> @@ -12170,7 +12169,8 @@ retry:
> readLen = 0;
> readSource = XLOG_FROM_ANY;
>
> - return -1;
> + xlogreader->readLen = -1;
> + return false;
> }
> }
It seems a bit weird to assign to XlogReaderState->readLen inside the
callbacks. I first thought it was just a transient state, but it's
not. I think it'd be good to wrap the xlogreader->readLen assignment an
an inline function. That we can add more asserts etc over time.
> -/* pg_waldump's XLogReaderRoutine->page_read callback */
> +/*
> + * pg_waldump's WAL page rader, also used as page_read callback for
> + * XLogFindNextRecord
> + */
> static bool
> -WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
> - XLogRecPtr targetPtr, char *readBuff)
> +WALDumpReadPage(XLogReaderState *state, void *priv)
> {
> - XLogDumpPrivate *private = state->private_data;
> + XLogRecPtr targetPagePtr = state->readPagePtr;
> + int reqLen= state->readLen;
> + char *readBuff = state->readBuf;
> + XLogDumpPrivate *private = (XLogDumpPrivate *) priv;
It seems weird to pass a void *priv to a function that now doesn't at
all need the type punning anymore.
Greetings,
Andres Freund
Re: Remove page-read callback from XLogReaderState.
On Wed, Apr 7, 2021 at 5:09 AM Thomas Munro wrote: > 0001 + 0002 get rid of the callback interface and replace it with a > state machine, making it the client's problem to supply data when it > returns XLREAD_NEED_DATA. I found this interface nicer to work with, > for my WAL decoding buffer patch (CF 2410), and I understand that the > encryption patch set can also benefit from it. Certainly when I > rebased my project on this patch set, I prefered the result. +state->readLen = pageHeaderSize; This variable is used for the XLogPageReader to say how much data it wants, but also for the caller to indicate how much data is loaded. Wouldn't it be better to split this into two variables: bytesWanted and bytesAvailable? (I admit that I spent a whole afternoon debugging after confusing myself about that, when rebasing my WAL readahead patch recently). I wonder if it would be better to have the client code access these values through functions (even if they just access the variables in a static inline function), to create a bit more separation? Something like XLogReaderGetWanted(_lsn, _wanted), and then XLogReaderSetAvailable(state, 42)? Just an idea.
Re: Remove page-read callback from XLogReaderState.
On Wed, Mar 31, 2021 at 7:17 PM Kyotaro Horiguchi
wrote:
> At Wed, 31 Mar 2021 10:00:02 +1300, Thomas Munro
> wrote in
> > On Thu, Mar 4, 2021 at 3:29 PM Kyotaro Horiguchi
> > wrote:
> > > A recent commot about LSN_FORMAT_ARGS conflicted this.
> > > Just rebased.
> >
> > FYI I've been looking at this, and I think it's a very nice
> > improvement. I'll post some review comments and a rebase shortly.
>
> Thanks for looking at this!
I rebased and pgindent-ed the first three patches and did some
testing. I think it looks pretty good, though I still need to check
the code coverage when running the recovery tests. There are three
compiler warnings from GCC when not using --enable-cassert, including
uninitialized variables: pageHeader and targetPagePtr. It looks like
they could be silenced as follows, or maybe you see a better way?
- XLogPageHeader pageHeader;
+ XLogPageHeader pageHeader = NULL;
uint32 pageHeaderSize;
- XLogRecPtr targetPagePtr;
+ XLogRecPtr targetPagePtr =
InvalidXLogRecPtr;
To summarise the patches:
0001 + 0002 get rid of the callback interface and replace it with a
state machine, making it the client's problem to supply data when it
returns XLREAD_NEED_DATA. I found this interface nicer to work with,
for my WAL decoding buffer patch (CF 2410), and I understand that the
encryption patch set can also benefit from it. Certainly when I
rebased my project on this patch set, I prefered the result.
0003 is nice global variable cleanup.
I haven't looked at 0004.
From 560cdfa444a3b05a0e6b8054f3cfeadf56e059fc Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v18 1/3] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Redesign the interface so that it asks the caller for more
data when required. This model works better for proposed projects that
encryption, prefetching and other new features that would require
extending the callback interface for each case.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal(), then the remaining tasks of the
function are taken over by the new function XLogNeedData().
Author: Kyotaro HORIGUCHI
Author: Heikki Linnakangas
Reviewed-by: Antonin Houska
Reviewed-by: Alvaro Herrera
Reviewed-by: Takashi Menjo
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/20190418.210257.43726183.horiguchi.kyotaro%40lab.ntt.co.jp
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 277 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 33 +--
src/include/access/xlogutils.h | 2 +-
8 files changed, 224 insertions(+), 155 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6f8810e149..449e17d2fa 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -920,7 +920,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4375,7 +4375,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -12111,7 +12110,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -12170,7 +12169,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -12265,7 +12265,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
Re: Remove page-read callback from XLogReaderState.
At Wed, 31 Mar 2021 10:00:02 +1300, Thomas Munro wrote in > On Thu, Mar 4, 2021 at 3:29 PM Kyotaro Horiguchi > wrote: > > A recent commot about LSN_FORMAT_ARGS conflicted this. > > Just rebased. > > FYI I've been looking at this, and I think it's a very nice > improvement. I'll post some review comments and a rebase shortly. Thanks for looking at this! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On Thu, Mar 4, 2021 at 3:29 PM Kyotaro Horiguchi wrote: > A recent commot about LSN_FORMAT_ARGS conflicted this. > Just rebased. FYI I've been looking at this, and I think it's a very nice improvement. I'll post some review comments and a rebase shortly.
Re: Remove page-read callback from XLogReaderState.
A recent commot about LSN_FORMAT_ARGS conflicted this.
Just rebased.
regards
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From b23cb65696a42e32b657f12bcf2355acd3100084 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v17 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 281 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 25 ++-
src/include/access/xlogutils.h | 2 +-
8 files changed, 222 insertions(+), 153 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 377afb8732..606328a65a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -917,7 +917,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4350,7 +4350,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11987,7 +11986,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -12046,7 +12045,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -12141,7 +12141,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -12155,8 +12156,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 42738eb940..7d04995b7b 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -276,7 +276,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int readOff;
/*
* randAccess indicates whether to verify the previous-record pointer of
@@ -326,14 +325,20 @@ XLogReadRecord(XLogReaderState *state, 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,
-
Re: Remove page-read callback from XLogReaderState.
Thank you for the comment and sorry for late reply.
At Fri, 17 Jul 2020 14:14:44 +0900, Takashi Menjo
wrote in
> Hi,
>
> I applied your v15 patchset to master
> ed2c7f65bd9f15f8f7cd21ad61602f983b1e72e9. Here are three feedback points
> for you:
>
> = 1. Build error when WAL_DEBUG is defined manually =
..
> Expected: PostgreSQL is successfully made.
> Actual: I got the following make error:
...
> 1219 |debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
Ah, I completely forgot about WAL_DEBUG paths. Fixed.
> = 2. readBuf allocation in XLogReaderAllocate =
> AFAIU, not XLogReaderAllocate() itself but its caller is now responsible
> for allocating XLogReaderState->readBuf. However, the following code still
> remains in src/backend/access/transam/xlogreader.c:
>
>
> 74 XLogReaderState *
> 75 XLogReaderAllocate(int wal_segment_size, const char *waldir,
> 76WALSegmentCleanupCB cleanup_cb)
> 77 {
> :
> 98 state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
> 99 MCXT_ALLOC_NO_OOM);
>
>
> Is this okay?
Oops! That's silly. However, I put a rethink on this. The reason of
the moving of responsibility comes from the fact that the actual
subject that fills-in the buffer is the callers of xlogreader, who
knows its size. In that light it's quite strange that xlogreader
works based on the fixed size of XLOG_BLCKSZ. I don't think it is
useful just now, but I changed 0004 so that XLOG_BLCKSZ is eliminated
from xlogreader.c. Buffer allocation is restored to
XLogReaderAllocate.
(But, I'm not sure it's worth doing..)
> = 3. XLOG_FROM_ANY assigned to global readSource =
> Regarding the following chunk in 0003:
...
> -static XLogSource readSource = XLOG_FROM_ANY;
> +static XLogSource readSource = 0; /* XLOG_FROM_* code */
>
> I think it is better to keep the line "static XLogSource readSource =
> XLOG_FROM_ANY;". XLOG_FROM_ANY is already defined as 0 in
> src/backend/access/transam/xlog.c.
That seems to be a mistake while past rebasding. XLOG_FROM_ANY is the
right thing to use here.
The attached is the rebased, and fixed version.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 84a2f6b1abe2e34b4cdfcb7da7380a2d6161b292 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v16 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 281 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 25 ++-
src/include/access/xlogutils.h | 2 +-
8 files changed, 222 insertions(+), 153 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 09c01ed4ae..a91e86b290 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -909,7 +909,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4329,7 +4329,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11866,7
Re: Remove page-read callback from XLogReaderState.
At Tue, 08 Sep 2020 11:56:29 +0900 (JST), Kyotaro Horiguchi wrote in > Thank you for the comment, Menjo-san, and noticing me of that, Michael. I found why the message I was writing was gone from the draft folder.. Sorry for the garbage. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
Thank you for the comment, Menjo-san, and noticing me of that, Michael.
Sorry for late reply.
At Fri, 17 Jul 2020 14:14:44 +0900, Takashi Menjo
wrote in
> Hi,
>
> I applied your v15 patchset to master
> ed2c7f65bd9f15f8f7cd21ad61602f983b1e72e9. Here are three feedback points
> for you:
>
>
> = 1. Build error when WAL_DEBUG is defined manually =
> How to reproduce:
>
> $ sed -i -E -e 's|^/\* #define WAL_DEBUG \*/$|#define WAL_DEBUG|'
> src/include/pg_config_manual.h
> $ ./configure && make
>
> Expected: PostgreSQL is successfully made.
> Actual: I got the following make error:
>
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
> -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
> -Wno-format-truncation -Wno-stringop-truncation -O2
> -I../../../../src/include -D_GNU_SOURCE -c -o xlog.o xlog.c
> In file included from /usr/include/x86_64-linux-gnu/bits/types/stack_t.h:23,
> from /usr/include/signal.h:303,
> from ../../../../src/include/storage/sinval.h:17,
> from ../../../../src/include/access/xact.h:22,
> from ../../../../src/include/access/twophase.h:17,
> from xlog.c:33:
> xlog.c: In function ‘XLogInsertRecord’:
> xlog.c:1219:56: error: called object is not a function or function pointer
> 1219 |debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
> |^~~~
> xlog.c:1219:19: error: too few arguments to function ‘XLogReaderAllocate’
> 1219 |debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
> | ^~
> In file included from ../../../../src/include/access/clog.h:14,
> from xlog.c:25:
> ../../../../src/include/access/xlogreader.h:243:25: note: declared here
> 243 | extern XLogReaderState *XLogReaderAllocate(int wal_segment_size,
> | ^~
> make[4]: *** [: xlog.o] Error 1
>
>
> The following chunk in 0002 seems to be the cause of the error. There is
> no comma between two NULLs.
>
>
> diff --git a/src/backend/access/transam/xlog.c
> b/src/backend/access/transam/xlog.c
> index e570e56a24..f9b0108602 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> (..snipped..)
> @@ -1225,8 +1218,7 @@ XLogInsertRecord(XLogRecData *rdata,
> appendBinaryStringInfo(, rdata->data, rdata->len);
>
> if (!debug_reader)
> - debug_reader = XLogReaderAllocate(wal_segment_size, NULL,
> - XL_ROUTINE(), NULL);
> + debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
>
> if (!debug_reader)
> {
>
>
>
> = 2. readBuf allocation in XLogReaderAllocate =
> AFAIU, not XLogReaderAllocate() itself but its caller is now responsible
> for allocating XLogReaderState->readBuf. However, the following code still
> remains in src/backend/access/transam/xlogreader.c:
>
>
> 74 XLogReaderState *
> 75 XLogReaderAllocate(int wal_segment_size, const char *waldir,
> 76WALSegmentCleanupCB cleanup_cb)
> 77 {
> :
> 98 state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
> 99 MCXT_ALLOC_NO_OOM);
>
>
> Is this okay?
>
>
> = 3. XLOG_FROM_ANY assigned to global readSource =
> Regarding the following chunk in 0003:
>
>
> diff --git a/src/backend/access/transam/xlog.c
> b/src/backend/access/transam/xlog.c
> index 6b42d9015f..bcb4ef270f 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -804,18 +804,14 @@ static XLogSegNo openLogSegNo = 0;
> * These variables are used similarly to the ones above, but for reading
> * the XLOG. Note, however, that readOff generally represents the offset
> * of the page just read, not the seek position of the FD itself, which
> - * will be just past that page. readLen indicates how much of the current
> - * page has been read into readBuf, and readSource indicates where we got
> - * the currently open file from.
> + * will be just past that page. readSource indicates where we got the
> + * currently open file from.
> * Note: we could use Reserve/ReleaseExternalFD to track consumption of
> * this FD too; but it doesn't currently seem worthwhile, since the XLOG is
> * not read by general-purpose sessions.
> */
> static int readFile = -1;
> -static XLogSegNo readSegNo = 0;
> -static uint32 readOff = 0;
> -static uint32 readLen = 0;
> -static XLogSource readSource = XLOG_FROM_ANY;
> +static XLogSource readSource = 0; /* XLOG_FROM_* code */
>
> /*
> * Keeps track of which source we're
Re: Remove page-read callback from XLogReaderState.
On Fri, Jul 17, 2020 at 02:14:44PM +0900, Takashi Menjo wrote: > I applied your v15 patchset to master > ed2c7f65bd9f15f8f7cd21ad61602f983b1e72e9. Here are three feedback points > for you: And the CF bot complains as well here. Horiguchi-san, this patch is waiting on author for a couple of weeks now. Could you rebase the patch and comment on the points raised upthread? -- Michael signature.asc Description: PGP signature
Re: Remove page-read callback from XLogReaderState.
Hi,
I applied your v15 patchset to master
ed2c7f65bd9f15f8f7cd21ad61602f983b1e72e9. Here are three feedback points
for you:
= 1. Build error when WAL_DEBUG is defined manually =
How to reproduce:
$ sed -i -E -e 's|^/\* #define WAL_DEBUG \*/$|#define WAL_DEBUG|'
src/include/pg_config_manual.h
$ ./configure && make
Expected: PostgreSQL is successfully made.
Actual: I got the following make error:
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -O2
-I../../../../src/include -D_GNU_SOURCE -c -o xlog.o xlog.c
In file included from /usr/include/x86_64-linux-gnu/bits/types/stack_t.h:23,
from /usr/include/signal.h:303,
from ../../../../src/include/storage/sinval.h:17,
from ../../../../src/include/access/xact.h:22,
from ../../../../src/include/access/twophase.h:17,
from xlog.c:33:
xlog.c: In function ‘XLogInsertRecord’:
xlog.c:1219:56: error: called object is not a function or function pointer
1219 |debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
|^~~~
xlog.c:1219:19: error: too few arguments to function ‘XLogReaderAllocate’
1219 |debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
| ^~
In file included from ../../../../src/include/access/clog.h:14,
from xlog.c:25:
../../../../src/include/access/xlogreader.h:243:25: note: declared here
243 | extern XLogReaderState *XLogReaderAllocate(int wal_segment_size,
| ^~
make[4]: *** [: xlog.o] Error 1
The following chunk in 0002 seems to be the cause of the error. There is
no comma between two NULLs.
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index e570e56a24..f9b0108602 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
(..snipped..)
@@ -1225,8 +1218,7 @@ XLogInsertRecord(XLogRecData *rdata,
appendBinaryStringInfo(, rdata->data, rdata->len);
if (!debug_reader)
- debug_reader = XLogReaderAllocate(wal_segment_size, NULL,
- XL_ROUTINE(), NULL);
+ debug_reader = XLogReaderAllocate(wal_segment_size, NULL NULL);
if (!debug_reader)
{
= 2. readBuf allocation in XLogReaderAllocate =
AFAIU, not XLogReaderAllocate() itself but its caller is now responsible
for allocating XLogReaderState->readBuf. However, the following code still
remains in src/backend/access/transam/xlogreader.c:
74 XLogReaderState *
75 XLogReaderAllocate(int wal_segment_size, const char *waldir,
76WALSegmentCleanupCB cleanup_cb)
77 {
:
98 state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
99 MCXT_ALLOC_NO_OOM);
Is this okay?
= 3. XLOG_FROM_ANY assigned to global readSource =
Regarding the following chunk in 0003:
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 6b42d9015f..bcb4ef270f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -804,18 +804,14 @@ static XLogSegNo openLogSegNo = 0;
* These variables are used similarly to the ones above, but for reading
* the XLOG. Note, however, that readOff generally represents the offset
* of the page just read, not the seek position of the FD itself, which
- * will be just past that page. readLen indicates how much of the current
- * page has been read into readBuf, and readSource indicates where we got
- * the currently open file from.
+ * will be just past that page. readSource indicates where we got the
+ * currently open file from.
* Note: we could use Reserve/ReleaseExternalFD to track consumption of
* this FD too; but it doesn't currently seem worthwhile, since the XLOG is
* not read by general-purpose sessions.
*/
static int readFile = -1;
-static XLogSegNo readSegNo = 0;
-static uint32 readOff = 0;
-static uint32 readLen = 0;
-static XLogSource readSource = XLOG_FROM_ANY;
+static XLogSource readSource = 0; /* XLOG_FROM_* code */
/*
* Keeps track of which source we're currently reading from. This is
I think it is better to keep the line "static XLogSource readSource =
XLOG_FROM_ANY;". XLOG_FROM_ANY is already defined as 0 in
src/backend/access/transam/xlog.c.
Regards,
Takashi
2020年7月2日(木) 13:53 Kyotaro Horiguchi :
> cfbot is complaining as this is no longer applicable. Rebased.
>
> In v14, some reference to XLogReaderState parameter to
Re: Remove page-read callback from XLogReaderState.
cfbot is complaining as this is no longer applicable. Rebased.
In v14, some reference to XLogReaderState parameter to read_pages
functions are accidentally replaced by the reference to the global
variable xlogreader. Fixed it, too.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From a4584ab5deddd49325c2f8e3fd78de406c95ce89 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v15 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 281 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 25 ++-
src/include/access/xlogutils.h | 2 +-
8 files changed, 222 insertions(+), 153 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fd93bcfaeb..e570e56a24 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -912,7 +912,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4332,7 +4332,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11860,7 +11859,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11919,7 +11918,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -12014,7 +12014,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -12028,8 +12029,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index cb76be4f46..3d599325ee 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -276,7 +276,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int readOff;
/*
* randAccess indicates whether to verify the previous-record pointer of
@@ -326,14 +325,20 @@ XLogReadRecord(XLogReaderState *state, char
Re: Remove page-read callback from XLogReaderState.
Thank you for the comment. At Tue, 26 May 2020 20:17:47 +0800, Craig Ringer wrote in > On Tue, 26 May 2020, 15:40 Kyotaro Horiguchi, > wrote: > > > > > This patch removes all the three callbacks (open/close/page_read) in > > XL_ROUTINE from XLogReaderState. It only has "cleanup" callback > > instead. > > > > I actually have a use in mind for these callbacks - to support reading WAL > for logical decoding from a restore_command like tool. So we can archive > wal when it's no longer required for recovery and reduce the risk of > filling pg_wal if a standby lags. > > I don't object to your cleanup at all. I'd like it to be properly > pluggable, whereas right now it has hard coded callbacks that differ for > little reason. > > Just noting that the idea of a callback here isn't a bad thing. I agree that plugin is generally not bad as far as it were standalone, that is, as far as it is not tightly cooperative with the opposite side of the caller of it. However, actually it seems to me that the xlogreader plugins are too-deeply coupled with the callers of xlogreader in many aspects involving error-handling and retry-mechanism. As Alvaro mentioned we may have page-decrypt callback shortly as another callback of xlogreader. Xlogreader could be more messy by introducing such plugins, that actually have no business with xlogreader at all. Evidently xlogreader can be a bottom-end module (that is, a module that doesn't depend on another module). It is I think a good thing to isolate xlogreader from the changes of its callers and correlated plugins. A major problem of this patch is that the state machine used there might be another mess here, though. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On Tue, 26 May 2020, 15:40 Kyotaro Horiguchi, wrote: > > This patch removes all the three callbacks (open/close/page_read) in > XL_ROUTINE from XLogReaderState. It only has "cleanup" callback > instead. > I actually have a use in mind for these callbacks - to support reading WAL for logical decoding from a restore_command like tool. So we can archive wal when it's no longer required for recovery and reduce the risk of filling pg_wal if a standby lags. I don't object to your cleanup at all. I'd like it to be properly pluggable, whereas right now it has hard coded callbacks that differ for little reason. Just noting that the idea of a callback here isn't a bad thing. >
Re: Remove page-read callback from XLogReaderState.
At Wed, 22 Apr 2020 10:12:46 +0900 (JST), Kyotaro Horiguchi
wrote in
> cd12323440 conflicts with this. Rebased.
b060dbe000 is conflicting. I gave up isolating XLogOpenSegment from
xlogreader.c, since the two are tightly coupled than I thought.
This patch removes all the three callbacks (open/close/page_read) in
XL_ROUTINE from XLogReaderState. It only has "cleanup" callback
instead.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From fd04ff06de3ab7bcc36f0c80fff42452e49e0259 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v10 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 281 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 25 ++-
src/include/access/xlogutils.h | 2 +-
8 files changed, 222 insertions(+), 153 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ca09d81b08..da468598e4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -909,7 +909,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4329,7 +4329,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11854,7 +11853,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11913,7 +11912,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -12008,7 +12008,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -12022,8 +12023,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 5995798b58..399bad0603 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -274,7 +274,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int
Re: Remove page-read callback from XLogReaderState.
At Tue, 21 Apr 2020 17:04:27 +0900 (JST), Kyotaro Horiguchi
wrote in
>
Mmm. The message body seems disappearing for uncertain reason.
cd12323440 conflicts with this. Rebased.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From b3b780c2143ae70b82fb1e8256e771f11276af31 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v9 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 278 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h | 2 +-
8 files changed, 218 insertions(+), 152 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 11e32733c4..b0ad9376d6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -908,7 +908,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4328,7 +4328,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11810,7 +11809,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11869,7 +11868,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11964,7 +11964,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11978,8 +11979,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 79ff976474..6250093dd9 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -272,7 +272,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int readOff;
/*
* randAccess indicates whether to verify the previous-record pointer of
@@ -322,14 +321,20 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
* byte to cover the whole record header, or at
Re: Remove page-read callback from XLogReaderState.
I found this conficts with a7e8ece41cf7a96d8a9c4c037cdfef304d950831.
Rebased on it.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From e67149578c750977a2a2872d3f4dbb3a86c82a6d Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v8 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 278 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 21 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h | 2 +-
8 files changed, 218 insertions(+), 152 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 11e32733c4..b0ad9376d6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -908,7 +908,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4328,7 +4328,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11810,7 +11809,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11869,7 +11868,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11964,7 +11964,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11978,8 +11979,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 79ff976474..6250093dd9 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -272,7 +272,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int readOff;
/*
* randAccess indicates whether to verify the previous-record pointer of
@@ -322,14 +321,20 @@ XLogReadRecord(XLogReaderState *state, 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,
Re: Remove page-read callback from XLogReaderState.
5d0c2d5eba shot out this.
Rebased.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 6837855f0c938b5e34039897158bc912c6619d2b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v7 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 272 ++--
src/backend/access/transam/xlogutils.c | 12 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h | 2 +-
8 files changed, 211 insertions(+), 148 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7621fc05e2..51c409d00e 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -900,7 +900,7 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath,
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool notfoundOk);
static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, XLogSource source);
-static int XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
+static bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4285,7 +4285,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11660,7 +11659,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11719,7 +11718,8 @@ retry:
readLen = 0;
readSource = XLOG_FROM_ANY;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11814,7 +11814,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11828,8 +11829,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 32f02256ed..2c1500443e 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -269,7 +269,6 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int readOff;
/*
* randAccess indicates whether to verify the previous-record pointer of
@@ -319,14 +318,20 @@ XLogReadRecord(XLogReaderState *state, 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 +
Re: Remove page-read callback from XLogReaderState.
At Tue, 21 Jan 2020 19:45:10 +0900 (JST), Kyotaro Horiguchi
wrote in
> Hello.
>
> At Mon, 20 Jan 2020 17:24:07 +0900 (JST), Kyotaro Horiguchi
> wrote in
> > Separating XLogBeginRead seems reasonable. The annoying recptr trick
> > is no longer needed. In particular some logical-decoding stuff become
> > far cleaner by the patch.
> >
> > fetching_ckpt of ReadRecord is a bit annoying but that coundn't be
> > moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway
> > XLogBeginRead is not the place, of course).
> >
> > As I looked through it, it looks good to me but give me some more time
> > to look closer.
>
> It seems to me that it works perfectly, and everything looks good to
> me except one point.
>
> - * In this case, the passed-in record pointer should already be
> + * In this case, EndRecPtr record pointer should already be
>
> I'm not confident but isn't the "record pointer" redundant? EndRecPtr
> seems containing that meaning, and the other occurances of that kind
> of variable names are not accompanied by that.
I rebased this on the 38a957316d and its follow-on patch 30012a04a6.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 2c605922be988292a80a479b4e59cf799299db19 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v13 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 282 ++
src/backend/access/transam/xlogutils.c| 12 +-
.../replication/logical/logicalfuncs.c| 2 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h| 2 +-
src/include/replication/logicalfuncs.h| 2 +-
10 files changed, 218 insertions(+), 155 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6e09ded597..357432ffa2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -883,7 +883,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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4262,7 +4262,6 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
XLogRecord *record;
XLogPageReadPrivate *private = (XLogPageReadPrivate *) xlogreader->private_data;
- /* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
private->randAccess = (xlogreader->ReadRecPtr == InvalidXLogRecPtr);
@@ -11586,7 +11585,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11645,7 +11644,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11740,7 +11740,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11754,8 +11755,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index
Re: Remove page-read callback from XLogReaderState.
On 21/01/2020 13:33, Craig Ringer wrote: On Tue, 21 Jan 2020 at 18:46, Kyotaro Horiguchi wrote: It seems to me that it works perfectly, and everything looks good I seem to remember some considerable pain in this area when it came to timeline switches. Especially with logical decoding and xlog records that split across a segment boundary. My first attempts at logical decoding timeline following appeared fine and passed tests until they were put under extended real world workloads, at which point they exploded when they tripped corner cases like this. I landed up writing ridiculous regression tests to trigger some of these behaviours. I don't recall how many of them made it into the final patch to core but it's worth a look in the TAP test suite. Yeah, the timeline switching stuff is complicated. The small XLogBeginRead() patch isn't really affected, but it's definitely something to watch out for in the callback API patch. If you happen to have any extra ridiculous tests still lying around, would be nice to look at them. - Heikki
Re: Remove page-read callback from XLogReaderState.
On 21/01/2020 12:45, Kyotaro Horiguchi wrote: At Mon, 20 Jan 2020 17:24:07 +0900 (JST), Kyotaro Horiguchi wrote in Separating XLogBeginRead seems reasonable. The annoying recptr trick is no longer needed. In particular some logical-decoding stuff become far cleaner by the patch. fetching_ckpt of ReadRecord is a bit annoying but that coundn't be moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway XLogBeginRead is not the place, of course). As I looked through it, it looks good to me but give me some more time to look closer. It seems to me that it works perfectly, and everything looks good to me except one point. -* In this case, the passed-in record pointer should already be +* In this case, EndRecPtr record pointer should already be I'm not confident but isn't the "record pointer" redundant? EndRecPtr seems containing that meaning, and the other occurances of that kind of variable names are not accompanied by that. I fixed that, fixed the XLogFindNextRecord() function so that it positions the reader like XLogBeginRead() does (I had already adjusted the comments to say that, but the function didn't actually do it), and pushed. Thanks for the review! I'll continue looking at the callback API changes on Monday. - Heikki
Re: Remove page-read callback from XLogReaderState.
On Tue, 21 Jan 2020 at 18:46, Kyotaro Horiguchi wrote: > > Hello. > > At Mon, 20 Jan 2020 17:24:07 +0900 (JST), Kyotaro Horiguchi > wrote in > > Separating XLogBeginRead seems reasonable. The annoying recptr trick > > is no longer needed. In particular some logical-decoding stuff become > > far cleaner by the patch. > > > > fetching_ckpt of ReadRecord is a bit annoying but that coundn't be > > moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway > > XLogBeginRead is not the place, of course). > > > > As I looked through it, it looks good to me but give me some more time > > to look closer. > > It seems to me that it works perfectly, and everything looks good I seem to remember some considerable pain in this area when it came to timeline switches. Especially with logical decoding and xlog records that split across a segment boundary. My first attempts at logical decoding timeline following appeared fine and passed tests until they were put under extended real world workloads, at which point they exploded when they tripped corner cases like this. I landed up writing ridiculous regression tests to trigger some of these behaviours. I don't recall how many of them made it into the final patch to core but it's worth a look in the TAP test suite. -- Craig Ringer http://www.2ndQuadrant.com/ 2ndQuadrant - PostgreSQL Solutions for the Enterprise
Re: Remove page-read callback from XLogReaderState.
Hello. At Mon, 20 Jan 2020 17:24:07 +0900 (JST), Kyotaro Horiguchi wrote in > Separating XLogBeginRead seems reasonable. The annoying recptr trick > is no longer needed. In particular some logical-decoding stuff become > far cleaner by the patch. > > fetching_ckpt of ReadRecord is a bit annoying but that coundn't be > moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway > XLogBeginRead is not the place, of course). > > As I looked through it, it looks good to me but give me some more time > to look closer. It seems to me that it works perfectly, and everything looks good to me except one point. -* In this case, the passed-in record pointer should already be +* In this case, EndRecPtr record pointer should already be I'm not confident but isn't the "record pointer" redundant? EndRecPtr seems containing that meaning, and the other occurances of that kind of variable names are not accompanied by that. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
Thanks!
At Fri, 17 Jan 2020 20:14:36 +0200, Heikki Linnakangas wrote
in
> On 29/11/2019 10:14, Kyotaro Horiguchi wrote:
> > At Thu, 28 Nov 2019 21:37:03 +0900 (JST), Kyotaro Horiguchi
> > wrote in
> 0dc8ead463 hit this. Rebased.
> >>>
> >>> Please review the pg_waldump.c hunks in 0001; they revert recent
> >>> changes.
> >>
> >> Ughhh! I'l check it. Thank you for noticing!!
> > Fixed that, re-rebased and small comment and cosmetic changes in this
> > version.
>
> Thanks! I finally got around to look at this again. A lot has happened
> since I last looked at this. Notably, commit 0dc8ead463 introduced
> another callback function into the XLogReader interface. It's not
> entirely clear what the big picture with the new callback was and how
> that interacts with the refactoring here. I'll have to spend some time
> to make myself familiar with those changes.
>
> Earlier in this thread, you wrote:
> > - Changed calling convention of XLogReadRecord
> > To make caller loop simple, XLogReadRecord now allows to specify
> > the same valid value while reading the a record. No longer need
> > to change lsn to invalid after the first call in the following
> > reader loop.
> >while (XLogReadRecord(state, lsn, , ) ==
> >XLREAD_NEED_DATA)
> >{
> > if (!page_reader(state))
> >break;
> >}
>
> Actually, I had also made a similar change in the patch version I
> posted at
> https://www.postgresql.org/message-id/4f7a5fad-fa04-b0a3-231b-56d200b646dc%40iki.fi.
> Maybe
> you missed that email? It looks like you didn't include any of the
> changes from that in the patch series. In any case, clearly that idea
> has some merit, since we both independently made a change in that
> calling convention :-).
I'm very sorry but I missed that...
> Actually, I propose that we make that change first, and then continue
> reviewing the rest of these patches. I think it's a more convenient
> interface, independently of the callback refactoring. What do you
> think of the attached patch?
Separating XLogBeginRead seems reasonable. The annoying recptr trick
is no longer needed. In particular some logical-decoding stuff become
far cleaner by the patch.
fetching_ckpt of ReadRecord is a bit annoying but that coundn't be
moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway
XLogBeginRead is not the place, of course).
As I looked through it, it looks good to me but give me some more time
to look closer.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On 2020-Jan-17, Heikki Linnakangas wrote: > I changed that by adding new function, XLogBeginRead(), that repositions the > reader, and removed the 'lsn' argument from XLogReadRecord() altogether. All > callers except one in findLastCheckPoint() pg_rewind.c positioned the reader > once, and then just read sequentially from there, so I think that API is > more convenient. I like it. +1 -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove page-read callback from XLogReaderState.
On 29/11/2019 10:14, Kyotaro Horiguchi wrote:
At Thu, 28 Nov 2019 21:37:03 +0900 (JST), Kyotaro Horiguchi
wrote in
0dc8ead463 hit this. Rebased.
Please review the pg_waldump.c hunks in 0001; they revert recent changes.
Ughhh! I'l check it. Thank you for noticing!!
Fixed that, re-rebased and small comment and cosmetic changes in this
version.
Thanks! I finally got around to look at this again. A lot has happened
since I last looked at this. Notably, commit 0dc8ead463 introduced
another callback function into the XLogReader interface. It's not
entirely clear what the big picture with the new callback was and how
that interacts with the refactoring here. I'll have to spend some time
to make myself familiar with those changes.
Earlier in this thread, you wrote:
- Changed calling convention of XLogReadRecord
To make caller loop simple, XLogReadRecord now allows to specify
the same valid value while reading the a record. No longer need
to change lsn to invalid after the first call in the following
reader loop.
while (XLogReadRecord(state, lsn, , ) == XLREAD_NEED_DATA)
{
if (!page_reader(state))
break;
}
Actually, I had also made a similar change in the patch version I posted
at
https://www.postgresql.org/message-id/4f7a5fad-fa04-b0a3-231b-56d200b646dc%40iki.fi.
Maybe you missed that email? It looks like you didn't include any of the
changes from that in the patch series. In any case, clearly that idea
has some merit, since we both independently made a change in that
calling convention :-).
I changed that by adding new function, XLogBeginRead(), that repositions
the reader, and removed the 'lsn' argument from XLogReadRecord()
altogether. All callers except one in findLastCheckPoint() pg_rewind.c
positioned the reader once, and then just read sequentially from there,
so I think that API is more convenient. With that, the usage looks
something like this:
state = XLogReaderAllocate (...)
XLogBeginRead(state, start_lsn);
while (ctx->reader->EndRecPtr < end_of_wal)
{
while (XLogReadRecord(state, , ) == XLREAD_NEED_DATA)
{
if (!page_reader(state))
break;
}
/* do stuff */
}
Actually, I propose that we make that change first, and then continue
reviewing the rest of these patches. I think it's a more convenient
interface, independently of the callback refactoring. What do you think
of the attached patch?
- Heikki
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6f7ee0c947d..5adf956f413 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1338,7 +1338,8 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
errmsg("out of memory"),
errdetail("Failed while allocating a WAL reading processor.")));
- record = XLogReadRecord(xlogreader, lsn, );
+ XLogBeginRead(xlogreader, lsn);
+ record = XLogReadRecord(xlogreader, );
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 7f4f784c0eb..882d5e8a73f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -897,7 +897,7 @@ static void UpdateLastRemovedPtr(char *filename);
static void ValidateXLOGDirectoryStructure(void);
static void CleanupBackupHistory(void);
static void UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force);
-static XLogRecord *ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr,
+static XLogRecord *ReadRecord(XLogReaderState *xlogreader,
int emode, bool fetching_ckpt);
static void CheckRecoveryConsistency(void);
static XLogRecord *ReadCheckpointRecord(XLogReaderState *xlogreader,
@@ -4246,17 +4246,17 @@ CleanupBackupHistory(void)
}
/*
- * Attempt to read an XLOG record.
+ * Attempt to read the next 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.
+ * Before first call, the reader needs to be positioned to the first record
+ * by calling XLogBeginRead().
*
* If no valid record is available, returns NULL, or fails if emode is PANIC.
* (emode must be either PANIC, LOG). In standby mode, retries until a valid
* record is available.
*/
static XLogRecord *
-ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
+ReadRecord(XLogReaderState *xlogreader, int emode,
bool fetching_ckpt)
{
XLogRecord *record;
@@ -4265,7 +4265,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
/* Pass through parameters to XLogPageRead */
private->fetching_ckpt = fetching_ckpt;
private->emode = emode;
- private->randAccess = (RecPtr != InvalidXLogRecPtr);
+ private->randAccess = (xlogreader->ReadRecPtr != InvalidXLogRecPtr);
/* This is the first attempt to read this page. */
lastSourceFailed = false;
@@ -4274,7 +4274,7 @@
Re: Remove page-read callback from XLogReaderState.
At Thu, 28 Nov 2019 21:37:03 +0900 (JST), Kyotaro Horiguchi
wrote in
> > > 0dc8ead463 hit this. Rebased.
> >
> > Please review the pg_waldump.c hunks in 0001; they revert recent changes.
>
> Ughhh! I'l check it. Thank you for noticing!!
Fixed that, re-rebased and small comment and cosmetic changes in this
version.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 27d9a619bb23cc3175658e34e2fec01eb545b40f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v12 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 272 ++
src/backend/access/transam/xlogutils.c| 12 +-
.../replication/logical/logicalfuncs.c| 2 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h| 2 +-
src/include/replication/logicalfuncs.h| 2 +-
10 files changed, 213 insertions(+), 150 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5f0ee50092..4a6bd6e002 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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
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);
@@ -11537,7 +11536,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11596,7 +11595,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11691,7 +11691,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11705,8 +11706,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 67418b05f1..063223701e 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -245,7 +245,6 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
Re: Remove page-read callback from XLogReaderState.
At Wed, 27 Nov 2019 01:11:40 -0300, Alvaro Herrera wrote in > On 2019-Nov-27, Kyotaro Horiguchi wrote: > > > At Thu, 24 Oct 2019 14:51:01 +0900 (JST), Kyotaro Horiguchi > > wrote in > > > Rebased. > > > > 0dc8ead463 hit this. Rebased. > > Please review the pg_waldump.c hunks in 0001; they revert recent changes. Ughhh! I'l check it. Thank you for noticing!! At Wed, 27 Nov 2019 12:57:47 +0900, Michael Paquier wrote in > Note: Moved to next CF. Thanks. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On 2019-Nov-27, Kyotaro Horiguchi wrote: > At Thu, 24 Oct 2019 14:51:01 +0900 (JST), Kyotaro Horiguchi > wrote in > > Rebased. > > 0dc8ead463 hit this. Rebased. Please review the pg_waldump.c hunks in 0001; they revert recent changes. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove page-read callback from XLogReaderState.
On Wed, Nov 27, 2019 at 12:09:23PM +0900, Kyotaro Horiguchi wrote: > 0dc8ead463 hit this. Rebased. Note: Moved to next CF. -- Michael signature.asc Description: PGP signature
Re: Remove page-read callback from XLogReaderState.
At Thu, 24 Oct 2019 14:51:01 +0900 (JST), Kyotaro Horiguchi
wrote in
> Rebased.
0dc8ead463 hit this. Rebased.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From d9007aee88f7400b0f03ced1b80584964a1b0b79 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v11 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 272 ++
src/backend/access/transam/xlogutils.c| 12 +-
.../replication/logical/logicalfuncs.c| 2 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c | 14 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h| 2 +-
src/include/replication/logicalfuncs.h| 2 +-
10 files changed, 216 insertions(+), 153 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5f0ee50092..4a6bd6e002 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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
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);
@@ -11537,7 +11536,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11596,7 +11595,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11691,7 +11691,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11705,8 +11706,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 67418b05f1..388662ade4 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -36,8 +36,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -245,7 +245,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
@@ -297,14 +296,20 @@
Re: Remove page-read callback from XLogReaderState.
Rebased.
I intentionally left duplicate code in XLogNeedData but changed my
mind to remove it. It makes the function small and clearer.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 7c4ce152d248546c8f56057febae6b17b6fa71bb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v9 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c | 273 ++
src/backend/access/transam/xlogutils.c| 10 +-
.../replication/logical/logicalfuncs.c| 2 +-
src/backend/replication/walsender.c | 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c | 8 +-
src/include/access/xlogreader.h | 23 +-
src/include/access/xlogutils.h| 2 +-
src/include/replication/logicalfuncs.h| 2 +-
src/test/recovery/t/011_crash_recovery.pl | 1 +
11 files changed, 213 insertions(+), 150 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b602e28f61..a7630c643a 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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
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);
@@ -11540,7 +11539,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11599,7 +11598,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11694,7 +11694,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11708,8 +11709,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index c8b0d2303d..66f3bc5597 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -34,8 +34,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
static void XLogReaderInvalReadState(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
@@ -244,7 +244,6 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
uint32 targetRecOff;
uint32 pageHeaderSize;
bool gotheader;
- int readOff;
/*
* randAccess indicates whether to
Re: Remove page-read callback from XLogReaderState.
At Wed, 25 Sep 2019 15:50:32 +0900 (Tokyo Standard Time), Kyotaro Horiguchi
wrote in
<20190925.155032.13779064.horikyota@gmail.com>
> 709d003fbd hit this. Rebased.
Oops! I found a silly silent bug that it doesn't verify the first
page in new segments. Moreover it didin't load the first page in
a new loaded segment.
- Fixed a bug that it didn't load the first segment once new
loaded segment is loaded.
- Fixed a bug that it didn't verify the first segment if it is
not the target page.
Some fishy codes are reaminig but I'll post once the fixed
version.
regares.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From d511d7b9db24450596c82973a380fbae3b2d6ea1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v8 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c| 306 +++--
src/backend/access/transam/xlogutils.c | 10 +-
src/backend/replication/logical/logicalfuncs.c | 2 +-
src/backend/replication/walsender.c| 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c| 8 +-
src/include/access/xlogreader.h| 23 +-
src/include/access/xlogutils.h | 2 +-
src/include/replication/logicalfuncs.h | 2 +-
src/test/recovery/t/011_crash_recovery.pl | 1 +
11 files changed, 239 insertions(+), 157 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6c69eb6dd7..5dcb2e500c 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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
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);
@@ -11522,7 +11521,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11581,7 +11580,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11676,7 +11676,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11690,8 +11691,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 27c27303d6..900a628752 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -34,8 +34,8 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool
Re: Remove page-read callback from XLogReaderState.
Hello.
709d003fbd hit this. Rebased.
Works fine but needs detailed verification and maybe further
cosmetic fixes.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 6be22b10c9df068c53c98ad3106e1bd88e07aeeb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v7 1/4] Move callback-call from ReadPageInternal to
XLogReadRecord.
The current WAL record reader reads page data using a call back
function. Although it is not so problematic alone, it would be a
problem if we are going to do add tasks like encryption which is
performed on page data before WAL reader reads them. To avoid that the
record reader facility has to have a new code path corresponds to
every new callback, this patch separates page reader from WAL record
reading facility by modifying the current WAL record reader to a state
machine.
As the first step of that change, this patch moves the page reader
function out of ReadPageInternal, then the remaining tasks of the
function are taken over by the new function XLogNeedData. As the
result XLogPageRead directly calls the page reader callback function
according to the feedback from XLogNeedData.
---
src/backend/access/transam/xlog.c | 16 +-
src/backend/access/transam/xlogreader.c| 332 +++--
src/backend/access/transam/xlogutils.c | 10 +-
src/backend/replication/logical/logicalfuncs.c | 2 +-
src/backend/replication/walsender.c| 10 +-
src/bin/pg_rewind/parsexlog.c | 16 +-
src/bin/pg_waldump/pg_waldump.c| 8 +-
src/include/access/xlogreader.h| 23 +-
src/include/access/xlogutils.h | 2 +-
src/include/replication/logicalfuncs.h | 2 +-
10 files changed, 259 insertions(+), 162 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6c69eb6dd7..5dcb2e500c 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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
int reqLen, XLogRecPtr targetRecPtr, char *readBuf);
static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
bool fetching_ckpt, XLogRecPtr tliRecPtr);
@@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
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);
@@ -11522,7 +11521,7 @@ CancelBackup(void)
* XLogPageRead() to try fetching the record from another source, or to
* sleep and retry.
*/
-static int
+static bool
XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *readBuf)
{
@@ -11581,7 +11580,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return false;
}
}
@@ -11676,7 +11676,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return true;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11690,8 +11691,9 @@ next_record_is_invalid:
/* In standby-mode, keep trying */
if (StandbyMode)
goto retry;
- else
- return -1;
+
+ xlogreader->readLen = -1;
+ return false;
}
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 27c27303d6..c2bb664f07 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -34,9 +34,9 @@
static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
pg_attribute_printf(2, 3);
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
-static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr,
- int reqLen);
-static void XLogReaderInvalReadState(XLogReaderState *state);
+static bool XLogNeedData(XLogReaderState *state, XLogRecPtr pageptr,
+ int reqLen, bool header_inclusive);
+static void XLogReaderDiscardReadingPage(XLogReaderState *state);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
XLogRecPtr PrevRecPtr, XLogRecord *record, bool randAccess);
static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record,
@@ -104,7 +104,7 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir,
/* system_identifier initialized to zeroes above */
Re: Remove page-read callback from XLogReaderState.
Attached is new version: - Rebased. Cleaned up - Rebased to the current master - Fixed a known bug in the first step patch. It caused timeline-following failure on a standby of a promoted primary. - Fixed confused naming and setting of the parameter includes_paeg_header. - Removed useless XLogNeedData call in XLREAD_NEED_CONTINUATION. The first call to the function ensures that all required data is loaded. Finally, every case block has just one XLogNeedData call. - Removed the label "again" in XLogReadRecord. It is now needed only to repeat XLREAD_NEED_CONTINUATION state. It is naturally writtable as a while loop. - Ensure record == NULL when XLogReadRecord returns other than XLREAD_SUCCESS. Previously the value was not changed in that case and it was not intuitive behavior for callers. - Renamed XLREAD_NEED_* to XLREAD_*. - Removed global variables readOff, readLen, readSegNo. (0003) Other similar variables like readFile/readSource are left alone as they are not common states of page reader and not in XLogReaderState. The attched are the current status, it is separated to two significant parts plus one for readability. v6-0001-Move-callback-call-from-ReadPageInternal-to-XLogR.patch: ReadPageInternal part of the patch. Moves callback calls from ReadPageInternal up to XLogReadRecord. Rerorded commit message and fixed the bug in v5. v6-0002-Move-page-reader-out-of-XLogReadRecord.patch The remaining part of the main work. Eliminates callback calls from XLogReadRecord. Reworded commit message and fixed several bugs. v6-0003-Remove-globals-readSegNo-readOff-readLen.patch Seprate patch to remove some globals that are duplicate with members of XLogReaderState. v6-0004-Change-policy-of-XLog-read-buffer-allocation.patch Separate patch to move page buffer allocation from XLogReaderAllocation from allers of XLogReadRecord. regards. -- Kyotaro Horiguchi NTT Open Source Software Center >From 81d3e58a1f017f34bb654cc4f66a4b9646469349 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Thu, 5 Sep 2019 20:21:55 +0900 Subject: [PATCH v6 1/4] Move callback-call from ReadPageInternal to XLogReadRecord. The current WAL record reader reads page data using a call back function. Although it is not so problematic alone, it would be a problem if we are going to do add tasks like encryption which is performed on page data before WAL reader reads them. To avoid that the record reader facility has to have a new code path corresponds to every new callback, this patch separates page reader from WAL record reading facility by modifying the current WAL record reader to a state machine. As the first step of that change, this patch moves the page reader function out of ReadPageInternal, then the remaining tasks of the function are taken over by the new function XLogNeedData. As the result XLogPageRead directly calls the page reader callback function according to the feedback from XLogNeedData. --- src/backend/access/transam/xlog.c | 16 +- src/backend/access/transam/xlogreader.c| 336 +++-- src/backend/access/transam/xlogutils.c | 10 +- src/backend/replication/logical/logicalfuncs.c | 4 +- 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| 26 +- src/include/access/xlogutils.h | 2 +- src/include/replication/logicalfuncs.h | 2 +- 10 files changed, 265 insertions(+), 166 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 6876537b62..d7f899e738 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 bool XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen, XLogRecPtr targetRecPtr, char *readBuf, TimeLineID *readTLI); static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess, @@ -4249,7 +4249,6 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode, 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); @@ -11521,7 +11520,7 @@ CancelBackup(void) * XLogPageRead() to try fetching the record from another source, or to * sleep and retry. */ -static int +static bool
Re: Remove page-read callback from XLogReaderState.
At Thu, 22 Aug 2019 10:43:52 +0900 (Tokyo Standard Time), Kyotaro Horiguchi
wrote in
<20190822.104352.26342272.horikyota@gmail.com>
> I think you diff is intelligible enough for me. I'll take this if
> you haven't done. Anyway I'm staring on this.
- Reducing state variables
It was a problem for me that there seems to be many state
variables than required. So first I tried to reduce them.
Now readPagePtr and readLen are used bidirectionally.
XLogNeedData sets it as request and page reader set readLen to
the actual length. Similarly verified* changes only when page
header is verified, so I introduced page_verified instead of the
variables.
- Changed calling convention of XLogReadRecord
To make caller loop simple, XLogReadRecord now allows to specify
the same valid value while reading the a record. No longer need
to change lsn to invalid after the first call in the following
reader loop.
while (XLogReadRecord(state, lsn, , ) == XLREAD_NEED_DATA)
{
if (!page_reader(state))
break;
}
- Frequent data request caused by seeing long page header.
XLogNeedData now takes the fourth parameter includes_page_header.
True means the caller is requesting with reqLen that is not
counting page header length. But it makes the function a bit too
complex than expected. Blindly requsting anticipating long page
header for a new page may prevent page-reader from returning the
bytes already at hand by waiting for bytes that won't come. To
prevent such a case the funtion should request anticipating short
page header first for a new page, then make a re-request using
SizeOfLongPHD if needed. Of course it is unlikely to happen for
file sources, and unlikely to harm physical replication (and the
behavior is not changed). Finally, the outcome is more or less
the same with just stashing the seemingly bogus retry from
XLogReadRecord to XLogNeedData. If we are allowed to utilize the
knowlege that long page header is attached to only the first page
of a segment, such complexitly could be eliminated.
- Moving page buffer allocation
As for page buffer allocation, I'm not sure it is meaningful, as
the reader assumes the buffer is in the same with page size,
which is immutable system-wide. It would be surely meanintful if
it were on the caller to decide its own block size, or loading
unit. Anyway it is in the third patch.
- Restored early check-out of record header
The current record reader code seems to be designed to bail-out
by broken record header as earlier as possible, perhaps in order
to prevent impossible size of read in. So I restored the
behavior.
The attched are the current status, it is separated to two
significant parts plus one for readability.
v5-0001-Move-callback-call-from-ReadPageInternal-to-XLogR.patch:
ReadPageInternal part of the patch. Moves callback calls from
ReadPageInternal up to XLogReadRecord. Some of recovery tests
fail applyin only this one but I don't want to put more efforts
to make this state perfecgt.
v5-0002-Move-page-reader-out-of-XLogReadRecord.patch
The remaining part of the main work. Eliminates callback calls
from XLogReadRecord. Applies to current master. Passes all
regression and TAP tests.
v5-0003-Change-policy-of-XLog-read-buffer-allocation.patch
Separate patch to move page buffer allocation from
XLogReaderAllocation from allers of XLogReadRecord.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 067c3bbb9105c391b7c19cc9602a5df6db5fb434 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 5 Sep 2019 20:21:55 +0900
Subject: [PATCH v5 1/3] Move callback-call from ReadPageInternal to
XLogReadRecord.
WAL reader facility used to call given page-reader callback function
in the ReadPageInternal, which is two levels below from
ReadRecord. That makes things a bit complex, but furthermore we are
going to have additional callbacks for encryption, compression or
something like that works before or after reading pages. Just adding
them as new callback makes things messier. If the caller of the
current ReadRecord could call page fetching function directly, things
would get quite easier.
As the first step of that change, this patch moves the place where
that callbacks are called by 1 level above
ReadPageInternal. XLogPageRead uses a loop over new function
XLogNeedData and the callback directly instead of ReadPageInternal.
---
src/backend/access/transam/xlog.c | 15 +-
src/backend/access/transam/xlogreader.c| 355 -
src/backend/access/transam/xlogutils.c | 10 +-
src/backend/replication/logical/logicalfuncs.c | 4 +-
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| 30 ++-
src/include/access/xlogutils.h | 2 +-
src/include/replication/logicalfuncs.h | 2 +-
Re: Remove page-read callback from XLogReaderState.
On 22/08/2019 04:43, Kyotaro Horiguchi wrote:
At Mon, 29 Jul 2019 22:39:57 +0300, Heikki Linnakangas wrote in
On 12/07/2019 10:10, Kyotaro Horiguchi wrote:
* XLogReaderState->readBuf is now allocated and controlled by the
* caller, not by xlogreader.c itself. When XLogReadRecord() needs data,
* the caller makes the data available in readBuf, which can point to the
* same buffer in all calls, or the caller may allocate a new buffer, or
* it may point to a part of a larger buffer, whatever is convenient for
* the caller. (Currently, all callers just allocate a BLCKSZ'd buffer,
* though). The caller also sets readPagPtr, readLen and readPageTLI to
* tell XLogReadRecord() what's in the buffer. So all these read* fields
* are now set by the caller, XLogReadRecord() only reads them.
The caller knows how many byes to be read. So the caller provides
the required buffer seems reasonable.
I also had in mind that the caller could provide a larger buffer,
spanning multiple pages, in one XLogReadRecord() call. It might be
convenient to load a whole WAL file in memory and pass it to
XLogReadRecord() in one call, for example. I think the interface would
now allow that, but the code won't actually take advantage of that.
XLogReadRecord() will always ask for one page at a time, and I think it
will ask the caller for more data between each page, even if the caller
supplies more than one page in one call.
I'm not sure how intelligible this patch is in its current state. But
I think the general idea is good. I plan to clean it up further next
week, but feel free to work on it before that, either based on this
patch or by starting afresh from your patch set.
I think you diff is intelligible enough for me. I'll take this if
you haven't done. Anyway I'm staring on this.
Thanks! I did actually spend some time on this last week, but got
distracted by something else before finishing it up and posting a patch.
Here's a snapshot of what I have in my local branch. It seems to pass
"make check-world", but probably needs some more cleanup.
Main changes since last version:
* I changed the interface so that there is a new function to set the
starting position, XLogBeginRead(), and XLogReadRecord() always
continues from where it left off. I think that's more clear than having
a starting point argument in XLogReadRecord(), which was only set on the
first call. It makes the calling code more clear, too, IMO.
* Refactored the implementation of XLogFindNextRecord().
XLogFindNextRecord() is now a sibling function of XLogBeginRead(). It
sets the starting point like XLogBeginRead(). The difference is that
with XLogFindNextRecord(), the starting point doesn't need to point to a
valid record, it will "fast forward" to the next valid record after the
point. The "fast forwarding" is done in an extra state in the state
machine in XLogReadRecord().
* I refactored XLogReadRecord() and the internal XLogNeedData()
function. XLogNeedData() used to contain logic for verifying segment and
page headers. That works quite differently now. Checking the segment
header is now a new state in the state machine, and the page header is
verified at the top of XLogReadRecord(), whenever the caller provides
new data. I think that makes the code in XLogReadRecord() more clear.
- Heikki
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 477709bbc23..8ecb5ea55c5 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1386,15 +1386,21 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
XLogReaderState *xlogreader;
char *errormsg;
- xlogreader = XLogReaderAllocate(wal_segment_size, _local_xlog_page,
- NULL);
+ xlogreader = XLogReaderAllocate(wal_segment_size);
if (!xlogreader)
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory"),
errdetail("Failed while allocating a WAL reading processor.")));
+ xlogreader->readBuf = palloc(XLOG_BLCKSZ);
+
+ XLogBeginRead(xlogreader, lsn);
+ while (XLogReadRecord(xlogreader, , ) ==
+ XLREAD_NEED_DATA)
+ {
+ read_local_xlog_page(xlogreader);
+ }
- record = XLogReadRecord(xlogreader, lsn, );
if (record == NULL)
ereport(ERROR,
(errcode_for_file_access(),
@@ -1416,6 +1422,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
*buf = palloc(sizeof(char) * XLogRecGetDataLen(xlogreader));
memcpy(*buf, XLogRecGetData(xlogreader), sizeof(char) * XLogRecGetDataLen(xlogreader));
+ pfree(xlogreader->readBuf);
XLogReaderFree(xlogreader);
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e651a841bbe..1bb303a90dc 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;
Re: Remove page-read callback from XLogReaderState.
Thank you for the suggestion, Heikki. At Mon, 29 Jul 2019 22:39:57 +0300, Heikki Linnakangas wrote in > On 12/07/2019 10:10, Kyotaro Horiguchi wrote: > >> Just FYI, to me this doesn't clearly enough look like an improvement, > >> for a change of this size. > > Thanks for the opiniton. I kinda agree about size but it is a > > decision between "having multiple callbacks called under the > > hood" vs "just calling a series of functions". I think the > > patched XlogReadRecord is easy to use in many situations. > > It would be better if I could completely refactor the function > > without the syntax tricks but I think the current patch is still > > smaller and clearer than overhauling it. > > I like the idea of refactoring XLogReadRecord() to not use a callback, > and return a XLREAD_NEED_DATA value instead. It feels like a nicer, > easier-to-use, interface, given that all the page-read functions need > quite a bit of state and internal logic themselves. I remember that I > felt that that would be a nicer interface when we originally extracted > xlogreader.c into a reusable module, but I didn't want to make such > big changes to XLogReadRecord() at that point. > > I don't much like the "continuation" style of implementing the state > machine. Nothing wrong with such a style in principle, but we don't do > that anywhere else, and the macros seem like overkill, and turning the Agreed that it's a kind of ugly. I could overhaul the logic to reduce state variables, but I thought that it would make the patch hardly reviewable. The "continuation" style was intended to impact the main path's shape as small as possible. For the same reason I made variables static instead of using individual state struct or reducing state variables. (And it the style was fun for me:p) > local variables static is pretty ugly. But I think XLogReadRecord() > could be rewritten into a more traditional state machine. > > I started hacking on that, to get an idea of what it would look like > and came up with the attached patch, to be applied on top of all your > patches. It's still very messy, it needs quite a lot of cleanup before > it can be committed, but I think the resulting switch-case state > machine in XLogReadRecord() is quite straightforward at high level, > with four states. Sorry for late reply. It seems less messy than I thought it could be if I refactored it more aggressively. > I made some further changes to the XLogReadRecord() interface: > > * If you pass a valid ReadPtr (i.e. the starting point to read from) > * argument to XLogReadRecord(), it always restarts reading from that > * record, even if it was in the middle of reading another record > * previously. (Perhaps it would be more convenient to provide a separate > * function to set the starting point, and remove the RecPtr argument > * from XLogReadRecord altogther?) Seems reasonable. randAccess property was replaced with the state.PrevRecPtr = Invalid. It is easier to understand for me. > * XLogReaderState->readBuf is now allocated and controlled by the > * caller, not by xlogreader.c itself. When XLogReadRecord() needs data, > * the caller makes the data available in readBuf, which can point to the > * same buffer in all calls, or the caller may allocate a new buffer, or > * it may point to a part of a larger buffer, whatever is convenient for > * the caller. (Currently, all callers just allocate a BLCKSZ'd buffer, > * though). The caller also sets readPagPtr, readLen and readPageTLI to > * tell XLogReadRecord() what's in the buffer. So all these read* fields > * are now set by the caller, XLogReadRecord() only reads them. The caller knows how many byes to be read. So the caller provides the required buffer seems reasonable. > * In your patch, if XLogReadRecord() was called with state->readLen == > * -1, XLogReadRecord() returned an error. That seemed a bit silly; if an > * error happened while reading the data, why call XLogReadRecord() at > * all? You could just report the error directly. So I removed that. Agreed. I forgot to move the error handling to more proper location. > I'm not sure how intelligible this patch is in its current state. But > I think the general idea is good. I plan to clean it up further next > week, but feel free to work on it before that, either based on this > patch or by starting afresh from your patch set. I think you diff is intelligible enough for me. I'll take this if you haven't done. Anyway I'm staring on this. Thanks! -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
On 12/07/2019 10:10, Kyotaro Horiguchi wrote:
At Tue, 28 May 2019 04:45:24 -0700, Andres Freund wrote in
<20190528114524.dvj6ymap2virl...@alap3.anarazel.de>
Hi,
On 2019-04-18 21:02:57 +0900, Kyotaro HORIGUCHI wrote:
Hello. As mentioned before [1], read_page callback in
XLogReaderState is a cause of headaches. Adding another
remote-controlling stuff to xlog readers makes things messier [2].
I refactored XLOG reading functions so that we don't need the
callback. In short, ReadRecrod now calls XLogPageRead directly
with the attached patch set.
| while (XLogReadRecord(xlogreader, RecPtr, , )
|== XLREAD_NEED_DATA)
| XLogPageRead(xlogreader, fetching_ckpt, emode, randAccess);
On the other hand, XLogReadRecord became a bit complex. The patch
makes XLogReadRecord a state machine. I'm not confident that the
additional complexity is worth doing. Anyway I'll gegister this
to the next CF.
Just FYI, to me this doesn't clearly enough look like an improvement,
for a change of this size.
Thanks for the opiniton. I kinda agree about size but it is a
decision between "having multiple callbacks called under the
hood" vs "just calling a series of functions". I think the
patched XlogReadRecord is easy to use in many situations.
It would be better if I could completely refactor the function
without the syntax tricks but I think the current patch is still
smaller and clearer than overhauling it.
I like the idea of refactoring XLogReadRecord() to not use a callback,
and return a XLREAD_NEED_DATA value instead. It feels like a nicer,
easier-to-use, interface, given that all the page-read functions need
quite a bit of state and internal logic themselves. I remember that I
felt that that would be a nicer interface when we originally extracted
xlogreader.c into a reusable module, but I didn't want to make such big
changes to XLogReadRecord() at that point.
I don't much like the "continuation" style of implementing the state
machine. Nothing wrong with such a style in principle, but we don't do
that anywhere else, and the macros seem like overkill, and turning the
local variables static is pretty ugly. But I think XLogReadRecord()
could be rewritten into a more traditional state machine.
I started hacking on that, to get an idea of what it would look like and
came up with the attached patch, to be applied on top of all your
patches. It's still very messy, it needs quite a lot of cleanup before
it can be committed, but I think the resulting switch-case state machine
in XLogReadRecord() is quite straightforward at high level, with four
states.
I made some further changes to the XLogReadRecord() interface:
* If you pass a valid ReadPtr (i.e. the starting point to read from)
argument to XLogReadRecord(), it always restarts reading from that
record, even if it was in the middle of reading another record
previously. (Perhaps it would be more convenient to provide a separate
function to set the starting point, and remove the RecPtr argument from
XLogReadRecord altogther?)
* XLogReaderState->readBuf is now allocated and controlled by the
caller, not by xlogreader.c itself. When XLogReadRecord() needs data,
the caller makes the data available in readBuf, which can point to the
same buffer in all calls, or the caller may allocate a new buffer, or it
may point to a part of a larger buffer, whatever is convenient for the
caller. (Currently, all callers just allocate a BLCKSZ'd buffer,
though). The caller also sets readPagPtr, readLen and readPageTLI to
tell XLogReadRecord() what's in the buffer. So all these read* fields
are now set by the caller, XLogReadRecord() only reads them.
* In your patch, if XLogReadRecord() was called with state->readLen ==
-1, XLogReadRecord() returned an error. That seemed a bit silly; if an
error happened while reading the data, why call XLogReadRecord() at all?
You could just report the error directly. So I removed that.
I'm not sure how intelligible this patch is in its current state. But I
think the general idea is good. I plan to clean it up further next week,
but feel free to work on it before that, either based on this patch or
by starting afresh from your patch set.
- Heikki
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index e7a086b71e8..8f07450f503 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1385,6 +1385,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
XLogRecord *record;
XLogReaderState *xlogreader;
char *errormsg;
+ XLogRecPtr ptr;
xlogreader = XLogReaderAllocate(wal_segment_size);
if (!xlogreader)
@@ -1392,10 +1393,15 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory"),
errdetail("Failed while allocating a WAL reading processor.")));
+ xlogreader->readBuf = palloc(XLOG_BLCKSZ);
- while
Re: Remove page-read callback from XLogReaderState.
At Tue, 28 May 2019 04:45:24 -0700, Andres Freund wrote in <20190528114524.dvj6ymap2virl...@alap3.anarazel.de> > Hi, > > On 2019-04-18 21:02:57 +0900, Kyotaro HORIGUCHI wrote: > > Hello. As mentioned before [1], read_page callback in > > XLogReaderState is a cause of headaches. Adding another > > remote-controlling stuff to xlog readers makes things messier [2]. > > > > I refactored XLOG reading functions so that we don't need the > > callback. In short, ReadRecrod now calls XLogPageRead directly > > with the attached patch set. > > > > | while (XLogReadRecord(xlogreader, RecPtr, , ) > > |== XLREAD_NEED_DATA) > > | XLogPageRead(xlogreader, fetching_ckpt, emode, randAccess); > > > > On the other hand, XLogReadRecord became a bit complex. The patch > > makes XLogReadRecord a state machine. I'm not confident that the > > additional complexity is worth doing. Anyway I'll gegister this > > to the next CF. > > Just FYI, to me this doesn't clearly enough look like an improvement, > for a change of this size. Thanks for the opiniton. I kinda agree about size but it is a decision between "having multiple callbacks called under the hood" vs "just calling a series of functions". I think the patched XlogReadRecord is easy to use in many situations. It would be better if I could completely refactor the function without the syntax tricks but I think the current patch is still smaller and clearer than overhauling it. If many of the folks think that adding a callback is better than this refactoring, I will withdraw this.. reagrds. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove page-read callback from XLogReaderState.
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
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)
{
;
}
---
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 = &\
+ static void *__xlr_state = & \
+ 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 = (&); \
+ 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
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.
---
Re: Remove page-read callback from XLogReaderState.
Hi, On 2019-04-18 21:02:57 +0900, Kyotaro HORIGUCHI wrote: > Hello. As mentioned before [1], read_page callback in > XLogReaderState is a cause of headaches. Adding another > remote-controlling stuff to xlog readers makes things messier [2]. > > I refactored XLOG reading functions so that we don't need the > callback. In short, ReadRecrod now calls XLogPageRead directly > with the attached patch set. > > | while (XLogReadRecord(xlogreader, RecPtr, , ) > |== XLREAD_NEED_DATA) > | XLogPageRead(xlogreader, fetching_ckpt, emode, randAccess); > > On the other hand, XLogReadRecord became a bit complex. The patch > makes XLogReadRecord a state machine. I'm not confident that the > additional complexity is worth doing. Anyway I'll gegister this > to the next CF. Just FYI, to me this doesn't clearly enough look like an improvement, for a change of this size. Greetings, Andres Freund
Re: Remove page-read callback from XLogReaderState.
Kyotaro HORIGUCHI wrote: > v3-0001 : Changed macrosas suggested. This attachment is missing, please send it too. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: Remove page-read callback from XLogReaderState.
Thank you for looking this, Antonin.
At Wed, 22 May 2019 13:53:23 +0200, Antonin Houska wrote in
<25494.1558526...@spoje.net>
> Kyotaro HORIGUCHI wrote:
>
> > Hello. Thank you for looking this.
> > ...
> > Yeah, I'll register this, maybe the week after next week.
>
> I've checked the new version. One more thing I noticed now is that XLR_STATE.j
> is initialized to zero, either by XLogReaderAllocate() which zeroes the whole
> reader state, or later by XLREAD_RESET. This special value then needs to be
> handled here:
>
> #define XLR_SWITCH() \
> do {\
> if ((XLR_STATE).j) \
> goto *((void *) (XLR_STATE).j); \
> XLR_CASE(XLR_INIT_STATE); \
> } while (0)
>
> I think it's better to set the label always to (&_INIT_STATE) so that
> XLR_SWITCH can perform the jump unconditionally.
I thought the same but did not do that since label is
function-scoped so it cannot be referred outside the defined
function.
I moved the state variable from XLogReaderState into functions
static variable. It's not problem since the functions are
non-reentrant in the first place.
> Attached is also an (unrelated) comment fix proposal.
Sounds reasoable. I found another typo "acutually" there.
-int32readLen;/* bytes acutually read, must be larger than
+int32readLen;/* bytes acutually read, must be at least
I fixed it with other typos found.
v3-0001 : Changed macrosas suggested.
v3-0002, 0004: Fixed comments. Fixes following changes of
macros. Renamed state symbols.
v3-0003, 0005-0010: No substantial change from v2.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 036b1a50ad79ebfde990e178bead9ba03c753d02 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
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 1c7dd51b9f..d1a29fe5cd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -885,7 +885,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,
@@ -11507,7 +11507,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)
{
@@ -11566,7 +11566,8 @@ retry:
readLen = 0;
readSource = 0;
- return -1;
+ xlogreader->readLen = -1;
+ return;
}
}
@@ -11661,7 +11662,8 @@ retry:
goto next_record_is_invalid;
}
- return readLen;
+ xlogreader->readLen = readLen;
+ return;
next_record_is_invalid:
lastSourceFailed = true;
@@ -11675,8 +11677,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 baf04b9f1f..f7499aff5e 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
Re: Remove page-read callback from XLogReaderState.
Kyotaro HORIGUCHI wrote:
> Hello. Thank you for looking this.
> ...
> Yeah, I'll register this, maybe the week after next week.
I've checked the new version. One more thing I noticed now is that XLR_STATE.j
is initialized to zero, either by XLogReaderAllocate() which zeroes the whole
reader state, or later by XLREAD_RESET. This special value then needs to be
handled here:
#define XLR_SWITCH()\
do {\
if ((XLR_STATE).j) \
goto *((void *) (XLR_STATE).j); \
XLR_CASE(XLR_INIT_STATE); \
} while (0)
I think it's better to set the label always to (&_INIT_STATE) so that
XLR_SWITCH can perform the jump unconditionally.
Attached is also an (unrelated) comment fix proposal.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 262bf0e77f..cb864a86d8 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -124,7 +124,7 @@ struct XLogReaderState
XLogRecPtr currRecPtr; /* beginning of the WAL record being read */
/* return from page reader */
- int32 readLen; /* bytes acutually read, must be larger than
+ int32 readLen; /* bytes acutually read, must be at least
* loadLen. -1 on error. */
TimeLineID readPageTLI; /* TLI for data currently in readBuf */
Re: Remove page-read callback from XLogReaderState.
Hello. Thank you for looking this.
At Thu, 25 Apr 2019 13:58:20 +0200, Antonin Houska wrote in
<18581.1556193500@localhost>
> Kyotaro HORIGUCHI wrote:
>
> > Hello. As mentioned before [1], read_page callback in
> > XLogReaderState is a cause of headaches. Adding another
> > remote-controlling stuff to xlog readers makes things messier [2].
>
> The patch I posted in thread [2] tries to solve another problem: it tries to
> merge xlogutils.c:XLogRead(), walsender.c:XLogRead() and
> pg_waldump.c:XLogDumpXLogRead() into a single function,
> xlogutils.c:XLogRead().
>
> > [2]
> > https://www.postgresql.org/message-id/20190412.122711.158276916.horiguchi.kyot...@lab.ntt.co.jp
>
> > I refactored XLOG reading functions so that we don't need the
> > callback.
>
> I was curious about the patch, so I reviewed it:
Thnak for the comment. (It's a shame that I might made it more complex..)
> * xlogreader.c
>
> ** Comments mention "opcode", "op" and "expression step" - probably leftover
> from the executor, which seems to have inspired you.
Uggh. Yes, exactly. I believed change them all. Fixed.
> ** XLR_DISPATCH() seems to be unused
Right. XLR_ macros are used to dispatch internally in a function
differently from EEO_ macros so I thought it uesless but I
hesitated to remove it. I remove it.
> ** Comment: "duplicatedly" -> "repeatedly" ?
It aimed reentrance. But I notieced that it doesn't work when
ERROR-exiting. So I remove the assertion and related code..
> ** XLogReadRecord(): comment "All internal state need ..." -> "needs"
Fixed.
> ** XLogNeedData()
>
> *** shouldn't only the minimum amount of data needed (SizeOfXLogLongPHD)
> be requested here?
>
> state->loadLen = XLOG_BLCKSZ;
> XLR_LEAVE(XLND_STATE_SEGHEAD, true);
>
> Note that ->loadLen is also set only to the minimum amount of data needed
> elsewhere.
Maybe right, but it is existing behavior so I preserved it as
focusing on refactoring.
> *** you still mention "read_page callback" in a comment.
Thanks. "the read_page callback" were translated to "the caller"
and it seems the last one.
> *** state->readLen is checked before one of the calls of XLR_LEAVE(), but
> I think it should happen before *each* call. Otherwise data can be read
> from the page even if it's already in the buffer.
That doesn't happen since XLogReadRecord doesn't LEAVE unless
XLogNeedData returns true (that is, needs more data) and
XLogNeedData returns true only when requested data is not on the
buffer yet. (If I refactored it correctly and it seems to me so.)
> * xlogreader.h
>
> ** XLND_STATE_PAGEFULLHEAD - maybe LONG rather than FULL? And perhaps HEAD
> -> HDR, so it's clear that it's about (page) header, not e.g. list head.
Perhaps that's better. Thanks.
>
> ** XLogReaderState.loadLen - why not reqLen? loadLen sounds to me like
> "loaded"
> as opposed to "requested". And assignemnt like this
>
> int reqLen = xlogreader->loadLen;
>
> will also be less confusing with ->reqLen.
>
> Maybe also ->loadPagePtr should be renamed to ->targetPagePtr.
Yeah, that's annoyance. reqLen *was* actually the "requested"
length to XLogNeedData FKA ReadPageInternal, but in the current
shape XLogNeedData makes different request to the callers (when
fetching the first page in the newly visited segment), so the two
(req(uest)Len and (to be)loadLen) are different things. At the
same time, targetPagePoint is different from loadPagePtr.
Of course the naming as arguable.
> * trailing whitespace: xlogreader.h:130, xlogreader.c:1058
Thanks, it have been fixed on my repo.
> * The 2nd argument of SimpleXLogPageRead() is "private", which seems too
> generic given that the function is no longer used as a callback. Since the
> XLogPageReadPrivate structure only has two fields, I think it'd be o.k. to
> pass them to the function directly.
Sound reasonable. Fixed.
> * I haven't found CF entry for this patch.
Yeah, I'll register this, maybe the week after next week.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From cff0f38cc03c2186505cad30b27115bc0228b42f Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 18 Apr 2019 10:22:49 +0900
Subject: [PATCH 01/10] Define macros to make XLogReadRecord a state machine
To minimize apparent imapct 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();
...
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)
{
;
}
---
src/backend/access/transam/xlogreader.c | 53
Re: Remove page-read callback from XLogReaderState.
Kyotaro HORIGUCHI wrote: > Hello. As mentioned before [1], read_page callback in > XLogReaderState is a cause of headaches. Adding another > remote-controlling stuff to xlog readers makes things messier [2]. The patch I posted in thread [2] tries to solve another problem: it tries to merge xlogutils.c:XLogRead(), walsender.c:XLogRead() and pg_waldump.c:XLogDumpXLogRead() into a single function, xlogutils.c:XLogRead(). > [2] > https://www.postgresql.org/message-id/20190412.122711.158276916.horiguchi.kyot...@lab.ntt.co.jp > I refactored XLOG reading functions so that we don't need the > callback. I was curious about the patch, so I reviewed it: * xlogreader.c ** Comments mention "opcode", "op" and "expression step" - probably leftover from the executor, which seems to have inspired you. ** XLR_DISPATCH() seems to be unused ** Comment: "duplicatedly" -> "repeatedly" ? ** XLogReadRecord(): comment "All internal state need ..." -> "needs" ** XLogNeedData() *** shouldn't only the minimum amount of data needed (SizeOfXLogLongPHD) be requested here? state->loadLen = XLOG_BLCKSZ; XLR_LEAVE(XLND_STATE_SEGHEAD, true); Note that ->loadLen is also set only to the minimum amount of data needed elsewhere. *** you still mention "read_page callback" in a comment. *** state->readLen is checked before one of the calls of XLR_LEAVE(), but I think it should happen before *each* call. Otherwise data can be read from the page even if it's already in the buffer. * xlogreader.h ** XLND_STATE_PAGEFULLHEAD - maybe LONG rather than FULL? And perhaps HEAD -> HDR, so it's clear that it's about (page) header, not e.g. list head. ** XLogReaderState.loadLen - why not reqLen? loadLen sounds to me like "loaded" as opposed to "requested". And assignemnt like this int reqLen = xlogreader->loadLen; will also be less confusing with ->reqLen. Maybe also ->loadPagePtr should be renamed to ->targetPagePtr. * trailing whitespace: xlogreader.h:130, xlogreader.c:1058 * The 2nd argument of SimpleXLogPageRead() is "private", which seems too generic given that the function is no longer used as a callback. Since the XLogPageReadPrivate structure only has two fields, I think it'd be o.k. to pass them to the function directly. * I haven't found CF entry for this patch. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Remove page-read callback from XLogReaderState.
Hello. As mentioned before [1], read_page callback in
XLogReaderState is a cause of headaches. Adding another
remote-controlling stuff to xlog readers makes things messier [2].
I refactored XLOG reading functions so that we don't need the
callback. In short, ReadRecrod now calls XLogPageRead directly
with the attached patch set.
| while (XLogReadRecord(xlogreader, RecPtr, , )
|== XLREAD_NEED_DATA)
| XLogPageRead(xlogreader, fetching_ckpt, emode, randAccess);
On the other hand, XLogReadRecord became a bit complex. The patch
makes XLogReadRecord a state machine. I'm not confident that the
additional complexity is worth doing. Anyway I'll gegister this
to the next CF.
[1]
https://www.postgresql.org/message-id/47215279-228d-f30d-35d1-16af695e5...@iki.fi
[2]
https://www.postgresql.org/message-id/20190412.122711.158276916.horiguchi.kyot...@lab.ntt.co.jp
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 3de14fb47987e9dd8189bfad3ca7264c26b719eb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 18 Apr 2019 10:22:49 +0900
Subject: [PATCH 01/10] Define macros to make XLogReadRecord a state machine
To minimize apparent imapct 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();
...
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)
{
;
}
---
src/backend/access/transam/xlogreader.c | 63 +
src/include/access/xlogreader.h | 3 ++
2 files changed, 66 insertions(+)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 9196aa3aae..5299765040 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -29,6 +29,69 @@
#include "utils/memutils.h"
#endif
+/*
+ * Use computed-goto-based opcode 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 */
+
+/*
+ * Macros for opcode dispatch.
+ *
+ * XLR_SWITCH - just hides the switch if not in use.
+ * XLR_CASE - labels the implementation of named expression step type.
+ * XLR_DISPATCH - jump to the implementation of the step type for 'op'.
+ * 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() \
+ /* Don't call duplicatedly */ \
+ static int callcnt = 0 PG_USED_FOR_ASSERTS_ONLY; \
+ do { \
+ if ((XLR_STATE).j) \
+ goto *((void *) (XLR_STATE).j); \
+ XLR_CASE(XLR_INIT_STATE); \
+ Assert(++callcnt == 1);\
+ } while (0)
+#define XLR_CASE(name) name:
+#define XLR_DISPATCH() goto *((void *) (XLR_STATE).j)
+#define XLR_LEAVE(name, code) do {\
+ (XLR_STATE).j = (&); return (code); \
+ XLR_CASE(name);\
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ Assert(--callcnt == 0);\
+ (XLR_STATE).j = (&_INIT_STATE); return (code); \
+ } while (0)
+#define XLR_SWITCH_END()
+#else /* !XLR_USE_COMPUTED_GOTO */
+#define XLR_SWITCH() \
+ /* Don't call duplicatedly */ \
+ static int callcnt = 0 PG_USED_FOR_ASSERTS_ONLY; \
+ switch ((XLR_STATE).c) { \
+ XLR_CASE(XLR_INIT_STATE); \
+ Assert(++callcnt == 1); \
+#define XLR_CASE(name) case name:
+#define XLR_DISPATCH() goto starteval
+#define XLR_LEAVE(name, code) \
+ do { \
+ (XLR_STATE).c = (name); return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ Assert(--callcnt == 0);\
+ (XLR_STATE).c = (XLR_INIT_STATE); 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,
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index f3bae0bf49..30500c35c7 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -240,6 +240,9 @@ extern bool DecodeXLogRecord(XLogReaderState *state, XLogRecord *record,
#define XLogRecBlockImageApply(decoder, block_id) \
((decoder)->blocks[block_id].apply_image)
+/* Reset the
