Attempt to consolidate reading of XLOG page

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Attempt to consolidate reading of XLOG page

Antonin Houska-2
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.

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


reuse_xlogread.patch (37K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Kyotaro HORIGUCHI-2
Hello.

At Thu, 11 Apr 2019 18:05:42 +0200, Antonin Houska <[hidden email]> wrote in <[hidden email]>

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

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



Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Michael Paquier-2
On Fri, Apr 12, 2019 at 12:27:11PM +0900, Kyotaro HORIGUCHI wrote:
> 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.
> Any other opinions, or thoughts?

The focus is on the stability of v12 for the next couple of months, so
please make sure to register it to the next CF if you want feedback.

Here are some basic thoughts after a very quick lookup.

+/*
+ * Position in XLOG file while reading it.
+ */
+typedef struct XLogReadPos
+{
+   int segFile;                /* segment file descriptor */
+   XLogSegNo   segNo;          /* segment number */
+   uint32 segOff;              /* offset in the segment */
+   TimeLineID tli;     /* timeline ID of the currently open file */
+
+   char    *dir;               /* directory (only needed by
frontends) */
+} XLogReadPos;
Not sure if there is any point to split that with the XLOG reader
status.

+static void fatal_error(const char *fmt,...) pg_attribute_printf(1, 2);
+
+static void
+fatal_error(const char *fmt,...)
In this more confusion accumulates with something which has enough
warts on HEAD when it comes to declare locally equivalents to the
elog() for src/common/.

+/*
+ * This is a front-end counterpart of XLogFileNameP.
+ */
+static char *
+XLogFileNameFE(TimeLineID tli, XLogSegNo segno)
+{
+   char       *result = palloc(MAXFNAMELEN);
+
+   XLogFileName(result, tli, segno, WalSegSz);
+   return result;
+}
We could use a pointer to an allocated area.  Or even better, just a
static variable as this just gets used for error messages to store
temporarily the segment name in a routine part of perhaps
xlogreader.c.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Haribabu Kommi-2
In reply to this post by Antonin Houska-2

On Fri, Apr 12, 2019 at 2:06 AM Antonin Houska <[hidden email]> wrote:
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.

I didn't check the code, but it is good to combine all the 3 page read functions
into one instead of spreading the logic.

Regards,
Haribabu Kommi
Fujitsu Australia
Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Alvaro Herrera-9
In reply to this post by Antonin Houska-2
On 2019-Apr-11, Antonin Houska wrote:

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

I agree that xlog reading is pretty messy.

I think ifdef'ing the way XLogRead reports errors is not great.  Maybe
we can pass a function pointer that is to be called in case of errors?
Not sure about the walsize; maybe it can be a member in XLogReadPos, and
given to XLogReadInitPos()?  (Maybe rename XLogReadPos as
XLogReadContext or something like that, indicating it's not just the
read position.)

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Antonin Houska-2
In reply to this post by Kyotaro HORIGUCHI-2
Kyotaro HORIGUCHI <[hidden email]> wrote:

> Hello.
>
> At Thu, 11 Apr 2019 18:05:42 +0200, Antonin Houska <[hidden email]> wrote in <[hidden email]>
> > 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@...

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


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Antonin Houska-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> wrote:

> On Fri, Apr 12, 2019 at 12:27:11PM +0900, Kyotaro HORIGUCHI wrote:
> > 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.
> > Any other opinions, or thoughts?
>
> The focus is on the stability of v12 for the next couple of months, so
> please make sure to register it to the next CF if you want feedback.

ok, will do. (A link to mailing list is needed for the CF entry, so I had to
post something anyway :-) Since I don't introduce any kind of "cool new
feature" here, I believe it did not disturb much.)

> Here are some basic thoughts after a very quick lookup.
> ...

Thanks.

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


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Antonin Houska-2
In reply to this post by Alvaro Herrera-9
Alvaro Herrera <[hidden email]> wrote:

> I agree that xlog reading is pretty messy.
>
> I think ifdef'ing the way XLogRead reports errors is not great.  Maybe
> we can pass a function pointer that is to be called in case of errors?

I'll try a bit harder to evaluate the existing approaches to report the same
error on both backend and frontend side.

> Not sure about the walsize; maybe it can be a member in XLogReadPos, and
> given to XLogReadInitPos()?  (Maybe rename XLogReadPos as
> XLogReadContext or something like that, indicating it's not just the
> read position.)

As pointed out by others, XLogReadPos is not necessary. So if XLogRead()
receives XLogReaderState instead, it can get the segment size from there.

Thanks.

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