Re: Attempt to consolidate reading of XLOG page

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> Hello.
>
> At Thu, 11 Apr 2019 18:05:42 +0200, Antonin Houska <ah@cybertec.at> wrote in <14984.1554998742@spoje.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,

Good point. The acceptable ways to open file on both frontend and backend side
need to be documented.

> 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.

ok, I missed the fact that XLogReaderState already contains most of the info
that I put into XLogReadPos. So XLogReadPos is not 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

Thanks for the link. My understanding is that the drawback of the
XLogReaderState.read_page callback is that it cannot easily switch between
XLOG sources in order to handle failure because the caller of XLogReadRecord()
usually controls those sources too.

However the callback I pass to XLogRead() is different: if it fails, it simply
raises ERROR. Since this indicates rather low-level problem, there's no reason
for this callback to try to recover from the failure.

> 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?

I don't see an overlap between what you do and what I do. It seems that even
if you change the XLOG reader API, you don't care what read_local_xlog_page()
does internally. What I try to fix is XLogRead(), and that is actually a
subroutine of read_local_xlog_page().

--
Antonin Houska
Web: https://www.cybertec-postgresql.com