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

On 22/10/2018 20:54, Andrey Lepikhov wrote:
> 
> 
> On 22.10.2018 2:06, Heikki Linnakangas wrote:
>> On 17/08/2018 06:47, Andrey Lepikhov wrote:
>>> I propose the patch for fix one small code defect.
>>> The XLogReadRecord() function reads the pages of a WAL segment that
>>> contain a WAL-record. Then it creates a readRecordBuf buffer in private
>>> memory of a backend and copy record from the pages to the readRecordBuf
>>> buffer. Pointer 'record' is set to the beginning of the readRecordBuf
>>> buffer.
>>>
>>> But if the WAL-record is fully placed on one page, the XLogReadRecord()
>>> function forgets to bind the "record" pointer with the beginning of the
>>> readRecordBuf buffer. In this case, XLogReadRecord() returns a pointer
>>> to an internal xlog page. This patch fixes the defect.
>>
>> Hmm. I agree it looks weird the way it is. But I wonder, why do we even
>> copy the record to readRecordBuf, if it fits on the WAL page? Returning
>> a pointer to the xlog page buffer seems OK in that case. What if we
>> remove the memcpy(), instead?
> 
> It depends on the PostgreSQL roadmap. If we want to compress main data
> and encode something to reduce a WAL-record size, than the
> representation of the WAL-record in shared buffers (packed) and in local
> memory (unpacked) will be different and the patch is needed.

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.

- Heikki