Re: Attempt to consolidate reading of XLOG page

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