Re: Attempt to consolidate reading of XLOG page

Hello.

At Thu, 11 Apr 2019 18:05:42 +0200, Antonin Houska <ah(at)cybertec(dot)at> wrote in <14984(dot)1554998742(at)spoje(dot)net>
> While working on the instance encryption I found it annoying to apply
> decyption of XLOG page to three different functions. Attached is a patch that
> tries to merge them all into one function, XLogRead(). The existing
> implementations differ in the way new segment is opened. So I added a pointer
> to callback function as a new argument. This callback handles the specific
> ways to determine segment file name and to open the file.
>
> I can split the patch into multiple diffs to make detailed review easier, but
> first I'd like to hear if anything is seriously wrong about this
> design. Thanks.

This patch changes XLogRead to allow using other than
BasicOpenFile to open a segment, and use XLogReaderState.private
to hold a new struct XLogReadPos for the segment reader. The new
struct is heavily duplicated with XLogReaderState and I'm not
sure the rason why the XLogReadPos is needed.

Anyway, in the first place, such two distinct-but-highly-related
callbacks makes things too complex. Heikki said that the log
reader stuff is better not using callbacks and I agree to that. I
did that once for my own but the code is no longer
applicable. But it seems to be the time to do that.

https://www.postgresql.org/message-id/47215279-228d-f30d-35d1-16af695e53f3@iki.fi

That would seems like follows. That refactoring separates log
reader and page reader.

for(;;)
{
rc = XLogReadRecord(reader, startptr, errormsg);

if (rc == XLREAD_SUCCESS)
{
/* great, got record */
}
if (rc == XLREAD_INVALID_PAGE || XLREAD_INVALID_RECORD)
{
elog(ERROR, "invalid record");
}
if (rc == XLREAD_NEED_DATA)
{
/*
* Read a page from disk, and place it into reader->readBuf
*/
XLogPageRead(reader->readPagePtr, /* page to read */
reader->reqLen /* # of bytes to read */ );
/*
* Now that we have read the data that XLogReadRecord()
* requested, call it again.
*/
continue;
}
}

DecodingContextFindStartpoint(ctx)
do
{
read_local_xlog_page(....);
rc = XLogReadRecord (reader);
while (rc == XLREAD_NEED_DATA);

I'm going to do that again.

Any other opinions, or thoughts?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center