Re: Remove page-read callback from XLogReaderState.

At Thu, 22 Aug 2019 10:43:52 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in <20190822(dot)104352(dot)26342272(dot)horikyota(dot)ntt(at)gmail(dot)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