Thank you for taking this. At Mon, 19 Nov 2018 10:27:29 +0900, Michael Paquier <mich...@paquier.xyz> wrote in <20181119012728.ga1...@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
