v23-0001-Add-check-in-WALReadFromBuffers-against-requeste.patch

application/x-patch

Filename: v23-0001-Add-check-in-WALReadFromBuffers-against-requeste.patch
Type: application/x-patch
Part: 3
Message: Re: Improve WALRead() to suck data directly from WAL buffers when possible

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: format-patch
Series: patch v23-0001
Subject: Add check in WALReadFromBuffers against requested WAL
File+
src/backend/access/transam/xlog.c 18 18
src/backend/access/transam/xlogreader.c 3 0
src/include/access/xlog.h 1 0
From 09e76fd9336352d6a34cc320c0b369e158b50b54 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 16 Feb 2024 06:54:10 +0000
Subject: [PATCH v23 1/5] Add check in WALReadFromBuffers against requested WAL

---
 src/backend/access/transam/xlog.c       | 36 ++++++++++++-------------
 src/backend/access/transam/xlogreader.c |  3 +++
 src/include/access/xlog.h               |  1 +
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4e14c242b1..884be9c805 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -698,7 +698,6 @@ static void ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos,
 									  XLogRecPtr *EndPos, XLogRecPtr *PrevPtr);
 static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
 							  XLogRecPtr *PrevPtr);
-static XLogRecPtr WaitXLogInsertionsToFinish(XLogRecPtr upto);
 static char *GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli);
 static XLogRecPtr XLogBytePosToRecPtr(uint64 bytepos);
 static XLogRecPtr XLogBytePosToEndRecPtr(uint64 bytepos);
@@ -1493,7 +1492,7 @@ WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt)
  * uninitialized page), and the inserter might need to evict an old WAL buffer
  * to make room for a new one, which in turn requires WALWriteLock.
  */
-static XLogRecPtr
+XLogRecPtr
 WaitXLogInsertionsToFinish(XLogRecPtr upto)
 {
 	uint64		bytepos;
@@ -1710,13 +1709,15 @@ GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli)
  * of bytes read successfully.
  *
  * Fewer than 'count' bytes may be read if some of the requested WAL data has
- * already been evicted from the WAL buffers, or if the caller requests data
- * that is not yet available.
+ * already been evicted from the WAL buffers.
  *
  * No locks are taken.
  *
  * The 'tli' argument is only used as a convenient safety check so that
  * callers do not read from WAL buffers on a historical timeline.
+ *
+ * Note: It is the caller's responsibility to ensure requested WAL up to
+ * 'startptr'+'count' is available by using WaitXLogInsertionsToFinish().
  */
 Size
 WALReadFromBuffers(char *dstbuf, XLogRecPtr startptr, Size count,
@@ -1724,26 +1725,25 @@ WALReadFromBuffers(char *dstbuf, XLogRecPtr startptr, Size count,
 {
 	char	   *pdst = dstbuf;
 	XLogRecPtr	recptr = startptr;
-	XLogRecPtr	upto;
-	Size		nbytes;
+	Size		nbytes = count;
 
 	if (RecoveryInProgress() || tli != GetWALInsertionTimeLine())
 		return 0;
 
 	Assert(!XLogRecPtrIsInvalid(startptr));
 
-	/*
-	 * Don't read past the available WAL data.
-	 *
-	 * Check using local copy of LogwrtResult. Ordinarily it's been updated by
-	 * the caller when determining how far to read; but if not, it just means
-	 * we'll read less data.
-	 *
-	 * XXX: the available WAL could be extended to the WAL insert pointer by
-	 * calling WaitXLogInsertionsToFinish().
-	 */
-	upto = Min(startptr + count, LogwrtResult.Write);
-	nbytes = upto - startptr;
+#ifdef USE_ASSERT_CHECKING
+	{
+		XLogRecPtr	upto = startptr + count;
+		XLogRecPtr	insert_pos = GetXLogInsertRecPtr();
+
+		if (upto > insert_pos)
+			ereport(ERROR,
+					(errmsg("cannot read past end of current insert position; request %X/%X, insert position %X/%X",
+							LSN_FORMAT_ARGS(upto),
+							LSN_FORMAT_ARGS(insert_pos))));
+	}
+#endif
 
 	/*
 	 * Loop through the buffers without a lock. For each buffer, atomically
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 74a6b11866..ae9904e7e4 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1500,6 +1500,9 @@ err:
  *
  * Returns true if succeeded, false if an error occurs, in which case
  * 'errinfo' receives error details.
+ *
+ * Note: It is the caller's responsibility to ensure requested WAL is written
+ * to disk, that is 'startptr'+'count' > LogwrtResult.Write.
  */
 bool
 WALRead(XLogReaderState *state,
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 76787a8267..74606a6846 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -252,6 +252,7 @@ extern XLogRecPtr GetLastImportantRecPtr(void);
 
 extern void SetWalWriterSleeping(bool sleeping);
 
+extern XLogRecPtr WaitXLogInsertionsToFinish(XLogRecPtr upto);
 extern Size WALReadFromBuffers(char *dstbuf, XLogRecPtr startptr, Size count,
 							   TimeLineID tli);
 
-- 
2.34.1