Hi all, I have been playing with the new APIs of xlogreader.h, and while merging some of my stuff with 13, I found the handling around ->seg.ws_file overcomplicated and confusing as it is necessary for a plugin to manipulate directly the fd of an opened segment in the WAL segment open/close callbacks.
Wouldn't it be cleaner to limit the exposition of ->seg.ws_file to the user if possible? There are cases like a WAL sender where you cannot do that, but something that came to my mind is to make WALSegmentOpenCB return the fd of the opened segment, and pass down the fd to close to WALSegmentCloseCB. Then xlogreader.c is in charge of resetting the field when a segment is closed. Any thoughts? -- Michael
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index c21b0ba972..fcf57b32eb 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -63,10 +63,10 @@ typedef int (*XLogPageReadCB) (XLogReaderState *xlogreader,
int reqLen,
XLogRecPtr targetRecPtr,
char *readBuf);
-typedef void (*WALSegmentOpenCB) (XLogReaderState *xlogreader,
+typedef int (*WALSegmentOpenCB) (XLogReaderState *xlogreader,
XLogSegNo nextSegNo,
TimeLineID *tli_p);
-typedef void (*WALSegmentCloseCB) (XLogReaderState *xlogreader);
+typedef void (*WALSegmentCloseCB) (XLogReaderState *xlogreader, int fd);
typedef struct XLogReaderRoutine
{
@@ -93,10 +93,10 @@ typedef struct XLogReaderRoutine
XLogPageReadCB page_read;
/*
- * Callback to open the specified WAL segment for reading. ->seg.ws_file
- * shall be set to the file descriptor of the opened segment. In case of
- * failure, an error shall be raised by the callback and it shall not
- * return.
+ * Callback to open the specified WAL segment for reading. Needs to
+ * return a file descriptor that will be saved as ->seg.ws_file,
+ * indicating the opened WAL segment. In case of failure, an error
+ * shall be raised by the callback and it shall not return.
*
* "nextSegNo" is the number of the segment to be opened.
*
@@ -107,8 +107,8 @@ typedef struct XLogReaderRoutine
WALSegmentOpenCB segment_open;
/*
- * WAL segment close callback. ->seg.ws_file shall be set to a negative
- * number.
+ * WAL segment close callback. "fd" is the file descriptor of the
+ * segment to close.
*/
WALSegmentCloseCB segment_close;
} XLogReaderRoutine;
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index e59b6cf3a9..c9cbdaffae 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -50,10 +50,10 @@ extern void FreeFakeRelcacheEntry(Relation fakerel);
extern int read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *cur_page);
-extern void wal_segment_open(XLogReaderState *state,
+extern int wal_segment_open(XLogReaderState *state,
XLogSegNo nextSegNo,
TimeLineID *tli_p);
-extern void wal_segment_close(XLogReaderState *state);
+extern void wal_segment_close(XLogReaderState *state, int fd);
extern void XLogReadDetermineTimeline(XLogReaderState *state,
XLogRecPtr wantPage, uint32 wantLength);
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 5995798b58..b026be4cbe 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -139,7 +139,10 @@ XLogReaderFree(XLogReaderState *state)
int block_id;
if (state->seg.ws_file != -1)
- state->routine.segment_close(state);
+ {
+ state->routine.segment_close(state, state->seg.ws_file);
+ state->seg.ws_file = -1;
+ }
for (block_id = 0; block_id <= XLR_MAX_BLOCK_ID; block_id++)
{
@@ -1089,10 +1092,13 @@ WALRead(XLogReaderState *state,
XLogSegNo nextSegNo;
if (state->seg.ws_file >= 0)
- state->routine.segment_close(state);
+ {
+ state->routine.segment_close(state, state->seg.ws_file);
+ state->seg.ws_file = -1;
+ }
XLByteToSeg(recptr, nextSegNo, state->segcxt.ws_segsize);
- state->routine.segment_open(state, nextSegNo, &tli);
+ state->seg.ws_file = state->routine.segment_open(state, nextSegNo, &tli);
/* This shouldn't happen -- indicates a bug in segment_open */
Assert(state->seg.ws_file >= 0);
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 322b0e8ff5..6d3d8f6a53 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -784,17 +784,18 @@ XLogReadDetermineTimeline(XLogReaderState *state, XLogRecPtr wantPage, uint32 wa
}
/* XLogReaderRoutine->segment_open callback for local pg_wal files */
-void
+int
wal_segment_open(XLogReaderState *state, XLogSegNo nextSegNo,
TimeLineID *tli_p)
{
TimeLineID tli = *tli_p;
char path[MAXPGPATH];
+ int fd = -1;
XLogFilePath(path, tli, nextSegNo, state->segcxt.ws_segsize);
- state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY);
- if (state->seg.ws_file >= 0)
- return;
+ fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
+ if (fd >= 0)
+ return fd;
if (errno == ENOENT)
ereport(ERROR,
@@ -806,15 +807,16 @@ wal_segment_open(XLogReaderState *state, XLogSegNo nextSegNo,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
path)));
+
+ return -1; /* keep compiler quiet */
}
/* stock XLogReaderRoutine->segment_close callback */
void
-wal_segment_close(XLogReaderState *state)
+wal_segment_close(XLogReaderState *state, int fd)
{
- close(state->seg.ws_file);
+ close(fd);
/* need to check errno? */
- state->seg.ws_file = -1;
}
/*
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 86847cbb54..c01eb2d2b6 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -254,7 +254,7 @@ 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(XLogReaderState *state, XLogSegNo nextSegNo,
+static int WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
TimeLineID *tli_p);
static void UpdateSpillStats(LogicalDecodingContext *ctx);
@@ -316,7 +316,7 @@ WalSndErrorCleanup(void)
pgstat_report_wait_end();
if (xlogreader != NULL && xlogreader->seg.ws_file >= 0)
- wal_segment_close(xlogreader);
+ wal_segment_close(xlogreader, xlogreader->seg.ws_file);
if (MyReplicationSlot != NULL)
ReplicationSlotRelease();
@@ -2456,11 +2456,12 @@ WalSndKill(int code, Datum arg)
}
/* XLogReaderRoutine->segment_open callback */
-static void
+static int
WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
TimeLineID *tli_p)
{
char path[MAXPGPATH];
+ int fd = -1;
/*-------
* When reading from a historic timeline, and there is a timeline switch
@@ -2497,9 +2498,9 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
}
XLogFilePath(path, *tli_p, nextSegNo, state->segcxt.ws_segsize);
- state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY);
- if (state->seg.ws_file >= 0)
- return;
+ fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
+ if (fd >= 0)
+ return fd;
/*
* If the file is not found, assume it's because the standby asked for a
@@ -2522,6 +2523,8 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
path)));
+
+ return -1; /* keep compiler quiet */
}
/*
@@ -2686,7 +2689,7 @@ XLogSendPhysical(void)
{
/* close the current file. */
if (xlogreader->seg.ws_file >= 0)
- wal_segment_close(xlogreader);
+ wal_segment_close(xlogreader, xlogreader->seg.ws_file);
/* Send CopyDone */
pq_putmessage_noblock('c', NULL, 0);
@@ -2791,7 +2794,7 @@ retry:
if (reload && xlogreader->seg.ws_file >= 0)
{
- wal_segment_close(xlogreader);
+ wal_segment_close(xlogreader, xlogreader->seg.ws_file);
goto retry;
}
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index d1a0678935..6410a869f3 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -280,13 +280,14 @@ identify_target_directory(char *directory, char *fname)
}
/* pg_waldump's XLogReaderRoutine->segment_open callback */
-static void
+static int
WALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo,
TimeLineID *tli_p)
{
TimeLineID tli = *tli_p;
char fname[MAXPGPATH];
int tries;
+ int fd = -1;
XLogFileName(fname, tli, nextSegNo, state->segcxt.ws_segsize);
@@ -298,9 +299,9 @@ WALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo,
*/
for (tries = 0; tries < 10; tries++)
{
- state->seg.ws_file = open_file_in_directory(state->segcxt.ws_dir, fname);
- if (state->seg.ws_file >= 0)
- return;
+ fd = open_file_in_directory(state->segcxt.ws_dir, fname);
+ if (fd >= 0)
+ return fd;
if (errno == ENOENT)
{
int save_errno = errno;
@@ -316,6 +317,7 @@ WALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo,
}
fatal_error("could not find file \"%s\": %m", fname);
+ return -1; /* keep compiler quiet */
}
/*
@@ -323,11 +325,10 @@ WALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo,
* wal_segment_close
*/
static void
-WALDumpCloseSegment(XLogReaderState *state)
+WALDumpCloseSegment(XLogReaderState *state, int fd)
{
- close(state->seg.ws_file);
+ close(fd);
/* need to check errno? */
- state->seg.ws_file = -1;
}
/* pg_waldump's XLogReaderRoutine->page_read callback */
signature.asc
Description: PGP signature
