Date: Fri, 15 Nov 2019 18:41:45 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Antonin Houska <ah@cybertec.at>
Cc: Michael Paquier <michael@paquier.xyz>,
	Thomas Munro <thomas.munro@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Attempt to consolidate reading of XLOG page
Message-ID: <20191115214145.GA15829@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="6c2NcOVqGQ03X4Wi"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20191115214102.GA15616@alvherre.pgsql>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--6c2NcOVqGQ03X4Wi
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Attachment.

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

--6c2NcOVqGQ03X4Wi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="xlogread-extra.patch"

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 006c6298c9..e7a504c304 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -298,8 +298,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
 	 * byte to cover the whole record header, or at least the part of it that
 	 * fits on the same page.
 	 */
-	readOff = ReadPageInternal(state,
-							   targetPagePtr,
+	readOff = ReadPageInternal(state, targetPagePtr,
 							   Min(targetRecOff + SizeOfXLogRecord, XLOG_BLCKSZ));
 	if (readOff < 0)
 		goto err;
@@ -1019,23 +1018,22 @@ out:
 #endif							/* FRONTEND */
 
 /*
- * Read 'count' bytes from WAL fetched from timeline 'tli' into 'buf',
- * starting at location 'startptr'. 'seg' is the last segment used,
- * 'openSegment' is a callback to open the next segment and 'segcxt' is
- * additional segment info that does not fit into 'seg'.
+ * Read 'count' bytes into 'buf', starting at location 'startptr', from WAL
+ * fetched from timeline 'tli'.
  *
- * 'errinfo' should point to XLogReadError structure which will receive error
- * details in case the read fails.
+ * 'seg/segcxt' identify the last segment used.  'openSegment' is a callback
+ * to open the next segment, if necessary.
  *
- * Returns true if succeeded, false if failed.
+ * Returns true if succeeded, false if an error occurs, in which case
+ * 'errinfo' receives error details.
  *
  * XXX probably this should be improved to suck data directly from the
  * WAL buffers when possible.
  */
 bool
-XLogRead(char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
-		 WALOpenSegment *seg, WALSegmentContext *segcxt,
-		 WALSegmentOpen openSegment, WALReadError *errinfo)
+WALRead(char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
+		WALOpenSegment *seg, WALSegmentContext *segcxt,
+		WALSegmentOpen openSegment, WALReadError *errinfo)
 {
 	char	   *p;
 	XLogRecPtr	recptr;
@@ -1066,7 +1064,7 @@ XLogRead(char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
 			XLByteToSeg(recptr, nextSegNo, segcxt->ws_segsize);
 
 			/* Open the next segment in the caller's way. */
-			openSegment(nextSegNo, segcxt, &tli, &seg->ws_file);
+			seg->ws_file = openSegment(nextSegNo, segcxt, &tli);
 
 			/* Update the current segment info. */
 			seg->ws_tli = tli;
@@ -1085,11 +1083,11 @@ XLogRead(char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
 
 			if (lseek(seg->ws_file, (off_t) startoff, SEEK_SET) < 0)
 			{
-				errinfo->xlr_seek = true;
+				errinfo->xlr_oper = WRE_OPER_SEEK;
 				errinfo->xlr_errno = errno;
 				errinfo->xlr_req = 0;
 				errinfo->xlr_read = 0;
-				errinfo->xlr_seg = seg;
+				errinfo->xlr_seg = *seg;
 				return false;
 			}
 		}
@@ -1119,11 +1117,11 @@ XLogRead(char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
 
 		if (readbytes <= 0)
 		{
-			errinfo->xlr_seek = false;
+			errinfo->xlr_oper = WRE_OPER_OPEN;
 			errinfo->xlr_errno = errno;
 			errinfo->xlr_req = segbytes;
 			errinfo->xlr_read = readbytes;
-			errinfo->xlr_seg = seg;
+			errinfo->xlr_seg = *seg;
 			return false;
 		}
 
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 98118f484e..4047be5a0b 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -931,8 +931,8 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
 	 * as 'count', read the whole page anyway. It's guaranteed to be
 	 * zero-padded up to the page boundary if it's incomplete.
 	 */
-	if (!XLogRead(cur_page, targetPagePtr, XLOG_BLCKSZ, tli, &state->seg,
-				  &state->segcxt, wal_segment_open, &errinfo))
+	if (!WALRead(cur_page, targetPagePtr, XLOG_BLCKSZ, tli, &state->seg,
+				 &state->segcxt, wal_segment_open, &errinfo))
 		WALReadRaiseError(&errinfo);
 
 	/* number of valid bytes in the buffer */
@@ -941,36 +941,41 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
 
 /*
  * Backend-specific convenience code to handle read errors encountered by
- * XLogRead().
+ * WALRead().
  */
 void
 WALReadRaiseError(WALReadError *errinfo)
 {
-	WALOpenSegment *seg = errinfo->xlr_seg;
+	WALOpenSegment *seg = &errinfo->xlr_seg;
 	char	   *fname = XLogFileNameP(seg->ws_tli, seg->ws_segno);
 
-	if (errinfo->xlr_seek)
+	switch (errinfo->xlr_oper)
 	{
-		errno = errinfo->xlr_errno;
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not seek in log segment %s to offset %u: %m",
-						fname, seg->ws_off)));
-	}
-	else if (errinfo->xlr_read < 0)
-	{
-		errno = errinfo->xlr_errno;
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not read from log segment %s, offset %u, length %zu: %m",
-						fname, seg->ws_off, (Size) errinfo->xlr_req)));
-	}
-	else if (errinfo->xlr_read == 0)
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_DATA_CORRUPTED),
-				 errmsg("could not read from log segment %s, offset %u: read %d of %zu",
-						fname, seg->ws_off, errinfo->xlr_read,
-						(Size) errinfo->xlr_req)));
+		case WRE_OPER_SEEK:
+			errno = errinfo->xlr_errno;
+			ereport(ERROR,
+					(errcode_for_file_access(),
+					 errmsg("could not seek in log segment %s to offset %u: %m",
+							fname, seg->ws_off)));
+			break;
+
+		case WRE_OPER_OPEN:
+			if (errinfo->xlr_read < 0)
+			{
+				errno = errinfo->xlr_errno;
+				ereport(ERROR,
+						(errcode_for_file_access(),
+						 errmsg("could not read from log segment %s, offset %u, length %zu: %m",
+								fname, seg->ws_off, (Size) errinfo->xlr_req)));
+			}
+			else if (errinfo->xlr_read == 0)
+			{
+				ereport(ERROR,
+						(errcode(ERRCODE_DATA_CORRUPTED),
+						 errmsg("could not read from log segment %s, offset %u: read %d of %zu",
+								fname, seg->ws_off, errinfo->xlr_read,
+								(Size) errinfo->xlr_req)));
+			}
+			break;
 	}
 }
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 80ef5eb909..668537a6fb 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -248,8 +248,8 @@ static void LagTrackerWrite(XLogRecPtr lsn, TimestampTz local_flush_time);
 static TimeOffset LagTrackerRead(int head, XLogRecPtr lsn, TimestampTz now);
 static bool TransactionIdInRecentPast(TransactionId xid, uint32 epoch);
 
-static void WalSndSegmentOpen(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
-							  TimeLineID *tli_p, int *file_p);
+static int WalSndSegmentOpen(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
+							  TimeLineID *tli_p);
 
 
 /* Initialize walsender process before entering the main command loop */
@@ -789,16 +789,16 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
 		count = flushptr - targetPagePtr;	/* part of the page available */
 
 	/* now actually read the data, we know it's there */
-	if (!XLogRead(cur_page,
-				  targetPagePtr,
-				  XLOG_BLCKSZ,
-				  sendSeg->ws_tli,	/* Pass the current TLI because only
+	if (!WALRead(cur_page,
+				 targetPagePtr,
+				 XLOG_BLCKSZ,
+				 sendSeg->ws_tli,	/* Pass the current TLI because only
 									 * WalSndSegmentOpen controls whether new
 									 * TLI is needed. */
-				  sendSeg,
-				  sendCxt,
-				  WalSndSegmentOpen,
-				  &errinfo))
+				 sendSeg,
+				 sendCxt,
+				 WalSndSegmentOpen,
+				 &errinfo))
 		WALReadRaiseError(&errinfo);
 
 	/*
@@ -2376,11 +2376,12 @@ WalSndKill(int code, Datum arg)
 /*
  * Callback for XLogRead() to open the next segment.
  */
-void
+int
 WalSndSegmentOpen(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
-				  TimeLineID *tli_p, int *file_p)
+				  TimeLineID *tli_p)
 {
 	char		path[MAXPGPATH];
+	int			fd;
 
 	/*-------
 	 * When reading from a historic timeline, and there is a timeline switch
@@ -2417,25 +2418,25 @@ WalSndSegmentOpen(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
 	}
 
 	XLogFilePath(path, *tli_p, nextSegNo, segcxt->ws_segsize);
-	*file_p = BasicOpenFile(path, O_RDONLY | PG_BINARY);
+	fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
+	if (fd >= 0)
+		return fd;
 
-	if (*file_p < 0)
-	{
-		/*
-		 * If the file is not found, assume it's because the standby asked for
-		 * a too old WAL segment that has already been removed or recycled.
-		 */
-		if (errno == ENOENT)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("requested WAL segment %s has already been removed",
-							XLogFileNameP(*tli_p, nextSegNo))));
-		else
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not open file \"%s\": %m",
-							path)));
-	}
+	/*
+	 * If the file is not found, assume it's because the standby asked for
+	 * a too old WAL segment that has already been removed or recycled.
+	 */
+	if (errno == ENOENT)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("requested WAL segment %s has already been removed",
+						XLogFileNameP(*tli_p, nextSegNo))));
+	else
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not open file \"%s\": %m",
+						path)));
+	return -1;					/* keep compiler quiet */
 }
 
 /*
@@ -2674,16 +2675,16 @@ XLogSendPhysical(void)
 	enlargeStringInfo(&output_message, nbytes);
 
 retry:
-	if (!XLogRead(&output_message.data[output_message.len],
-				  startptr,
-				  nbytes,
-				  sendSeg->ws_tli,	/* Pass the current TLI because only
+	if (!WALRead(&output_message.data[output_message.len],
+				 startptr,
+				 nbytes,
+				 sendSeg->ws_tli,	/* Pass the current TLI because only
 									 * WalSndSegmentOpen controls whether new
 									 * TLI is needed. */
-				  sendSeg,
-				  sendCxt,
-				  WalSndSegmentOpen,
-				  &errinfo))
+				 sendSeg,
+				 sendCxt,
+				 WalSndSegmentOpen,
+				 &errinfo))
 		WALReadRaiseError(&errinfo);
 
 	/* See logical_read_xlog_page(). */
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index f1908a8868..cb71d33f48 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -280,12 +280,13 @@ identify_target_directory(char *directory, char *fname)
 	return NULL;				/* not reached */
 }
 
-static void
+static int
 WALDumpOpenSegment(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
-				   TimeLineID *tli_p, int *file_p)
+				   TimeLineID *tli_p)
 {
 	TimeLineID	tli = *tli_p;
 	char		fname[MAXPGPATH];
+	int			fd;
 	int			tries;
 
 	XLogFileName(fname, tli, nextSegNo, segcxt->ws_segsize);
@@ -298,9 +299,9 @@ WALDumpOpenSegment(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
 	 */
 	for (tries = 0; tries < 10; tries++)
 	{
-		*file_p = open_file_in_directory(segcxt->ws_dir, fname);
-		if (*file_p >= 0)
-			break;
+		fd = open_file_in_directory(segcxt->ws_dir, fname);
+		if (fd >= 0)
+			return fd;
 		if (errno == ENOENT)
 		{
 			int			save_errno = errno;
@@ -315,17 +316,16 @@ WALDumpOpenSegment(XLogSegNo nextSegNo, WALSegmentContext *segcxt,
 		break;
 	}
 
-	if (*file_p < 0)
-		fatal_error("could not find file \"%s\": %s",
-					fname, strerror(errno));
+	fatal_error("could not find file \"%s\": %s", fname, strerror(errno));
+	return -1;					/* keep compiler quiet */
 }
 
 /*
  * XLogReader read_page callback
  */
 static int
-XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
-				 XLogRecPtr targetPtr, char *readBuff)
+WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
+				XLogRecPtr targetPtr, char *readBuff)
 {
 	XLogDumpPrivate *private = state->private_data;
 	int			count = XLOG_BLCKSZ;
@@ -344,25 +344,31 @@ XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
 		}
 	}
 
-	if (!XLogRead(readBuff, targetPagePtr, count, private->timeline,
-				  &state->seg, &state->segcxt, WALDumpOpenSegment, &errinfo))
+	if (!WALRead(readBuff, targetPagePtr, count, private->timeline,
+				 &state->seg, &state->segcxt, WALDumpOpenSegment, &errinfo))
 	{
-		WALOpenSegment *seg = errinfo.xlr_seg;
+		WALOpenSegment *seg = &errinfo.xlr_seg;
 		char		fname[MAXPGPATH];
 
 		XLogFileName(fname, seg->ws_tli, seg->ws_segno,
 					 state->segcxt.ws_segsize);
 
-		if (errinfo.xlr_seek)
-			fatal_error("could not seek in file %s to offset %u: %s",
-						fname, seg->ws_off, strerror(errinfo.xlr_errno));
-		else if (errinfo.xlr_errno != 0)
-			fatal_error("could not read in file %s, offset %u, length %zu: %s",
-						fname, seg->ws_off, (Size) errinfo.xlr_req,
-						strerror(errinfo.xlr_errno));
-		else
-			fatal_error("could not read in file %s, offset %u: length: %zu",
-						fname, seg->ws_off, (Size) errinfo.xlr_req);
+		switch (errinfo.xlr_oper)
+		{
+			case WRE_OPER_SEEK:
+				fatal_error("could not seek in file %s to offset %u: %s",
+							fname, seg->ws_off, strerror(errinfo.xlr_errno));
+				break;
+			case WRE_OPER_OPEN:
+				if (errinfo.xlr_errno != 0)
+					fatal_error("could not read in file %s, offset %u, length %zu: %s",
+								fname, seg->ws_off, (Size) errinfo.xlr_req,
+								strerror(errinfo.xlr_errno));
+				else
+					fatal_error("could not read in file %s, offset %u: length: %zu",
+								fname, seg->ws_off, (Size) errinfo.xlr_req);
+				break;
+		}
 	}
 
 	return count;
@@ -1026,7 +1032,7 @@ main(int argc, char **argv)
 	/* done with argument parsing, do the actual work */
 
 	/* we have everything we need, start reading */
-	xlogreader_state = XLogReaderAllocate(WalSegSz, waldir, XLogDumpReadPage,
+	xlogreader_state = XLogReaderAllocate(WalSegSz, waldir, WALDumpReadPage,
 										  &private);
 	if (!xlogreader_state)
 		fatal_error("out of memory");
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 02d1514429..6b51538a62 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -217,9 +217,9 @@ extern XLogReaderState *XLogReaderAllocate(int wal_segment_size,
 /* Free an XLogReader */
 extern void XLogReaderFree(XLogReaderState *state);
 
-/* Initialize supporting structures */
 /*
- * Callback to open the specified WAL segment for reading.
+ * Callback to open the specified WAL segment for reading.  Returns a valid
+ * file descriptor when the file was opened successfully.
  *
  * "nextSegNo" is the number of the segment to be opened.
  *
@@ -229,15 +229,13 @@ extern void XLogReaderFree(XLogReaderState *state);
  * timeline in which the new segment should be found, but the callback can use
  * it to return the TLI that it actually opened.
  *
- * "file_p" points to an address the segment file descriptor should be stored
- * at.
- *
  * BasicOpenFile() is the preferred way to open the segment file in backend
  * code, whereas open(2) should be used in frontend.
  */
-typedef void (*WALSegmentOpen) (XLogSegNo nextSegNo, WALSegmentContext *segcxt,
-								TimeLineID *tli_p, int *file_p);
+typedef int (*WALSegmentOpen) (XLogSegNo nextSegNo, WALSegmentContext *segcxt,
+							   TimeLineID *tli_p);
 
+/* Initialize supporting structures */
 extern void WALOpenSegmentInit(WALOpenSegment *seg, WALSegmentContext *segcxt,
 							   int segsize, const char *waldir);
 
@@ -252,22 +250,29 @@ extern bool XLogReaderValidatePageHeader(XLogReaderState *state,
 #ifdef FRONTEND
 extern XLogRecPtr XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr);
 #endif							/* FRONTEND */
+
 /*
- * Error information that both backend and frontend caller can process.
+ * Error information from WALRead that both backend and frontend caller can
+ * process.
  */
+typedef enum
+{
+	WRE_OPER_OPEN = (1 << 0),
+	WRE_OPER_SEEK = (1 << 1)
+} WALReadErrorOper;
 typedef struct WALReadError
 {
-	bool		xlr_seek;		/* Error during lseek() ? */
+	WALReadErrorOper xlr_oper;	/* operation that caused the error */
 	int			xlr_errno;		/* errno set by the last read() / lseek() */
 	int			xlr_read;		/* Bytes read by the last read(). */
 	int			xlr_req;		/* Bytes requested to be read. */
-	WALOpenSegment *xlr_seg;	/* Segment we tried to read from. */
+	WALOpenSegment xlr_seg;		/* Segment we tried to read from. */
 } WALReadError;
 
-extern bool XLogRead(char *buf, XLogRecPtr startptr, Size count,
-					 TimeLineID tli, WALOpenSegment *seg,
-					 WALSegmentContext *segcxt, WALSegmentOpen openSegment,
-					 WALReadError *errinfo);
+extern bool WALRead(char *buf, XLogRecPtr startptr, Size count,
+					TimeLineID tli, WALOpenSegment *seg,
+					WALSegmentContext *segcxt, WALSegmentOpen openSegment,
+					WALReadError *errinfo);
 
 /* Functions for decoding an XLogRecord */
 
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index 1ffc765c6a..3fe5b36748 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -55,4 +55,5 @@ extern void XLogReadDetermineTimeline(XLogReaderState *state,
 									  XLogRecPtr wantPage, uint32 wantLength);
 
 void		WALReadRaiseError(WALReadError *errinfo);
+
 #endif

--6c2NcOVqGQ03X4Wi--