At Fri, 26 Oct 2018 11:43:14 +0500, Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote in
<ea3dca6a-b898-e7a8-dcfa-b3ad227a6349@postgrespro.ru>
>
>
> On 26.10.2018 10:33, Kyotaro HORIGUCHI wrote:
> > Hello.
> > At Tue, 23 Oct 2018 10:25:27 +0500, Andrey Lepikhov
> > <a.lepikhov@postgrespro.ru> wrote in
> > <2553f2b0-0e39-eb0e-d382-6c0ed08ca8ae@postgrespro.ru>
> >>
> >> On 23.10.2018 0:53, Heikki Linnakangas wrote:
> >>> I'd expect the decompression to read from the on-disk buffer, and
> >>> unpack to readRecordBuf, I still don't see a need to copy the packed
> >>> record to readRecordBuf. If there is a need for that, though, the
> >>> patch that implements the packing or compression can add the memcpy()
> >>> where it needs it.
> >>
> >> I agree with it. Eventually, placement of the WAL-record can be
> >> defined by comparison the record, readBuf and readRecordBuf pointers.
> >> In attachment new version of the patch.
> > This looks quite clear and what it does is reasonable to me.
> > Applies cleanly on top of current master and no regression seen.
> > Just one comment. This patch leaves the following code.
> > > /* Record does not cross a page boundary */
> > > if (!ValidXLogRecord(state, record, RecPtr))
> > > goto err;
> > >
> > > state->EndRecPtr = RecPtr + MAXALIGN(total_len);
> > !>
> > > state->ReadRecPtr = RecPtr;
> > > }
> > The empty line (marked by '!') looks a bit too much standing out
> > after this patch. Could you remove the line? Then could you
> > transpose the two assignments if you don't mind?
>
> Thanks, see attachment.
Thanks.
This patch eliminates unnecessary copying that was done for
non-continued records. Now the return value of XLogReadRecord
directly points into page buffer holded in XLogReaderStats. It is
safe because no caller site uses the returned pointer beyond the
replacement of buffer content at the next call to the same
function.
The code looks fine and correct.
This applied on HEAD@master cleanly.
Passed all regression tests.
Passed all tests under src/test/recovery
I marked this "Ready for Committer".
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center