Remove page-read callback from XLogReaderState.

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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]
/messages/by-id/20190412.122711.158276916.horiguchi.kyotaro@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

Hello. Thank you for looking this.

At Thu, 25 Apr 2019 13:58:20 +0200, Antonin Houska <ah@cybertec.at> wrote in <18581.1556193500@localhost>

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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]
/messages/by-id/20190412.122711.158276916.horiguchi.kyotaro@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

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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 (&&XLR_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

Thank you for looking this, Antonin.

At Wed, 22 May 2019 13:53:23 +0200, Antonin Houska <ah@cybertec.at> wrote in <25494.1558526003@spoje.net>

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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 (&&XLR_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.

-    int32        readLen;        /* bytes acutually read, must be larger than
+    int32        readLen;        /* 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

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

v3-0001 : Changed macrosas suggested.

This attachment is missing, please send it too.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

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, &record, &errormsg)
| == 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

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

At Tue, 28 May 2019 04:45:24 -0700, Andres Freund <andres@anarazel.de> wrote in <20190528114524.dvj6ymap2virlzro@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, &record, &errormsg)
| == 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

On 12/07/2019 10:10, Kyotaro Horiguchi wrote:

At Tue, 28 May 2019 04:45:24 -0700, Andres Freund <andres@anarazel.de> wrote in <20190528114524.dvj6ymap2virlzro@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, &record, &errormsg)
| == 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

Thank you for the suggestion, Heikki.

At Mon, 29 Jul 2019 22:39:57 +0300, Heikki Linnakangas <hlinnaka@iki.fi> wrote in <e1ecb53b-663d-98ed-2249-dfa30a74f8c1@iki.fi>

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

On 22/08/2019 04:43, Kyotaro Horiguchi wrote:

At Mon, 29 Jul 2019 22:39:57 +0300, Heikki Linnakangas <hlinnaka@iki.fi> wrote in <e1ecb53b-663d-98ed-2249-dfa30a74f8c1@iki.fi>

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

At Thu, 22 Aug 2019 10:43:52 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in <20190822.104352.26342272.horikyota.ntt@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, &record, &errormsg) == 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

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

Hello.

709d003fbd hit this. Rebased.

Works fine but needs detailed verification and maybe further
cosmetic fixes.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 25 Sep 2019 15:50:32 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in <20190925.155032.13779064.horikyota.ntt@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

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

At Thu, 24 Oct 2019 14:51:01 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

Rebased.

0dc8ead463 hit this. Rebased.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Nov 27, 2019 at 12:09:23PM +0900, Kyotaro Horiguchi wrote:

0dc8ead463 hit this. Rebased.

Note: Moved to next CF.
--
Michael

On 2019-Nov-27, Kyotaro Horiguchi wrote:

At Thu, 24 Oct 2019 14:51:01 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

Rebased.

0dc8ead463 hit this. Rebased.

Please review the pg_waldump.c hunks in 0001; they revert recent changes.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services