Attempt to consolidate reading of XLOG page

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

/messages/by-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

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

On Fri, Apr 12, 2019 at 2:06 AM Antonin Houska <ah@cybertec.at> 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

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

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.

/messages/by-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

Michael Paquier <michael@paquier.xyz> 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

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Antonin Houska <ah@cybertec.at> wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> 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

On Thu, May 2, 2019 at 12:18 PM Antonin Houska <ah@cybertec.at> 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

On 2019-May-06, Robert Haas wrote:

On Thu, May 2, 2019 at 12:18 PM Antonin Houska <ah@cybertec.at> 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

On Mon, May 6, 2019 at 2:21 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2019-May-06, Robert Haas wrote:

On Thu, May 2, 2019 at 12:18 PM Antonin Houska <ah@cybertec.at> 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

Robert Haas <robertmhaas@gmail.com> 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

On Tue, May 21, 2019 at 9:12 PM Antonin Houska <ah@cybertec.at> wrote:

Robert Haas <robertmhaas@gmail.com> 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

Thomas Munro <thomas.munro@gmail.com> wrote:

On Tue, May 21, 2019 at 9:12 PM Antonin Houska <ah@cybertec.at> wrote:

Robert Haas <robertmhaas@gmail.com> 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

Hi Antonin, could you please rebase again?

Thanks

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

Pushed 0001.

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

Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Hi Antonin, could you please rebase again?

Attached.

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

I was confused by the struct name XLogSegment -- the struct is used to
represent a WAL segment while it's kept open, rather than just a WAL
segment in abstract. Also, now that we've renamed everything to use the
term WAL, it seems wrong to use the name XLog for new structs. I
propose the name WALOpenSegment for the struct, which solves both
problems. (Its initializer function would get the name
WALOpenSegmentInit.)

Now, the patch introduces a callback for XLogRead, the type of which is
called XLogOpenSegment. If we rename it from XLog to WAL, both names
end up the same. I propose to rename the function type to
WALSegmentOpen, which in a "noun-verb" view of the world, represents the
action of opening a WAL segment.

I attach a patch for all this renaming, on top of your series.

I wonder if each of those WALSegmentOpen callbacks should reset [at
least some members of] the struct; they're already in charge of setting
->file, and apparently we're leaving the responsibility of setting the
rest of the members to XLogRead. That seems weird. Maybe we should say
that the CB should only open the segment and not touch the struct at all
and XLogRead is in charge of everything. Perhaps the other way around
-- the CB should set everything correctly ... I'm not sure which is
best. But having half here and half there seems a recipe for confusion
and bugs.

Another thing I didn't like much is that everything seems to assume that
the only error possible from XLogRead is a read error. Maybe that's
okay, because it seems to be the current reality, but it seemed odd.

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

Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I was confused by the struct name XLogSegment -- the struct is used to
represent a WAL segment while it's kept open, rather than just a WAL
segment in abstract. Also, now that we've renamed everything to use the
term WAL, it seems wrong to use the name XLog for new structs. I
propose the name WALOpenSegment for the struct, which solves both
problems. (Its initializer function would get the name
WALOpenSegmentInit.)

Now, the patch introduces a callback for XLogRead, the type of which is
called XLogOpenSegment. If we rename it from XLog to WAL, both names
end up the same. I propose to rename the function type to
WALSegmentOpen, which in a "noun-verb" view of the world, represents the
action of opening a WAL segment.

I attach a patch for all this renaming, on top of your series.

ok, thanks.

In addition I renamed WalSndOpenSegment() to WalSndSegmentOpen() and
read_local_xlog_page_open_segment() to read_local_xlog_page_segment_open()

I wonder if each of those WALSegmentOpen callbacks should reset [at
least some members of] the struct; they're already in charge of setting
->file, and apparently we're leaving the responsibility of setting the
rest of the members to XLogRead. That seems weird. Maybe we should say
that the CB should only open the segment and not touch the struct at all
and XLogRead is in charge of everything. Perhaps the other way around
-- the CB should set everything correctly ... I'm not sure which is
best. But having half here and half there seems a recipe for confusion
and bugs.

ok, I've changed the CB signature. Now it receives poiners to the two
variables that it can change while the "seg" argument is documented as
read-only. To indicate that the CB should determine timeline itself, I
introduced a new constant InvalidTimeLineID, see the 0004 part.

Another thing I didn't like much is that everything seems to assume that
the only error possible from XLogRead is a read error. Maybe that's
okay, because it seems to be the current reality, but it seemed odd.

In this case I only moved the ereport() code from XLogRead() away (so that
both backend and frontend can call the function). Given that the code to open
WAL segment is now in the callbacks, the only thing that XLogRead() can
ereport is that read() failed. BTW, I introduced one more structure,
XLogReadError, in this patch version. I think it's better than adding
error-specific fields to the WALOpenSegment structure.

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