Attempt to consolidate reading of XLOG page

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 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


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Antonin Houska-2
Antonin Houska <[hidden email]> wrote:

> Alvaro Herrera <[hidden email]> wrote:
>
> > 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.

Eventually I found out that it's good to have a separate structure for the
read position because walsender calls the XLogRead() function directly, not
via the XLOG reader. Currently the structure name is XLogSegment (maybe
someone can propose better name) and it's a member of XLogReaderState. No
field of the new structure is duplicated now.

The next version of the patch is attached.

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


v02_001_Unexport_XLogReaderInvalReadState.patch (1K) Download Attachment
v02_002_Remove_TLI_argument_from_XLR_callback.patch (9K) Download Attachment
v02_003_Introduce_XLogSegment.patch (18K) Download Attachment
v02_004_Use_only_one_implementation_of_XLogRead.patch (15K) Download Attachment
v02_005_Cleanup.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Robert Haas
On Thu, May 2, 2019 at 12:18 PM Antonin Houska <[hidden email]> wrote:
> The next version of the patch is attached.

I don't think any of this looks acceptable:

+#ifndef FRONTEND
+/*
+ * Backend should have wal_segment_size variable initialized, segsize is not
+ * used.
+ */
+#define XLogFileNameCommon(tli, num, segsize) XLogFileNameP((tli), (num))
+#define xlr_error(...) ereport(ERROR, (errcode_for_file_access(),
errmsg(__VA_ARGS__)))
+#else
+static char xlr_error_msg[MAXFNAMELEN];
+#define XLogFileNameCommon(tli, num, segsize)
(XLogFileName(xlr_error_msg, (tli), (num), (segsize)),\
+    xlr_error_msg)
+#include "fe_utils/logging.h"
+/*
+ * Frontend application (currently only pg_waldump.c) cannot catch and further
+ * process errors, so they simply treat them as fatal.
+ */
+#define xlr_error(...) do {pg_log_fatal(__VA_ARGS__);
exit(EXIT_FAILURE); } while(0)
+#endif

The backend part doesn't look OK because depending on the value of a
global variable instead of getting the information via parameters
seems like a step backward.  The frontend part doesn't look OK because
it locks every application that uses the xlogreader stuff into using
pg_log_fatal when an error occurs, which may not be what everybody
wants to do.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Alvaro Herrera-9
On 2019-May-06, Robert Haas wrote:

> On Thu, May 2, 2019 at 12:18 PM Antonin Houska <[hidden email]> wrote:
> > The next version of the patch is attached.
>
> I don't think any of this looks acceptable:

I agree.  I inteded to suggest upthread to pass an additional argument
to XLogRead, which is a function that takes a message string and
SQLSTATE; in backend, the function does errstart / errstate / errmsg /
errfinish, and in frontend programs it does pg_log_fatal (and ignores
sqlstate).  The message must be sprintf'ed and translated by XLogRead.
(xlogreader.c could itself provide a default error reporting callback,
at least for frontend, to avoid repeating the code).  That way, if a
different frontend program wants to do something different, it's fairly
easy to pass a different function pointer.

BTW, having frontend's XLogFileNameCommon use a totally unrelated
variable for its printing is naughty.

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

Robert Haas
On Mon, May 6, 2019 at 2:21 PM Alvaro Herrera <[hidden email]> wrote:

> On 2019-May-06, Robert Haas wrote:
> > On Thu, May 2, 2019 at 12:18 PM Antonin Houska <[hidden email]> wrote:
> > > The next version of the patch is attached.
> >
> > I don't think any of this looks acceptable:
>
> I agree.  I inteded to suggest upthread to pass an additional argument
> to XLogRead, which is a function that takes a message string and
> SQLSTATE; in backend, the function does errstart / errstate / errmsg /
> errfinish, and in frontend programs it does pg_log_fatal (and ignores
> sqlstate).  The message must be sprintf'ed and translated by XLogRead.
> (xlogreader.c could itself provide a default error reporting callback,
> at least for frontend, to avoid repeating the code).  That way, if a
> different frontend program wants to do something different, it's fairly
> easy to pass a different function pointer.

It seems to me that it's better to unwind the stack i.e. have the
function return the error information to the caller and let the caller
do as it likes.  The other thread to which Horiguchi-san referred
earlier in this thread seems to me to have basically concluded that
the XLogPageReadCB callback to XLogReaderAllocate is a pain to use
because it doesn't unwind the stack, and work is under way over there
to get rid of that callback for just that reason.  Adding a new
callback for error-reporting would just be creating a new instance of
the same issue.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Antonin Houska-2
Robert Haas <[hidden email]> wrote:
> It seems to me that it's better to unwind the stack i.e. have the
> function return the error information to the caller and let the caller
> do as it likes.

Thanks for a hint. The next version tries to do that.

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


v03_001_Unexport_XLogReaderInvalReadState.patch (1K) Download Attachment
v03_002_Remove_TLI_argument_from_XLR_callback.patch (9K) Download Attachment
v03_003_Introduce_XLogSegment.patch (18K) Download Attachment
v03_004_Use_only_one_implementation_of_XLogRead.patch (15K) Download Attachment
v03_005_Cleanup.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Thomas Munro-5
On Tue, May 21, 2019 at 9:12 PM Antonin Houska <[hidden email]> wrote:
> Robert Haas <[hidden email]> wrote:
> > It seems to me that it's better to unwind the stack i.e. have the
> > function return the error information to the caller and let the caller
> > do as it likes.
>
> Thanks for a hint. The next version tries to do that.

Hi Antonin,

Could you please send a fresh rebase for the new Commitfest?

Thanks,


--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Attempt to consolidate reading of XLOG page

Antonin Houska-2
Thomas Munro <[hidden email]> wrote:

> On Tue, May 21, 2019 at 9:12 PM Antonin Houska <[hidden email]> wrote:
> > Robert Haas <[hidden email]> wrote:
> > > It seems to me that it's better to unwind the stack i.e. have the
> > > function return the error information to the caller and let the caller
> > > do as it likes.
> >
> > Thanks for a hint. The next version tries to do that.
>
> Hi Antonin,
>
> Could you please send a fresh rebase for the new Commitfest?
Rebased.

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


v04-0001-Make-XLogReaderInvalReadState-static.patch (2K) Download Attachment
v04-0002-Remove-TLI-from-some-argument-lists.patch (11K) Download Attachment
v04-0003-Introduce-XLogSegment-structure.patch (19K) Download Attachment
v04-0004-Use-only-xlogreader.c-XLogRead.patch (16K) Download Attachment
v04-0005-Remove-the-old-implemenations-of-XLogRead.patch (14K) Download Attachment