Re: [PATCH] XLogReadRecord returns pointer to currently read page

On 26.10.2018 10:33, Kyotaro HORIGUCHI wrote:
> Hello.
>
> At Tue, 23 Oct 2018 10:25:27 +0500, Andrey Lepikhov <a(dot)lepikhov(at)postgrespro(dot)ru> wrote in <2553f2b0-0e39-eb0e-d382-6c0ed08ca8ae(at)postgrespro(dot)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.

--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company