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(at)paquier(dot)xyz> wrote in <20181119012728(dot)GA1578(at)paquier(dot)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