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

Thank you for taking this.

At Mon, 19 Nov 2018 10:27:29 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20181119012728.GA1578@paquier.xyz>
> On Fri, Nov 16, 2018 at 03:23:55PM +0900, Michael Paquier wrote:
> > I was looking at this patch, and shouldn't we worry about compatibility
> > with plugins or utilities which look directly at what's in readRecordBuf
> > for the record contents?  Let's not forget that the contents of
> > XLogReaderState are public.
> 
> And a couple of days later, committed.  I did not notice first that the
> comments of xlogreader.h mention that a couple of areas are private, and
> readRecordBuf is part of that, so objection withdrawn.

It wasn't changed the way the function replaces the content of
the buffer. (Regardless of whether it is private or not, I
believe no one tries to write directly there outside the
function.)  Also the memory pointed by the retuned pointer from
the previous code was regarded as read-only. The only point to
notice was the lifetime of the returned pointer, I suppose.

> There is a comment in xlog.c (on top of ReadRecord) referring to
> readRecordBuf which has not been removed as part of 7fcbf6a4 when WAL
> reading has been moved to its own facility.  The original comment was
> from 0ffe11a.  So I have removed the comment at the same time.

- * The record is copied into readRecordBuf, so that on successful return,
- * the returned record pointer always points there.

Yeah, it is incorrect in some sense, but the comment was
suggesting the lifetime of the pointed record. So I'd like to
have a comment like this instead.

+ * The record is copied into xlogreader, so that on successful return,
+ * the returned record pointer always points there.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center