|From:||Heikki Linnakangas <hlinnaka(at)iki(dot)fi>|
|To:||Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>|
|Subject:||Re: Remove page-read callback from XLogReaderState.|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 22/08/2019 04:43, Kyotaro Horiguchi wrote:
> At Mon, 29 Jul 2019 22:39:57 +0300, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote in <e1ecb53b-663d-98ed-2249-dfa30a74f8c1(at)iki(dot)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.
|Next Messageemail@example.com||2019-08-22 09:46:38||Email to hackers for test coverage|
|Previous Message||Kyotaro Horiguchi||2019-08-22 09:13:46||Re: when the IndexScan reset to the next ScanKey for in operator|