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
