ReadRecentBuffer() is broken for local buffer
ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables.
The bug is pretty clear if you look at the code:
if (BufferIsLocal(recent_buffer))
{
- bufHdr = GetBufferDescriptor(-recent_buffer - 1);
+ bufHdr = GetLocalBufferDescriptor(-recent_buffer - 1);The code after that looks suspicious, too. It increases the usage count
even if the buffer was already pinned. That's different from what it
does for a shared buffer, and different from LocalBufferAlloc(). That's
pretty harmless, just causes the usage count to be bumped more
frequently, but I don't think it was intentional. The ordering of
bumping the usage count, the local ref count, and registration in the
resource owner are different too. As far as I can see, that makes no
difference, but I think we should keep this code as close as possible to
similar code used elsewhere, unless there's a particular reason to differ.
I propose the attached to fix those things.
I tested this by adding this little snippet to a random place where we
have just read a page with ReadBuffer:
diff --git a/src/backend/access/heap/heapam.c
b/src/backend/access/heap/heapam.c
index aab8d6fa4e5..c4abdbc96dd 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -403,6 +403,14 @@ heapgetpage(TableScanDesc sscan, BlockNumber page) RBM_NORMAL, scan->rs_strategy);
scan->rs_cblock = page;
+ {
+ bool still_ok;
+
+ still_ok =
ReadRecentBuffer(scan->rs_base.rs_rd->rd_locator, MAIN_FORKNUM, page,
scan->rs_cbuf);
+ Assert(still_ok);
+ ReleaseBuffer(scan->rs_cbuf);
+ }
+
if (!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE))
return;Without the fix, the assertion is fails quickly on "make check".
- Heikki
Attachments:
0001-Fix-ReadRecentBuffer-for-local-buffers.patchtext/x-patch; charset=UTF-8; name=0001-Fix-ReadRecentBuffer-for-local-buffers.patchDownload
From 55140caaa6788ae07840948bef196d7ede927937 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Sun, 24 Jul 2022 21:12:52 +0300
Subject: [PATCH] Fix ReadRecentBuffer for local buffers.
It incorrectly used GetBufferDescriptor instead of
GetLocalBufferDescriptor, causing it to not find the correct buffer in
most cases, and performing an out-of-bounds memory read in the corner
case that temp_buffers > shared_buffers.
It also bumped the usage-count on the buffer, even if it was
previously pinned. That wont lead to crashes or incorrect results, but
it's different from what the shared-buffer case does, and different
from the usual code in LocalBufferAlloc. Fix that too, and make the
code ordering match LocalBufferAlloc() more closely, so that it's
easier to inspect it's doing the same thing.
Currently, ReadRecentBuffer() is only used with non-temp relations, in
WAL redo, so this broken code is currently dead code. However, it
could be used by extensions.
Backpatch-through: 14
Discussion: xxx
Reviewed-by: xxx
---
src/backend/storage/buffer/bufmgr.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index c7d7abcd738..b7488b5d89e 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -636,18 +636,28 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
if (BufferIsLocal(recent_buffer))
{
- bufHdr = GetBufferDescriptor(-recent_buffer - 1);
+ int b = -recent_buffer - 1;
+
+ bufHdr = GetLocalBufferDescriptor(b);
buf_state = pg_atomic_read_u32(&bufHdr->state);
/* Is it still valid and holding the right tag? */
if ((buf_state & BM_VALID) && BUFFERTAGS_EQUAL(tag, bufHdr->tag))
{
- /* Bump local buffer's ref and usage counts. */
+ /*
+ * Bump buffer's ref and usage counts. This is equivalent of
+ * PinBuffer for a shared buffer.
+ */
+ if (LocalRefCount[b] == 0)
+ {
+ if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
+ {
+ buf_state += BUF_USAGECOUNT_ONE;
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ }
+ }
+ LocalRefCount[b]++;
ResourceOwnerRememberBuffer(CurrentResourceOwner, recent_buffer);
- LocalRefCount[-recent_buffer - 1]++;
- if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
- pg_atomic_write_u32(&bufHdr->state,
- buf_state + BUF_USAGECOUNT_ONE);
pgBufferUsage.local_blks_hit++;
--
2.30.2
On Mon, Jul 25, 2022 at 6:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables.
The bug is pretty clear if you look at the code:
- bufHdr = GetBufferDescriptor(-recent_buffer - 1);
+ int b = -recent_buffer - 1;
+
+ bufHdr = GetLocalBufferDescriptor(b);Ugh, right. Obviously this code path is not reached currently. I
added the local path for completeness but I didn't think of the idea
of testing it the way you suggested, hence thinko escaped into the
wild. That way of testing seems good and the patch indeed fixes the
problem.
- /* Bump local buffer's ref and usage counts. */
+ /*
+ * Bump buffer's ref and usage counts. This is equivalent of
+ * PinBuffer for a shared buffer.
+ */
+ if (LocalRefCount[b] == 0)
+ {
+ if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
+ {
+ buf_state += BUF_USAGECOUNT_ONE;
+ pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
+ }
+ }
+ LocalRefCount[b]++;
ResourceOwnerRememberBuffer(CurrentResourceOwner, recent_buffer);
- LocalRefCount[-recent_buffer - 1]++;
- if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
- pg_atomic_write_u32(&bufHdr->state,
- buf_state + BUF_USAGECOUNT_ONE);+1, it makes sense to do it only if it wasn't pinned already, and it
really should look identical to the code in LocalBufferAlloc, and
perhaps the comment should even say so.
LGTM.
Nice catch, LGTM.
Show quoted text
On Jul 25, 2022, at 02:22, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables. The bug is pretty clear if you look at the code:
if (BufferIsLocal(recent_buffer)) { - bufHdr = GetBufferDescriptor(-recent_buffer - 1); + bufHdr = GetLocalBufferDescriptor(-recent_buffer - 1);The code after that looks suspicious, too. It increases the usage count even if the buffer was already pinned. That's different from what it does for a shared buffer, and different from LocalBufferAlloc(). That's pretty harmless, just causes the usage count to be bumped more frequently, but I don't think it was intentional. The ordering of bumping the usage count, the local ref count, and registration in the resource owner are different too. As far as I can see, that makes no difference, but I think we should keep this code as close as possible to similar code used elsewhere, unless there's a particular reason to differ.
I propose the attached to fix those things.
I tested this by adding this little snippet to a random place where we have just read a page with ReadBuffer:
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index aab8d6fa4e5..c4abdbc96dd 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -403,6 +403,14 @@ heapgetpage(TableScanDesc sscan, BlockNumber page) RBM_NORMAL, scan->rs_strategy); scan->rs_cblock = page;+ { + bool still_ok; + + still_ok = ReadRecentBuffer(scan->rs_base.rs_rd->rd_locator, MAIN_FORKNUM, page, scan->rs_cbuf); + Assert(still_ok); + ReleaseBuffer(scan->rs_cbuf); + } + if (!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE)) return;Without the fix, the assertion is fails quickly on "make check".
- Heikki<0001-Fix-ReadRecentBuffer-for-local-buffers.patch>
On Mon, Jul 25, 2022 at 2:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
if (BufferIsLocal(recent_buffer)) { - bufHdr = GetBufferDescriptor(-recent_buffer - 1); + bufHdr = GetLocalBufferDescriptor(-recent_buffer - 1);
Aha, we're using the wrong buffer descriptors here. Currently this
function is only called in XLogReadBufferExtended(), so the branch for
local buffer cannot be reached. Maybe that's why it is not identified
until now.
The code after that looks suspicious, too. It increases the usage count
even if the buffer was already pinned. That's different from what it
does for a shared buffer, and different from LocalBufferAlloc(). That's
pretty harmless, just causes the usage count to be bumped more
frequently, but I don't think it was intentional. The ordering of
bumping the usage count, the local ref count, and registration in the
resource owner are different too. As far as I can see, that makes no
difference, but I think we should keep this code as close as possible to
similar code used elsewhere, unless there's a particular reason to differ.
Agree. Maybe we can wrap the codes in an inline function or macro and
call that in both LocalBufferAlloc and here.
Thanks
Richard
On 25/07/2022 00:35, Thomas Munro wrote:
On Mon, Jul 25, 2022 at 6:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
ReadRecentBuffer() doesn't work for local buffers, i.e. for temp tables.
The bug is pretty clear if you look at the code:- bufHdr = GetBufferDescriptor(-recent_buffer - 1); + int b = -recent_buffer - 1; + + bufHdr = GetLocalBufferDescriptor(b);Ugh, right. Obviously this code path is not reached currently. I
added the local path for completeness but I didn't think of the idea
of testing it the way you suggested, hence thinko escaped into the
wild. That way of testing seems good and the patch indeed fixes the
problem.
Pushed, thanks for the reviews.
- Heikki