Re: Remove page-read callback from XLogReaderState.

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: andres(at)anarazel(dot)de, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Remove page-read callback from XLogReaderState.
Date: 2019-08-22 09:43:30
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

- Heikki

Attachment Content-Type Size
refactor-xlogreaderstate-callback-heikki-v2.patch text/x-patch 86.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message 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