public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bharath Rupireddy <[email protected]>
To: Andres Freund <[email protected]>
Cc: Jeff Davis <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: Nathan Bossart <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Subject: Re: Improve WALRead() to suck data directly from WAL buffers when possible
Date: Thu, 7 Dec 2023 15:59:00 +0530
Message-ID: <CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@mail.gmail.com> (raw)
In-Reply-To: <CALj2ACXo-_boM3h-PJuvfuvFzP1nv2wcZBoZN_YbRbQBGQYAUg@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CALj2ACU2DGH-BB_187N7gfVkLdxfJ+doVTcteS0ugLn_c8mLOw@mail.gmail.com>
<CALj2ACVgVgA5BSSrEYO2eTMEGB=QUbcYosYm3vZ3R2=GPB6tNw@mail.gmail.com>
<[email protected]>
<CALj2ACVjzHgQvONnTLffyBJRACPwhsf-cYG4TQ2KHKrFfFop-w@mail.gmail.com>
<[email protected]>
<CALj2ACV123Ke6wn05QCnCgJhFpcarGCv7sKM1p39t+qpUNHUgQ@mail.gmail.com>
<[email protected]>
<CALj2ACUSydTHPcucftm4XdiJ4osH8zeNENyADd-zfDzEMf6akA@mail.gmail.com>
<[email protected]>
<CALj2ACXo-_boM3h-PJuvfuvFzP1nv2wcZBoZN_YbRbQBGQYAUg@mail.gmail.com>
On Mon, Nov 13, 2023 at 7:02 PM Bharath Rupireddy
<[email protected]> wrote:
>
> On Fri, Nov 10, 2023 at 2:28 AM Andres Freund <[email protected]> wrote:
>
> > I think the code needs to make sure that *never* happens. That seems unrelated
> > to holding or not holding WALBufMappingLock. Even if the page header is
> > already valid, I don't think it's ok to just read/parse WAL data that's
> > concurrently being modified.
> >
> > We can never allow WAL being read that's past
> > XLogBytePosToRecPtr(XLogCtl->Insert->CurrBytePos)
> > as it does not exist.
>
> Agreed. Erroring out in XLogReadFromBuffers() if passed in WAL is past
> the CurrBytePos is an option. Another cleaner way is to just let the
> caller decide what it needs to do (retry or error out) - fill an error
> message in XLogReadFromBuffers() and return as-if nothing was read or
> return a special negative error code like XLogDecodeNextRecord so that
> the caller can deal with it.
In the attached v17 patch, I've ensured that the XLogReadFromBuffers
returns when the caller requests a WAL that's past the current insert
position at the moment.
> Also, reading CurrBytePos with insertpos_lck spinlock can come in the
> way of concurrent inserters. A possible way is to turn both
> CurrBytePos and PrevBytePos 64-bit atomics so that
> XLogReadFromBuffers() can read CurrBytePos without any lock atomically
> and leave it to the caller to deal with non-existing WAL reads.
>
> > And if the to-be-read LSN is between
> > XLogCtl->LogwrtResult->Write and XLogBytePosToRecPtr(Insert->CurrBytePos)
> > we need to call WaitXLogInsertionsToFinish() before copying the data.
>
> Agree to wait for all in-flight insertions to the pages we're about to
> read to finish. But, reading XLogCtl->LogwrtRqst.Write requires either
> XLogCtl->info_lck spinlock or WALWriteLock. Maybe turn
> XLogCtl->LogwrtRqst.Write a 64-bit atomic and read it without any
> lock, rely on
> WaitXLogInsertionsToFinish()'s return value i.e. if
> WaitXLogInsertionsToFinish() returns a value >= Insert->CurrBytePos,
> then go read that page from WAL buffers.
In the attached v17 patch, I've ensured that the XLogReadFromBuffers
waits for all in-progress insertions to finish when the caller
requests WAL that's past the current write position and before the
current insert position.
I've also ensured that the XLogReadFromBuffers returns special return
codes for various scenarios (when asked to read in recovery, read on a
different TLI, read a non-existent WAL and so on.) instead of it
erroring out. This gives flexibility to the caller to decide what to
do.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[application/octet-stream] v17-0001-Use-64-bit-atomics-for-xlblocks-array-elements.patch (7.2K, ../CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@mail.gmail.com/2-v17-0001-Use-64-bit-atomics-for-xlblocks-array-elements.patch)
download | inline diff:
From 9264635eb097f0f2e85f733d358b67e6d038edad Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Fri, 1 Dec 2023 04:53:30 +0000
Subject: [PATCH v17] Use 64-bit atomics for xlblocks array elements
In AdvanceXLInsertBuffer(), xlblocks value of a WAL buffer page is
updated only at the end after the page is initialized with all
zeros. A problem with this approach is that anyone reading
xlblocks and WAL buffer page without holding WALBufMappingLock
will see the wrong page contents if the read happens before the
xlblocks is marked with a new entry in AdvanceXLInsertBuffer() at
the end.
To fix this issue, xlblocks is made to use 64-bit atomics instead
of XLogRecPtr and the xlblocks value is marked with
InvalidXLogRecPtr just before the page initialization begins. Once
the page initialization finishes, only then the actual value of
the newly initialized page is marked in xlblocks. A write barrier
is placed in between xlblocks update with InvalidXLogRecPtr and
the page initialization to not cause any memory ordering problems.
With this fix, one can read xlblocks and WAL buffer page without
WALBufMappingLock in the following manner:
endptr = pg_atomic_read_u64(&XLogCtl->xlblocks[idx]);
/* Requested WAL isn't available in WAL buffers. */
if (expectedEndPtr != endptr)
break;
page = XLogCtl->pages + idx * (Size) XLOG_BLCKSZ;
data = page + ptr % XLOG_BLCKSZ;
...
pg_read_barrier();
...
memcpy(buf, data, bytes_to_read);
...
pg_read_barrier();
/* Recheck if the page still exists in WAL buffers. */
endptr = pg_atomic_read_u64(&XLogCtl->xlblocks[idx]);
/* Return if the page got initalized while we were reading it */
if (expectedEndPtr != endptr)
break;
---
src/backend/access/transam/xlog.c | 55 ++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6526bd4f43..b3c08f3980 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -501,7 +501,7 @@ typedef struct XLogCtlData
* WALBufMappingLock.
*/
char *pages; /* buffers for unwritten XLOG pages */
- XLogRecPtr *xlblocks; /* 1st byte ptr-s + XLOG_BLCKSZ */
+ pg_atomic_uint64 *xlblocks; /* 1st byte ptr-s + XLOG_BLCKSZ */
int XLogCacheBlck; /* highest allocated xlog buffer index */
/*
@@ -1634,20 +1634,19 @@ GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli)
* out to disk and evicted, and the caller is responsible for making sure
* that doesn't happen.
*
- * However, we don't hold a lock while we read the value. If someone has
- * just initialized the page, it's possible that we get a "torn read" of
- * the XLogRecPtr if 64-bit fetches are not atomic on this platform. In
- * that case we will see a bogus value. That's ok, we'll grab the mapping
- * lock (in AdvanceXLInsertBuffer) and retry if we see anything else than
- * the page we're looking for. But it means that when we do this unlocked
- * read, we might see a value that appears to be ahead of the page we're
- * looking for. Don't PANIC on that, until we've verified the value while
- * holding the lock.
+ * However, we don't hold a lock while we read the value. If someone is
+ * just about to initialize or has just initialized the page, it's
+ * possible that we get InvalidXLogRecPtr. That's ok, we'll grab the
+ * mapping lock (in AdvanceXLInsertBuffer) and retry if we see anything
+ * else than the page we're looking for. But it means that when we do this
+ * unlocked read, we might see a value that appears to be ahead of the
+ * page we're looking for. Don't PANIC on that, until we've verified the
+ * value while holding the lock.
*/
expectedEndPtr = ptr;
expectedEndPtr += XLOG_BLCKSZ - ptr % XLOG_BLCKSZ;
- endptr = XLogCtl->xlblocks[idx];
+ endptr = pg_atomic_read_u64(&XLogCtl->xlblocks[idx]);
if (expectedEndPtr != endptr)
{
XLogRecPtr initializedUpto;
@@ -1678,7 +1677,7 @@ GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli)
WALInsertLockUpdateInsertingAt(initializedUpto);
AdvanceXLInsertBuffer(ptr, tli, false);
- endptr = XLogCtl->xlblocks[idx];
+ endptr = pg_atomic_read_u64(&XLogCtl->xlblocks[idx]);
if (expectedEndPtr != endptr)
elog(PANIC, "could not find WAL buffer for %X/%X",
@@ -1865,7 +1864,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
* be zero if the buffer hasn't been used yet). Fall through if it's
* already written out.
*/
- OldPageRqstPtr = XLogCtl->xlblocks[nextidx];
+ OldPageRqstPtr = pg_atomic_read_u64(&XLogCtl->xlblocks[nextidx]);
if (LogwrtResult.Write < OldPageRqstPtr)
{
/*
@@ -1934,6 +1933,20 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
NewPage = (XLogPageHeader) (XLogCtl->pages + nextidx * (Size) XLOG_BLCKSZ);
+ /*
+ * Make sure to mark the xlblocks with InvalidXLogRecPtr before the
+ * initialization of the page begins so that others reading xlblocks
+ * without holding a lock, will know that the page initialization has
+ * just begun.
+ */
+ pg_atomic_write_u64(&XLogCtl->xlblocks[nextidx], InvalidXLogRecPtr);
+
+ /*
+ * A write barrier here helps to not reorder the above xlblocks atomic
+ * write with below page initialization.
+ */
+ pg_write_barrier();
+
/*
* Be sure to re-zero the buffer so that bytes beyond what we've
* written will look like zeroes and not valid XLOG records...
@@ -1987,8 +2000,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
*/
pg_write_barrier();
- *((volatile XLogRecPtr *) &XLogCtl->xlblocks[nextidx]) = NewPageEndPtr;
-
+ pg_atomic_write_u64(&XLogCtl->xlblocks[nextidx], NewPageEndPtr);
XLogCtl->InitializedUpTo = NewPageEndPtr;
npages++;
@@ -2206,7 +2218,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
* if we're passed a bogus WriteRqst.Write that is past the end of the
* last page that's been initialized by AdvanceXLInsertBuffer.
*/
- XLogRecPtr EndPtr = XLogCtl->xlblocks[curridx];
+ XLogRecPtr EndPtr = pg_atomic_read_u64(&XLogCtl->xlblocks[curridx]);
if (LogwrtResult.Write >= EndPtr)
elog(PANIC, "xlog write request %X/%X is past end of log %X/%X",
@@ -4708,10 +4720,13 @@ XLOGShmemInit(void)
* needed here.
*/
allocptr = ((char *) XLogCtl) + sizeof(XLogCtlData);
- XLogCtl->xlblocks = (XLogRecPtr *) allocptr;
- memset(XLogCtl->xlblocks, 0, sizeof(XLogRecPtr) * XLOGbuffers);
- allocptr += sizeof(XLogRecPtr) * XLOGbuffers;
+ XLogCtl->xlblocks = (pg_atomic_uint64 *) allocptr;
+ allocptr += sizeof(pg_atomic_uint64) * XLOGbuffers;
+ for (i = 0; i < XLOGbuffers; i++)
+ {
+ pg_atomic_init_u64(&XLogCtl->xlblocks[i], InvalidXLogRecPtr);
+ }
/* WAL insertion locks. Ensure they're aligned to the full padded size */
allocptr += sizeof(WALInsertLockPadded) -
@@ -5748,7 +5763,7 @@ StartupXLOG(void)
memcpy(page, endOfRecoveryInfo->lastPage, len);
memset(page + len, 0, XLOG_BLCKSZ - len);
- XLogCtl->xlblocks[firstIdx] = endOfRecoveryInfo->lastPageBeginPtr + XLOG_BLCKSZ;
+ pg_atomic_write_u64(&XLogCtl->xlblocks[firstIdx], endOfRecoveryInfo->lastPageBeginPtr + XLOG_BLCKSZ);
XLogCtl->InitializedUpTo = endOfRecoveryInfo->lastPageBeginPtr + XLOG_BLCKSZ;
}
else
--
2.34.1
[application/octet-stream] v17-0002-Allow-WAL-reading-from-WAL-buffers.patch (13.1K, ../CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@mail.gmail.com/3-v17-0002-Allow-WAL-reading-from-WAL-buffers.patch)
download | inline diff:
From d855d4e01ef552c79267a2c06904200dfc637224 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 7 Dec 2023 08:17:06 +0000
Subject: [PATCH v17] Allow WAL reading from WAL buffers
This commit adds WALRead() the capability to read WAL from WAL
buffers when possible. When requested WAL isn't available in WAL
buffers, the WAL is read from the WAL file as usual.
This commit benefits the callers of WALRead(), that are walsenders
and pg_walinspect. They can now avoid reading WAL from the WAL
file (possibly avoiding disk IO). Tests show that the WAL buffers
hit ratio stood at 95% for 1 primary, 1 sync standby, 1 async
standby, with pgbench --scale=300 --client=32 --time=900. In other
words, the walsenders avoided 95% of the time reading from the
file/avoided pread system calls:
https://www.postgresql.org/message-id/CALj2ACXKKK%3DwbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54%2BNa%3DQ%40mail.gmail.com
This commit also benefits when direct IO is enabled for WAL.
Reading WAL from WAL buffers puts back the performance close to
that of without direct IO for WAL:
https://www.postgresql.org/message-id/CALj2ACV6rS%2B7iZx5%2BoAvyXJaN4AG-djAQeM1mrM%3DYSDkVrUs7g%40mail.gmail.com
This commit paves the way for the following features in future:
- Improves synchronous replication performance by replicating
directly from WAL buffers.
- A opt-in way for the walreceivers to receive unflushed WAL.
More details here:
https://www.postgresql.org/message-id/20231011224353.cl7c2s222dw3de4j%40awork3.anarazel.de
Author: Bharath Rupireddy
Reviewed-by: Dilip Kumar, Andres Freund
Reviewed-by: Nathan Bossart, Kuntal Ghosh
Discussion: https://www.postgresql.org/message-id/CALj2ACXKKK%3DwbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54%2BNa%3DQ%40mail.gmail.com
---
src/backend/access/transam/xlog.c | 189 ++++++++++++++++++++++++
src/backend/access/transam/xlogreader.c | 47 +++++-
src/backend/access/transam/xlogutils.c | 11 +-
src/backend/replication/walsender.c | 10 +-
src/include/access/xlog.h | 23 +++
5 files changed, 268 insertions(+), 12 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4ebd918198..b0eb6d5d56 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1707,6 +1707,195 @@ GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli)
return cachedPos + ptr % XLOG_BLCKSZ;
}
+/*
+ * Read WAL from WAL buffers.
+ *
+ * This function reads 'bytes_to_read' bytes of WAL from WAL buffers into
+ * 'buf' starting at location 'startptr' on timeline 'tli' and returns
+ * appropriate result code and fills total read bytes if any into
+ * 'bytes_read'.
+ *
+ * Points to note:
+ *
+ * - This function reads as much as it can from WAL buffers, meaning, it may
+ * not read all the requested 'bytes_to_read' bytes. Caller must be aware of
+ * this and deal with it.
+ *
+ * - This function reads WAL from WAL buffers without holding any lock. First
+ * it reads xlblocks atomically for checking page existence, then it reads the
+ * page contents, validates. Finally, it rechecks the page existence by
+ * rereading xlblocks, if the read page is replaced, it discards read page and
+ * returns.
+ *
+ * - This function is not available for frontend code as WAL buffers is an
+ * internal mechanism to the server.
+ *
+ * - Caller must look at the result code to take appropriate action such as
+ * error out on failure or emit warning or continue.
+ *
+ * - This function waits for any in-progress WAL insertions to WAL buffers to
+ * finish.
+ */
+XLogReadFromBuffersResult
+XLogReadFromBuffers(XLogRecPtr startptr,
+ TimeLineID tli,
+ Size bytes_to_read,
+ char *buf,
+ Size *bytes_read)
+{
+ XLogRecPtr ptr;
+ Size nbytes;
+ char *dst;
+ uint64 bytepos;
+ XLogReadFromBuffersResult result = XLREADBUFS_OK;
+
+ *bytes_read = 0;
+
+ /* WAL buffers aren't in use when server is in recovery. */
+ if (RecoveryInProgress())
+ return XLREADBUFS_IN_RECOVERY;
+
+ /* WAL is inserted into WAL buffers on current server's insertion TLI. */
+ if (tli != GetWALInsertionTimeLine())
+ return XLREADBUFS_NOT_INSERT_TLI;
+
+ if (XLogRecPtrIsInvalid(startptr))
+ return XLREADBUFS_INVALID_INPUT;
+
+ ptr = startptr;
+ nbytes = bytes_to_read;
+ dst = buf;
+
+ while (nbytes > 0)
+ {
+ XLogRecPtr expectedEndPtr;
+ XLogRecPtr endptr;
+ int idx;
+ char *page;
+ char *data;
+ XLogPageHeader phdr;
+ Size nread;
+ XLogRecPtr reservedUpto;
+ XLogwrtResult LogwrtResult;
+
+ idx = XLogRecPtrToBufIdx(ptr);
+ expectedEndPtr = ptr;
+ expectedEndPtr += XLOG_BLCKSZ - ptr % XLOG_BLCKSZ;
+ endptr = pg_atomic_read_u64(&XLogCtl->xlblocks[idx]);
+
+ /* Requested WAL isn't available in WAL buffers. */
+ if (expectedEndPtr != endptr)
+ break;
+
+ /*
+ * We found WAL buffer page containing given XLogRecPtr. Get starting
+ * address of the page and a pointer to the right location of given
+ * XLogRecPtr in that page.
+ */
+ page = XLogCtl->pages + idx * (Size) XLOG_BLCKSZ;
+ data = page + ptr % XLOG_BLCKSZ;
+
+ /*
+ * Make sure we don't read xlblocks up above before the page contents
+ * down below.
+ */
+ pg_read_barrier();
+
+ nread = 0;
+
+ /* Read what is wanted, not the whole page. */
+ if ((data + nbytes) <= (page + XLOG_BLCKSZ))
+ {
+ /* All the bytes are in one page. */
+ nread = nbytes;
+ }
+ else
+ {
+ /*
+ * All the bytes are not in one page. Read available bytes on the
+ * current page, copy them over to output buffer and continue to
+ * read remaining bytes.
+ */
+ nread = XLOG_BLCKSZ - (data - page);
+ Assert(nread > 0 && nread <= nbytes);
+ }
+
+ Assert(nread > 0);
+ memcpy(dst, data, nread);
+
+ /*
+ * Make sure we don't read xlblocks down below before the page
+ * contents up above.
+ */
+ pg_read_barrier();
+
+ /* Recheck if the read page still exists in WAL buffers. */
+ endptr = pg_atomic_read_u64(&XLogCtl->xlblocks[idx]);
+
+ /* Return if the page got initalized while we were reading it. */
+ if (expectedEndPtr != endptr)
+ break;
+
+ /* Read the current insert position */
+ SpinLockAcquire(&XLogCtl->Insert.insertpos_lck);
+ bytepos = XLogCtl->Insert.CurrBytePos;
+ SpinLockRelease(&XLogCtl->Insert.insertpos_lck);
+
+ reservedUpto = XLogBytePosToEndRecPtr(bytepos);
+
+ /*
+ * We can't allow WAL being read is past the current insert position
+ * as it does not yet exist.
+ */
+ if ((ptr + nread) > reservedUpto)
+ {
+ result = XLREADBUFS_NON_EXISTENT_WAL;
+ break;
+ }
+
+ SpinLockAcquire(&XLogCtl->info_lck);
+ LogwrtResult = XLogCtl->LogwrtResult;
+ SpinLockRelease(&XLogCtl->info_lck);
+
+ /* Wait for any in-progress WAL insertions to WAL buffers to finish. */
+ if ((ptr + nread) > LogwrtResult.Write &&
+ (ptr + nread) <= reservedUpto)
+ WaitXLogInsertionsToFinish(ptr + nread);
+
+ /*
+ * Typically, we must not read a WAL buffer page that just got
+ * initialized, because we waited enough for the in-progress WAL
+ * insertions to finish above. However, there can exists a slight
+ * window after the above wait finishes in which the read buffer page
+ * can get replaced especially under high WAL generation rates. So,
+ * let's not account such buffer page.
+ */
+ phdr = (XLogPageHeader) page;
+ if (!(phdr->xlp_magic == XLOG_PAGE_MAGIC &&
+ phdr->xlp_pageaddr == (ptr - (ptr % XLOG_BLCKSZ)) &&
+ phdr->xlp_tli == tli))
+ {
+ result = XLREADBUFS_UNINITIALIZED_WAL;
+ break;
+ }
+
+ dst += nread;
+ ptr += nread;
+ *bytes_read += nread;
+ nbytes -= nread;
+ }
+
+ /* We never read more than what the caller has asked for. */
+ Assert(*bytes_read <= bytes_to_read);
+
+ ereport(DEBUG1,
+ errmsg_internal("read %zu bytes out of %zu bytes from WAL buffers for given start LSN %X/%X, timeline ID %u",
+ *bytes_read, bytes_to_read,
+ LSN_FORMAT_ARGS(startptr), tli));
+
+ return result;
+}
+
/*
* Converts a "usable byte position" to XLogRecPtr. A usable byte position
* is the position starting from the beginning of WAL, excluding all WAL
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index e0baa86bd3..bb8871b671 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1473,17 +1473,54 @@ err:
* 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.
+ * When possible, this function reads data directly from WAL buffers. When
+ * requested WAL isn't available in WAL buffers, the WAL is read from the WAL
+ * file as usual. The callers may avoid reading WAL from the WAL file thus
+ * reducing read system calls or even disk IOs.
*/
bool
-WALRead(XLogReaderState *state,
- char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
- WALReadError *errinfo)
+WALRead(XLogReaderState *state, char *buf, XLogRecPtr startptr,
+ Size count, TimeLineID tli, WALReadError *errinfo)
{
char *p;
XLogRecPtr recptr;
Size nbytes;
+#ifndef FRONTEND
+ Size nread;
+#endif
+
+#ifndef FRONTEND
+
+ /*
+ * Try reading WAL from WAL buffers. Frontend code has no idea of WAL
+ * buffers.
+ */
+ (void) XLogReadFromBuffers(startptr, tli, count, buf, &nread);
+
+ if (nread > 0)
+ {
+ /*
+ * Check if we have read fully (hit), partially (partial hit) or
+ * nothing (miss) from WAL buffers. If we have read either partially
+ * or nothing, then continue to read the remaining bytes the usual
+ * way, that is, read from WAL file.
+ *
+ * XXX: It might be worth to expose WAL buffer read stats.
+ */
+ if (nread == count)
+ return true; /* Buffer hit, so return. */
+ else if (nread < count)
+ {
+ /*
+ * Buffer partial hit, so reset the state to count the read bytes
+ * and continue.
+ */
+ buf += nread;
+ startptr += nread;
+ count -= nread;
+ }
+ }
+#endif
p = buf;
recptr = startptr;
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 43f7b31205..057c9b4ea0 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -1007,12 +1007,13 @@ read_local_xlog_page_guts(XLogReaderState *state, XLogRecPtr targetPagePtr,
}
/*
- * Even though we just determined how much of the page can be validly read
- * as 'count', read the whole page anyway. It's guaranteed to be
- * zero-padded up to the page boundary if it's incomplete.
+ * We determined how much of the page can be validly read as 'count', read
+ * that much only, not the entire page. Since WALRead() can read the page
+ * from WAL buffers, in which case, the page is not guaranteed to be
+ * zero-padded up to the page boundary because of the concurrent
+ * insertions.
*/
- if (!WALRead(state, cur_page, targetPagePtr, XLOG_BLCKSZ, tli,
- &errinfo))
+ if (!WALRead(state, cur_page, targetPagePtr, count, tli, &errinfo))
WALReadRaiseError(&errinfo);
/* number of valid bytes in the buffer */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 3bc9c82389..a00bcd30bf 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -943,11 +943,17 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
else
count = flushptr - targetPagePtr; /* part of the page available */
- /* now actually read the data, we know it's there */
+ /*
+ * We determined how much of the page can be validly read as 'count', read
+ * that much only, not the entire page. Since WALRead() can read the page
+ * from WAL buffers, in which case, the page is not guaranteed to be
+ * zero-padded up to the page boundary because of the concurrent
+ * insertions.
+ */
if (!WALRead(state,
cur_page,
targetPagePtr,
- XLOG_BLCKSZ,
+ count,
currTLI, /* Pass the current TLI because only
* WalSndSegmentOpen controls whether new TLI
* is needed. */
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index a14126d164..9035a12a7b 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -193,6 +193,23 @@ typedef enum WALAvailability
WALAVAIL_REMOVED, /* WAL segment has been removed */
} WALAvailability;
+/* Return values from XLogReadFromBuffers. */
+typedef enum XLogReadFromBuffersResult
+{
+ XLREADBUFS_OK = 0, /* no error */
+ XLREADBUFS_INVALID_INPUT = -1, /* invalid startptr */
+ XLREADBUFS_IN_RECOVERY = -2, /* read attempted when in recovery */
+
+ /* read attempted with TLI that's different from server insertion TLI */
+ XLREADBUFS_NOT_INSERT_TLI = -3,
+
+ /* read attempted for non-existent WAL */
+ XLREADBUFS_NON_EXISTENT_WAL = -4,
+
+ /* uninitialized WAL buffer page */
+ XLREADBUFS_UNINITIALIZED_WAL = -5
+} XLogReadFromBuffersResult;
+
struct XLogRecData;
struct XLogReaderState;
@@ -251,6 +268,12 @@ extern XLogRecPtr GetLastImportantRecPtr(void);
extern void SetWalWriterSleeping(bool sleeping);
+extern XLogReadFromBuffersResult XLogReadFromBuffers(XLogRecPtr startptr,
+ TimeLineID tli,
+ Size bytes_to_read,
+ char *buf,
+ Size *bytes_read);
+
/*
* Routines used by xlogrecovery.c to call back into xlog.c during recovery.
*/
--
2.34.1
[application/octet-stream] v17-0003-Add-test-module-for-verifying-WAL-read-from-WAL-.patch (9.6K, ../CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@mail.gmail.com/4-v17-0003-Add-test-module-for-verifying-WAL-read-from-WAL-.patch)
download | inline diff:
From be13735c7fc641d1160d85ad9069c404b94adc5a Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 7 Dec 2023 08:27:41 +0000
Subject: [PATCH v17] Add test module for verifying WAL read from WAL buffers
This commit adds a test module to verify WAL read from WAL
buffers.
Author: Bharath Rupireddy
Reviewed-by: Dilip Kumar
Discussion: https://www.postgresql.org/message-id/CALj2ACXKKK%3DwbiG5_t6dGao5GoecMwRkhr7GjVBM_jg54%2BNa%3DQ%40mail.gmail.com
---
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
.../test_wal_read_from_buffers/.gitignore | 4 ++
.../test_wal_read_from_buffers/Makefile | 23 ++++++++
.../test_wal_read_from_buffers/meson.build | 33 ++++++++++++
.../test_wal_read_from_buffers/t/001_basic.pl | 54 +++++++++++++++++++
.../test_wal_read_from_buffers--1.0.sql | 16 ++++++
.../test_wal_read_from_buffers.c | 46 ++++++++++++++++
.../test_wal_read_from_buffers.control | 4 ++
9 files changed, 182 insertions(+)
create mode 100644 src/test/modules/test_wal_read_from_buffers/.gitignore
create mode 100644 src/test/modules/test_wal_read_from_buffers/Makefile
create mode 100644 src/test/modules/test_wal_read_from_buffers/meson.build
create mode 100644 src/test/modules/test_wal_read_from_buffers/t/001_basic.pl
create mode 100644 src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers--1.0.sql
create mode 100644 src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.c
create mode 100644 src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..64a051ce1c 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -33,6 +33,7 @@ SUBDIRS = \
test_rls_hooks \
test_shm_mq \
test_slru \
+ test_wal_read_from_buffers \
unsafe_tests \
worker_spi \
xid_wraparound
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index b76f588559..52b0cd5812 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -30,6 +30,7 @@ subdir('test_resowner')
subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
+subdir('test_wal_read_from_buffers')
subdir('unsafe_tests')
subdir('worker_spi')
subdir('xid_wraparound')
diff --git a/src/test/modules/test_wal_read_from_buffers/.gitignore b/src/test/modules/test_wal_read_from_buffers/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_wal_read_from_buffers/Makefile b/src/test/modules/test_wal_read_from_buffers/Makefile
new file mode 100644
index 0000000000..7472494501
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_wal_read_from_buffers/Makefile
+
+MODULE_big = test_wal_read_from_buffers
+OBJS = \
+ $(WIN32RES) \
+ test_wal_read_from_buffers.o
+PGFILEDESC = "test_wal_read_from_buffers - test module to read WAL from WAL buffers"
+
+EXTENSION = test_wal_read_from_buffers
+DATA = test_wal_read_from_buffers--1.0.sql
+
+TAP_TESTS = 1
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_wal_read_from_buffers
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_wal_read_from_buffers/meson.build b/src/test/modules/test_wal_read_from_buffers/meson.build
new file mode 100644
index 0000000000..40bd5dcd33
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+test_wal_read_from_buffers_sources = files(
+ 'test_wal_read_from_buffers.c',
+)
+
+if host_system == 'windows'
+ test_wal_read_from_buffers_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+ '--NAME', 'test_wal_read_from_buffers',
+ '--FILEDESC', 'test_wal_read_from_buffers - test module to read WAL from WAL buffers',])
+endif
+
+test_wal_read_from_buffers = shared_module('test_wal_read_from_buffers',
+ test_wal_read_from_buffers_sources,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_wal_read_from_buffers
+
+test_install_data += files(
+ 'test_wal_read_from_buffers.control',
+ 'test_wal_read_from_buffers--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_wal_read_from_buffers',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'tap': {
+ 'tests': [
+ 't/001_basic.pl',
+ ],
+ },
+}
diff --git a/src/test/modules/test_wal_read_from_buffers/t/001_basic.pl b/src/test/modules/test_wal_read_from_buffers/t/001_basic.pl
new file mode 100644
index 0000000000..1d842bb02e
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/t/001_basic.pl
@@ -0,0 +1,54 @@
+# Copyright (c) 2021-2023, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('test');
+
+$node->init;
+
+# Ensure nobody interferes with us so that the WAL in WAL buffers don't get
+# overwritten while running tests.
+$node->append_conf(
+ 'postgresql.conf', qq(
+autovacuum = off
+checkpoint_timeout = 1h
+wal_writer_delay = 10000ms
+wal_writer_flush_after = 1GB
+));
+$node->start;
+
+# Setup.
+$node->safe_psql('postgres', 'CREATE EXTENSION test_wal_read_from_buffers;');
+
+# Get current insert LSN. After this, we generate some WAL which is guranteed
+# to be in WAL buffers as there is no other WAL generating activity is
+# happening on the server. We then verify if we can read the WAL from WAL
+# buffers using this LSN.
+my $lsn = $node->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
+
+# Generate minimal WAL so that WAL buffers don't get overwritten.
+$node->safe_psql('postgres',
+ "CREATE TABLE t (c int); INSERT INTO t VALUES (1);");
+
+# Check if WAL is successfully read from WAL buffers.
+my $result = $node->safe_psql('postgres',
+ qq{SELECT test_wal_read_from_buffers('$lsn');});
+is($result, 't', "WAL is successfully read from WAL buffers");
+
+# Check with a WAL that doesn't yet exist.
+$lsn = $node->safe_psql('postgres', 'SELECT pg_current_wal_flush_lsn()+8192;');
+$result = $node->safe_psql('postgres',
+ qq{SELECT test_wal_read_from_buffers('$lsn');});
+is($result, 'f', "WAL that doesn't yet exist is not read from WAL buffers");
+
+# Check with invalid input.
+$result = $node->safe_psql('postgres',
+ qq{SELECT test_wal_read_from_buffers('0/0');});
+is($result, 'f', "WAL is not read from WAL buffers with invalid input");
+
+done_testing();
diff --git a/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers--1.0.sql b/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers--1.0.sql
new file mode 100644
index 0000000000..c6ffb3fa65
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers--1.0.sql
@@ -0,0 +1,16 @@
+/* src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_wal_read_from_buffers" to load this file. \quit
+
+--
+-- test_wal_read_from_buffers()
+--
+-- Returns true if WAL data at a given LSN can be read from WAL buffers.
+-- Otherwise returns false.
+--
+CREATE FUNCTION test_wal_read_from_buffers(IN lsn pg_lsn,
+ read_successful OUT boolean
+)
+AS 'MODULE_PATHNAME', 'test_wal_read_from_buffers'
+LANGUAGE C STRICT;
diff --git a/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.c b/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.c
new file mode 100644
index 0000000000..aff609ead7
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.c
@@ -0,0 +1,46 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_wal_read_from_buffers.c
+ * Test module to read WAL from WAL buffers.
+ *
+ * Portions Copyright (c) 2023, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.c
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "access/xlog.h"
+#include "fmgr.h"
+#include "utils/pg_lsn.h"
+
+PG_MODULE_MAGIC;
+
+/*
+ * SQL function for verifying that WAL data at a given LSN can be read from WAL
+ * buffers. Returns true if read from WAL buffers, otherwise false.
+ */
+PG_FUNCTION_INFO_V1(test_wal_read_from_buffers);
+Datum
+test_wal_read_from_buffers(PG_FUNCTION_ARGS)
+{
+ char data[XLOG_BLCKSZ] = {0};
+ Size nread;
+ XLogReadFromBuffersResult result;
+ bool is_read;
+
+ result = XLogReadFromBuffers(PG_GETARG_LSN(0),
+ GetWALInsertionTimeLine(),
+ XLOG_BLCKSZ,
+ data,
+ &nread);
+
+ if (nread > 0)
+ is_read = true;
+ else
+ is_read = false;
+
+ PG_RETURN_BOOL(is_read);
+}
diff --git a/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.control b/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.control
new file mode 100644
index 0000000000..eda8d47954
--- /dev/null
+++ b/src/test/modules/test_wal_read_from_buffers/test_wal_read_from_buffers.control
@@ -0,0 +1,4 @@
+comment = 'Test module to read WAL from WAL buffers'
+default_version = '1.0'
+module_pathname = '$libdir/test_wal_read_from_buffers'
+relocatable = true
--
2.34.1
view thread (26+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Improve WALRead() to suck data directly from WAL buffers when possible
In-Reply-To: <CALj2ACVfFMfqD5oLzZSQQZWfXiJqd-NdX0_317veP6FuB31QWA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox