Date: Tue, 17 Sep 2019 19:15:21 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Antonin Houska <ah@cybertec.at>
Cc: 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: <20190917221521.GA15733@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="PEIAKu/WMn1b1Hv9"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <13170.1568024430@antos>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


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

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

--PEIAKu/WMn1b1Hv9
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="xlogopen-rename.patch"

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 2a80bc5823..3e03d72a18 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -100,7 +100,7 @@ XLogReaderAllocate(int wal_segment_size, XLogPageReadCB pagereadfunc,
 	}
 
 	/* Initialize segment pointer. */
-	XLogSegmentInit(&state->seg, wal_segment_size);
+	WALOpenSegmentInit(&state->seg, wal_segment_size);
 
 	state->read_page = pagereadfunc;
 	/* system_identifier initialized to zeroes above */
@@ -1006,7 +1006,7 @@ out:
  * Initialize the passed segment pointer.
  */
 void
-XLogSegmentInit(XLogSegment *seg, int size)
+WALOpenSegmentInit(WALOpenSegment *seg, int size)
 {
 	seg->file = -1;
 	seg->num = 0;
@@ -1032,7 +1032,7 @@ XLogSegmentInit(XLogSegment *seg, int size)
  */
 bool
 XLogRead(char *buf, XLogRecPtr startptr, Size count,
-		 TimeLineID *tli, XLogSegment *seg, XLogOpenSegment openSegment)
+		 TimeLineID *tli, WALOpenSegment *seg, WALSegmentOpen openSegment)
 {
 	char	   *p;
 	XLogRecPtr	recptr;
@@ -1120,7 +1120,7 @@ XLogRead(char *buf, XLogRecPtr startptr, Size count,
  * Backend-specific code to handle errors encountered by XLogRead().
  */
 void
-XLogReadProcessError(XLogSegment *seg)
+XLogReadProcessError(WALOpenSegment *seg)
 {
 	if (errno != 0)
 	{
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 5b0ba39f17..2b9758c3ce 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -776,13 +776,15 @@ XLogReadDetermineTimeline(XLogReaderState *state, XLogRecPtr wantPage, uint32 wa
  */
 static void
 read_local_xlog_page_open_segment(XLogSegNo nextSegNo, TimeLineID *tli,
-								  XLogSegment *seg)
+								  WALOpenSegment *seg)
 {
 	char		path[MAXPGPATH];
 
 	XLogFilePath(path, *tli, nextSegNo, seg->size);
 	seg->file = BasicOpenFile(path, O_RDONLY | PG_BINARY);
 
+	/* XXX should we clear the other fields of WALOpenSegment?? */
+
 	if (seg->file < 0)
 	{
 		if (errno == ENOENT)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 072d46d853..a5ca834020 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -128,7 +128,7 @@ bool		log_replication_commands = false;
  */
 bool		wake_wal_senders = false;
 
-static XLogSegment *sendSeg = NULL;
+static WALOpenSegment *sendSeg = NULL;
 
 /*
  * These variables keep track of the state of the timeline we're currently
@@ -248,7 +248,7 @@ static TimeOffset LagTrackerRead(int head, XLogRecPtr lsn, TimestampTz now);
 static bool TransactionIdInRecentPast(TransactionId xid, uint32 epoch);
 
 static void WalSndOpenSegment(XLogSegNo nextSegNo, TimeLineID *tli,
-							  XLogSegment *seg);
+							  WALOpenSegment *seg);
 
 
 /* Initialize walsender process before entering the main command loop */
@@ -279,8 +279,9 @@ InitWalSender(void)
 	lag_tracker = MemoryContextAllocZero(TopMemoryContext, sizeof(LagTracker));
 
 	/* Make sure we can remember the current read position in XLOG. */
-	sendSeg = (XLogSegment *) MemoryContextAlloc(TopMemoryContext, sizeof(XLogSegment));
-	XLogSegmentInit(sendSeg, wal_segment_size);
+	sendSeg = (WALOpenSegment *)
+		MemoryContextAlloc(TopMemoryContext, sizeof(WALOpenSegment));
+	WALOpenSegmentInit(sendSeg, wal_segment_size);
 }
 
 /*
@@ -2354,7 +2355,7 @@ WalSndKill(int code, Datum arg)
  * Callback for XLogRead() to open the next segment.
  */
 void
-WalSndOpenSegment(XLogSegNo nextSegNo, TimeLineID *tli, XLogSegment *seg)
+WalSndOpenSegment(XLogSegNo nextSegNo, TimeLineID *tli, WALOpenSegment *seg)
 {
 	char		path[MAXPGPATH];
 
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index cd44090d36..8466890748 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -296,7 +296,7 @@ identify_target_directory(XLogDumpPrivate *private, char *directory,
 }
 
 static void
-XLogDumpOpenSegment(XLogSegNo nextSegNo, TimeLineID *tli, XLogSegment *seg)
+WALDumpOpenSegment(XLogSegNo nextSegNo, TimeLineID *tli, WALOpenSegment *seg)
 {
 	char		fname[MAXPGPATH];
 	int			tries;
@@ -358,9 +358,9 @@ XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
 	}
 
 	if (!XLogRead(readBuff, targetPagePtr, count, &private->timeline,
-				  &state->seg, XLogDumpOpenSegment))
+				  &state->seg, WALDumpOpenSegment))
 	{
-		XLogSegment *seg = &state->seg;
+		WALOpenSegment *seg = &state->seg;
 		int			err = errno;
 		char		fname[MAXPGPATH];
 		int			save_errno = errno;
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 4731023ccc..c93b255d84 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -32,9 +32,9 @@
 #include "access/xlogrecord.h"
 
 /*
- * Position in XLOG file while reading it.
+ * WALOpenSegment represents a WAL segment being read.
  */
-typedef struct XLogSegment
+typedef struct WALOpenSegment
 {
 	int			file;			/* segment file descriptor */
 	XLogSegNo	num;			/* segment number */
@@ -44,7 +44,7 @@ typedef struct XLogSegment
 	char	   *dir;			/* directory (only needed by frontends) */
 	int			size;			/* segment size */
 	int			last_req;		/* the amount of data requested last time */
-} XLogSegment;
+} WALOpenSegment;
 
 typedef struct XLogReaderState XLogReaderState;
 
@@ -165,7 +165,7 @@ struct XLogReaderState
 	uint32		readLen;
 
 	/* last read XLOG position for data currently in readBuf */
-	XLogSegment seg;
+	WALOpenSegment seg;
 
 	/*
 	 * beginning of prior page read, and its TLI.  Doesn't necessarily
@@ -227,7 +227,7 @@ extern XLogRecPtr XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr);
 #endif							/* FRONTEND */
 
 /*
- * Callback to open the specified XLOG segment nextSegNo in timeline *tli for
+ * Callback to open the specified WAL segment nextSegNo in timeline *tli for
  * reading, and assign the descriptor to ->file. BasicOpenFile() is the
  * preferred way to open the segment file in backend code, whereas open(2)
  * should be used in frontend.
@@ -235,15 +235,15 @@ extern XLogRecPtr XLogFindNextRecord(XLogReaderState *state, XLogRecPtr RecPtr);
  * If NULL is passed for tli, the callback must determine the timeline
  * itself. In any case it's supposed to eventually set ->tli.
  */
-typedef void (*XLogOpenSegment) (XLogSegNo nextSegNo, TimeLineID *tli,
-								 XLogSegment *seg);
+typedef void (*WALSegmentOpen) (XLogSegNo nextSegNo, TimeLineID *tli,
+								WALOpenSegment *seg);
 
-extern void XLogSegmentInit(XLogSegment *seg, int size);
+extern void WALOpenSegmentInit(WALOpenSegment *seg, int size);
 extern bool XLogRead(char *buf, XLogRecPtr startptr, Size count,
-					 TimeLineID *tli, XLogSegment *seg,
-					 XLogOpenSegment openSegment);
+					 TimeLineID *tli, WALOpenSegment *seg,
+					 WALSegmentOpen openSegment);
 #ifndef FRONTEND
-void		XLogReadProcessError(XLogSegment *seg);
+void		XLogReadProcessError(WALOpenSegment *seg);
 #endif
 
 /* Functions for decoding an XLogRecord */

--PEIAKu/WMn1b1Hv9--