From: Antonin Houska <ah@cybertec.at>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
cc: pgsql-hackers@postgresql.org
Subject: Re: Attempt to consolidate reading of XLOG page
In-reply-to: <20190412.122711.158276916.horiguchi.kyotaro@lab.ntt.co.jp>
References: <14984.1554998742@spoje.net>
 <20190412.122711.158276916.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
   message dated "Fri, 12 Apr 2019 12:27:11 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <12353.1555316525.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 15 Apr 2019 10:22:05 +0200
Message-ID: <12354.1555316525@localhost>
Precedence: bulk

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

> Hello.
> =

> At Thu, 11 Apr 2019 18:05:42 +0200, Antonin Houska <ah@cybertec.at> wrot=
e 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 pat=
ch 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 spec=
ific
> > ways to determine segment file name and to open the file.
> > =

> > I can split the patch into multiple diffs to make detailed review easi=
er, 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 XLogReaderStat=
e
> and I'm not sure the rason why the XLogReadPos is needed.

ok, I missed the fact that XLogReaderState already contains most of the in=
fo
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-16af695e53=
f3@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 XLogReadReco=
rd()
usually controls those sources too.

However the callback I pass to XLogRead() is different: if it fails, it si=
mply
raises ERROR. Since this indicates rather low-level problem, there's no re=
ason
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 =3D XLogReadRecord(reader, startptr, errormsg);
> =

>     if (rc =3D=3D XLREAD_SUCCESS)
>     {
>         /* great, got record */
>     }
>     if (rc =3D=3D XLREAD_INVALID_PAGE || XLREAD_INVALID_RECORD)
>     {
>         elog(ERROR, "invalid record");
>     }
>     if (rc =3D=3D 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 =3D XLogReadRecord (reader);
>   while (rc =3D=3D 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 ev=
en
if you change the XLOG reader API, you don't care what read_local_xlog_pag=
e()
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