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

From Heikki Linnakangas
Subject Re: Remove page-read callback from XLogReaderState.
Date
Msg-id 4f7a5fad-fa04-b0a3-231b-56d200b646dc@iki.fi
Whole thread Raw
In response to Re: Remove page-read callback from XLogReaderState.  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Kyotaro Horiguchi
Date:
Subject: Re: when the IndexScan reset to the next ScanKey for in operator
Next
From: "movead.li@highgo.ca"
Date:
Subject: Email to hackers for test coverage