Accessing an invalid pointer in BufferManagerRelation structure
Started by Daniil Davydov12 months ago8 messages
Hi,
Postgres allows us to pass BufferManagerRelation structure to
functions in two ways : BMR_REL and BMR_SMGR.
In case we use BMR_REL, the "smgr" field of structure initialized this way :
***
if (bmr.smgr == NULL)
{
bmr.smgr = RelationGetSmgr(bmr.rel);
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
}
***
Thus, we set the "smgr" field only once. But in case of frequent cache
invalidation (for example with debug_discard_caches parameter
enabled),
this pointer may become invalid (because RelationCloseSmgr will be called).
I have not found any places in the current code where this could
happen. But if (just for example) we add acquiring of new lock into
ExtendBufferedRelLocal
or ExtendBufferedRelShared, relation cache will be invalidated (inside
AcceptInvalidationMessages).
I would suggest adding a special macro to access the "smgr" field
(check attached patch for REL_17_STABLE).
What do you think about this?
--
Best regards,
Daniil Davydov
Attachments:
0001-Add-marcos-for-safety-access-to-smgr-field-of-Buffer.patchtext/x-patch; charset=US-ASCII; name=0001-Add-marcos-for-safety-access-to-smgr-field-of-Buffer.patchDownload
From 828e798d2a4d42aa901f4c02e9a0c76ebb057c41 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Mon, 27 Jan 2025 18:36:10 +0700
Subject: [PATCH] Add marcos for safety access to smgr field of
BufferManagerRelation structure
---
src/backend/storage/buffer/bufmgr.c | 54 ++++++++++++---------------
src/backend/storage/buffer/localbuf.c | 8 ++--
src/include/storage/bufmgr.h | 13 +++++++
3 files changed, 41 insertions(+), 34 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 6181673095..d2c9297edb 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -886,11 +886,8 @@ ExtendBufferedRelBy(BufferManagerRelation bmr,
Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
Assert(extend_by > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == 0)
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
return ExtendBufferedRelCommon(bmr, fork, strategy, flags,
extend_by, InvalidBlockNumber,
@@ -922,11 +919,8 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == 0)
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
/*
* If desired, create the file if it doesn't exist. If
@@ -934,15 +928,15 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* an smgrexists call.
*/
if ((flags & EB_CREATE_FORK_IF_NEEDED) &&
- (bmr.smgr->smgr_cached_nblocks[fork] == 0 ||
- bmr.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
- !smgrexists(bmr.smgr, fork))
+ (BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == 0 ||
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
+ !smgrexists(BMR_GET_SMGR(bmr), fork))
{
LockRelationForExtension(bmr.rel, ExclusiveLock);
/* recheck, fork might have been created concurrently */
- if (!smgrexists(bmr.smgr, fork))
- smgrcreate(bmr.smgr, fork, flags & EB_PERFORMING_RECOVERY);
+ if (!smgrexists(BMR_GET_SMGR(bmr), fork))
+ smgrcreate(BMR_GET_SMGR(bmr), fork, flags & EB_PERFORMING_RECOVERY);
UnlockRelationForExtension(bmr.rel, ExclusiveLock);
}
@@ -952,13 +946,13 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
/*
* Estimate how many pages we'll need to extend by. This avoids acquiring
* unnecessarily many victim buffers.
*/
- current_size = smgrnblocks(bmr.smgr, fork);
+ current_size = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Since no-one else can be looking at the page contents yet, there is no
@@ -1002,7 +996,7 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
if (buffer == InvalidBuffer)
{
Assert(extended_by == 0);
- buffer = ReadBuffer_common(bmr.rel, bmr.smgr, 0,
+ buffer = ReadBuffer_common(bmr.rel, BMR_GET_SMGR(bmr), 0,
fork, extend_to - 1, mode, strategy);
}
@@ -2144,10 +2138,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
BlockNumber first_block;
TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
extend_by);
if (bmr.relpersistence == RELPERSISTENCE_TEMP)
@@ -2161,10 +2155,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
*extended_by = extend_by;
TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
*extended_by,
first_block);
@@ -2230,9 +2224,9 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Now that we have the accurate relation size, check if the caller wants
@@ -2275,7 +2269,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork),
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork),
MaxBlockNumber)));
/*
@@ -2297,7 +2291,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ResourceOwnerEnlarge(CurrentResourceOwner);
ReservePrivateRefCountEntry();
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork, first_block + i);
hash = BufTableHashCode(&tag);
partition_lock = BufMappingPartitionLock(hash);
@@ -2346,7 +2340,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
if (valid && !PageIsNew((Page) buf_block))
ereport(ERROR,
(errmsg("unexpected data beyond EOF in block %u of relation %s",
- existing_hdr->tag.blockNum, relpath(bmr.smgr->smgr_rlocator, fork)),
+ existing_hdr->tag.blockNum, relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork)),
errhint("This has been seen to occur with buggy kernels; consider updating your system.")));
/*
@@ -2404,7 +2398,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
*
* We don't need to set checksum for all-zero pages.
*/
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
/*
* Release the file-extension lock; it's now OK for someone else to extend
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 8da7dd6c98..237785f115 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -340,7 +340,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
MemSet((char *) buf_block, 0, BLCKSZ);
}
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
if (extend_upto != InvalidBlockNumber)
{
@@ -359,7 +359,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork),
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork),
MaxBlockNumber)));
for (uint32 i = 0; i < extend_by; i++)
@@ -376,7 +376,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
/* in case we need to pin an existing buffer below */
ResourceOwnerEnlarge(CurrentResourceOwner);
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork, first_block + i);
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, (void *) &tag, HASH_ENTER, &found);
@@ -416,7 +416,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
io_start = pgstat_prepare_io_time(track_io_timing);
/* actually extend relation */
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
io_start, extend_by);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index a1e71013d3..b57c82cff2 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -100,13 +100,26 @@ typedef enum ExtendBufferedFlags
typedef struct BufferManagerRelation
{
Relation rel;
+
+ /*
+ * Must be accessed via BMR_GET_SMGR if we want to access some fields
+ * of structure
+ */
struct SMgrRelationData *smgr;
+
char relpersistence;
} BufferManagerRelation;
#define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel})
#define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence})
+/*
+ * In case of cache invalidation, we need to be sure that we access a valid
+ * smgr reference
+ */
+#define BMR_GET_SMGR(bmr) \
+ (RelationIsValid((bmr).rel) ? RelationGetSmgr((bmr).rel) : (bmr).smgr)
+
/* Zero out page if reading fails. */
#define READ_BUFFERS_ZERO_ON_ERROR (1 << 0)
/* Call smgrprefetch() if I/O necessary. */
--
2.43.0
Re: Accessing an invalid pointer in BufferManagerRelation structure
Hi,
I am posting the new v2 patch, which is now rebased on the `master` branch.
Waiting for your feedback :)
--
Best regards,
Daniil Davydov
Attachments:
v2-0001-Add-macros-for-safety-access-to-smgr.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Add-macros-for-safety-access-to-smgr.patchDownload
From 2e43a1411ebcb37b2c0c1d6bac758e48799d2c4e Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Tue, 11 Mar 2025 17:11:16 +0700
Subject: [PATCH v2] Add macros for safety access to smgr
---
src/backend/storage/buffer/bufmgr.c | 55 ++++++++++++---------------
src/backend/storage/buffer/localbuf.c | 10 +++--
src/include/storage/bufmgr.h | 14 +++++++
3 files changed, 45 insertions(+), 34 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 7915ed624c1..c55228f298a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -887,11 +887,8 @@ ExtendBufferedRelBy(BufferManagerRelation bmr,
Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
Assert(extend_by > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == 0)
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
return ExtendBufferedRelCommon(bmr, fork, strategy, flags,
extend_by, InvalidBlockNumber,
@@ -923,11 +920,8 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == 0)
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
/*
* If desired, create the file if it doesn't exist. If
@@ -935,15 +929,15 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* an smgrexists call.
*/
if ((flags & EB_CREATE_FORK_IF_NEEDED) &&
- (bmr.smgr->smgr_cached_nblocks[fork] == 0 ||
- bmr.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
- !smgrexists(bmr.smgr, fork))
+ (BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == 0 ||
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
+ !smgrexists(BMR_GET_SMGR(bmr), fork))
{
LockRelationForExtension(bmr.rel, ExclusiveLock);
/* recheck, fork might have been created concurrently */
- if (!smgrexists(bmr.smgr, fork))
- smgrcreate(bmr.smgr, fork, flags & EB_PERFORMING_RECOVERY);
+ if (!smgrexists(BMR_GET_SMGR(bmr), fork))
+ smgrcreate(BMR_GET_SMGR(bmr), fork, flags & EB_PERFORMING_RECOVERY);
UnlockRelationForExtension(bmr.rel, ExclusiveLock);
}
@@ -953,13 +947,13 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
/*
* Estimate how many pages we'll need to extend by. This avoids acquiring
* unnecessarily many victim buffers.
*/
- current_size = smgrnblocks(bmr.smgr, fork);
+ current_size = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Since no-one else can be looking at the page contents yet, there is no
@@ -1003,7 +997,7 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
if (buffer == InvalidBuffer)
{
Assert(extended_by == 0);
- buffer = ReadBuffer_common(bmr.rel, bmr.smgr, bmr.relpersistence,
+ buffer = ReadBuffer_common(bmr.rel, BMR_GET_SMGR(bmr), bmr.relpersistence,
fork, extend_to - 1, mode, strategy);
}
@@ -2153,10 +2147,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
BlockNumber first_block;
TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
extend_by);
if (bmr.relpersistence == RELPERSISTENCE_TEMP)
@@ -2170,10 +2164,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
*extended_by = extend_by;
TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
*extended_by,
first_block);
@@ -2239,9 +2233,9 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Now that we have the accurate relation size, check if the caller wants
@@ -2284,7 +2278,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork).str,
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str,
MaxBlockNumber)));
/*
@@ -2306,7 +2300,8 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ResourceOwnerEnlarge(CurrentResourceOwner);
ReservePrivateRefCountEntry();
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork,
+ first_block + i);
hash = BufTableHashCode(&tag);
partition_lock = BufMappingPartitionLock(hash);
@@ -2356,7 +2351,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errmsg("unexpected data beyond EOF in block %u of relation %s",
existing_hdr->tag.blockNum,
- relpath(bmr.smgr->smgr_rlocator, fork).str),
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str),
errhint("This has been seen to occur with buggy kernels; consider updating your system.")));
/*
@@ -2414,7 +2409,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
*
* We don't need to set checksum for all-zero pages.
*/
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
/*
* Release the file-extension lock; it's now OK for someone else to extend
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 80b83444eb2..0b03920c17f 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -24,6 +24,7 @@
#include "utils/guc_hooks.h"
#include "utils/memutils.h"
#include "utils/resowner.h"
+#include "utils/rel.h"
/*#define LBDEBUG*/
@@ -341,7 +342,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
MemSet(buf_block, 0, BLCKSZ);
}
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
if (extend_upto != InvalidBlockNumber)
{
@@ -360,7 +361,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork).str,
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str,
MaxBlockNumber)));
for (uint32 i = 0; i < extend_by; i++)
@@ -377,7 +378,8 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
/* in case we need to pin an existing buffer below */
ResourceOwnerEnlarge(CurrentResourceOwner);
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork,
+ first_block + i);
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, &tag, HASH_ENTER, &found);
@@ -417,7 +419,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
io_start = pgstat_prepare_io_time(track_io_timing);
/* actually extend relation */
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
io_start, 1, extend_by * BLCKSZ);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 7c1e4316dde..3f4228f88dc 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -100,13 +100,27 @@ typedef enum ExtendBufferedFlags
typedef struct BufferManagerRelation
{
Relation rel;
+
+ /*
+ * Must be accessed via BMR_GET_SMGR if we want to access some fields
+ * of structure
+ */
struct SMgrRelationData *smgr;
+
char relpersistence;
} BufferManagerRelation;
#define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel})
#define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence})
+/*
+ * In case of cache invalidation, we need to be sure that we access a valid
+ * smgr reference
+ */
+
+#define BMR_GET_SMGR(bmr) \
+ (RelationIsValid((bmr).rel) ? RelationGetSmgr((bmr).rel) : (bmr).smgr)
+
/* Zero out page if reading fails. */
#define READ_BUFFERS_ZERO_ON_ERROR (1 << 0)
/* Call smgrprefetch() if I/O necessary. */
--
2.43.0
Re: Accessing an invalid pointer in BufferManagerRelation structure
Hello,
Dmitriy Bondar and I decided to review this patch as part of the CommitFest
Review.
The first thing we both noticed is that the macro calls a function that
won't be available without an additional header. This seems a bit
inconvenient.
I also have a question: is the logic correct that if the relation is valid,
we should fetch it rather than the other way around? Additionally, is
checking only the `rd_isvalid` flag sufficient, or should we also consider
the flag below?
```
bool rd_isvalid; /* relcache entry is valid */
```
Other than that, the tests pass, and there are no issues with code style.
Thanks for the patch - it could indeed help prevent future issues.
Best regards,
Stepan Neretin
вт, 11 мар. 2025 г., 17:32 Daniil Davydov <3danissimo@gmail.com>:
Show quoted text
Hi,
I am posting the new v2 patch, which is now rebased on the `master` branch.
Waiting for your feedback :)--
Best regards,
Daniil Davydov
Re: Accessing an invalid pointer in BufferManagerRelation structure
Hi,
On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf@gmail.com> wrote:
The first thing we both noticed is that the macro calls a function that won't be available without an additional header. This seems a bit inconvenient.
Well, I rebased the patch onto the latest `master`
(b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't
need to include `rel.h` in `localbuf.c` directly anymore, because
`#include lmgr.h` was added in memutils.h
I guess it solves this issue. Please, see v3 patch.
I also have a question: is the logic correct that if the relation is valid, we should fetch it rather than the other way around? Additionally, is checking only the `rd_isvalid` flag sufficient, or should we also consider the flag below?
```
bool rd_isvalid; /* relcache entry is valid */
I don't think that we should check any Relation's flags here. We are
checking `RelationIsValid((bmr).rel) ?` to decide whether
BufferManagerRelation was created via BMR_REL or BMR_SMGR.
If the `rel` field is not NULL, we can definitely say that BMR_REL was
used, so we should call RelationGetSmgr in order to access smgr.
--
Best regards,
Daniil Davydov
Attachments:
v3-0001-Add-macros-for-safety-access-to-smgr.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Add-macros-for-safety-access-to-smgr.patchDownload
From a1c572652e69f13b954ec3a915ed55c1ec11c772 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Mon, 14 Apr 2025 13:07:38 +0700
Subject: [PATCH v3] Add macros for safety access to smgr
---
src/backend/storage/buffer/bufmgr.c | 55 ++++++++++++---------------
src/backend/storage/buffer/localbuf.c | 9 +++--
src/include/storage/bufmgr.h | 13 +++++++
3 files changed, 43 insertions(+), 34 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 0b0e056eea2..f2b117088a8 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -892,11 +892,8 @@ ExtendBufferedRelBy(BufferManagerRelation bmr,
Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
Assert(extend_by > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == 0)
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
return ExtendBufferedRelCommon(bmr, fork, strategy, flags,
extend_by, InvalidBlockNumber,
@@ -928,11 +925,8 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == 0)
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
/*
* If desired, create the file if it doesn't exist. If
@@ -940,15 +934,15 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* an smgrexists call.
*/
if ((flags & EB_CREATE_FORK_IF_NEEDED) &&
- (bmr.smgr->smgr_cached_nblocks[fork] == 0 ||
- bmr.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
- !smgrexists(bmr.smgr, fork))
+ (BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == 0 ||
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
+ !smgrexists(BMR_GET_SMGR(bmr), fork))
{
LockRelationForExtension(bmr.rel, ExclusiveLock);
/* recheck, fork might have been created concurrently */
- if (!smgrexists(bmr.smgr, fork))
- smgrcreate(bmr.smgr, fork, flags & EB_PERFORMING_RECOVERY);
+ if (!smgrexists(BMR_GET_SMGR(bmr), fork))
+ smgrcreate(BMR_GET_SMGR(bmr), fork, flags & EB_PERFORMING_RECOVERY);
UnlockRelationForExtension(bmr.rel, ExclusiveLock);
}
@@ -958,13 +952,13 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
/*
* Estimate how many pages we'll need to extend by. This avoids acquiring
* unnecessarily many victim buffers.
*/
- current_size = smgrnblocks(bmr.smgr, fork);
+ current_size = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Since no-one else can be looking at the page contents yet, there is no
@@ -1008,7 +1002,7 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
if (buffer == InvalidBuffer)
{
Assert(extended_by == 0);
- buffer = ReadBuffer_common(bmr.rel, bmr.smgr, bmr.relpersistence,
+ buffer = ReadBuffer_common(bmr.rel, BMR_GET_SMGR(bmr), bmr.relpersistence,
fork, extend_to - 1, mode, strategy);
}
@@ -2568,10 +2562,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
BlockNumber first_block;
TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
extend_by);
if (bmr.relpersistence == RELPERSISTENCE_TEMP)
@@ -2585,10 +2579,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
*extended_by = extend_by;
TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
*extended_by,
first_block);
@@ -2654,9 +2648,9 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Now that we have the accurate relation size, check if the caller wants
@@ -2699,7 +2693,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork).str,
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str,
MaxBlockNumber)));
/*
@@ -2721,7 +2715,8 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ResourceOwnerEnlarge(CurrentResourceOwner);
ReservePrivateRefCountEntry();
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork,
+ first_block + i);
hash = BufTableHashCode(&tag);
partition_lock = BufMappingPartitionLock(hash);
@@ -2771,7 +2766,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errmsg("unexpected data beyond EOF in block %u of relation %s",
existing_hdr->tag.blockNum,
- relpath(bmr.smgr->smgr_rlocator, fork).str),
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str),
errhint("This has been seen to occur with buggy kernels; consider updating your system.")));
/*
@@ -2829,7 +2824,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
*
* We don't need to set checksum for all-zero pages.
*/
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
/*
* Release the file-extension lock; it's now OK for someone else to extend
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 63101d56a07..a3bcc4e22b3 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -372,7 +372,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
MemSet(buf_block, 0, BLCKSZ);
}
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
if (extend_upto != InvalidBlockNumber)
{
@@ -391,7 +391,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork).str,
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str,
MaxBlockNumber)));
for (uint32 i = 0; i < extend_by; i++)
@@ -408,7 +408,8 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
/* in case we need to pin an existing buffer below */
ResourceOwnerEnlarge(CurrentResourceOwner);
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork,
+ first_block + i);
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, &tag, HASH_ENTER, &found);
@@ -456,7 +457,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
io_start = pgstat_prepare_io_time(track_io_timing);
/* actually extend relation */
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
io_start, 1, extend_by * BLCKSZ);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 33a8b8c06fb..880cb09a7a4 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -101,13 +101,26 @@ typedef enum ExtendBufferedFlags
typedef struct BufferManagerRelation
{
Relation rel;
+
+ /*
+ * Must be accessed via BMR_GET_SMGR if we want to access some fields
+ * of structure
+ */
struct SMgrRelationData *smgr;
+
char relpersistence;
} BufferManagerRelation;
#define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel})
#define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence})
+/*
+ * In case of cache invalidation, we need to be sure that we access a valid
+ * smgr reference
+ */
+#define BMR_GET_SMGR(bmr) \
+ (RelationIsValid((bmr).rel) ? RelationGetSmgr((bmr).rel) : (bmr).smgr)
+
/* Zero out page if reading fails. */
#define READ_BUFFERS_ZERO_ON_ERROR (1 << 0)
/* Call smgrprefetch() if I/O necessary. */
--
2.43.0
Re: Accessing an invalid pointer in BufferManagerRelation structure
On Mon, Apr 14, 2025 at 1:10 PM Daniil Davydov <3danissimo@gmail.com> wrote:
Hi,
On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf@gmail.com> wrote:
The first thing we both noticed is that the macro calls a function that
won't be available without an additional header. This seems a bit
inconvenient.Well, I rebased the patch onto the latest `master`
(b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't
need to include `rel.h` in `localbuf.c` directly anymore, because
`#include lmgr.h` was added in memutils.h
I guess it solves this issue. Please, see v3 patch.I also have a question: is the logic correct that if the relation is
valid, we should fetch it rather than the other way around? Additionally,
is checking only the `rd_isvalid` flag sufficient, or should we also
consider the flag below?```
bool rd_isvalid; /* relcache entry is valid */I don't think that we should check any Relation's flags here. We are
checking `RelationIsValid((bmr).rel) ?` to decide whether
BufferManagerRelation was created via BMR_REL or BMR_SMGR.
If the `rel` field is not NULL, we can definitely say that BMR_REL was
used, so we should call RelationGetSmgr in order to access smgr.--
Best regards,
Daniil Davydov
Hi, now looks good for me.
--
Best regards,
Stepan Neretin
Re: Accessing an invalid pointer in BufferManagerRelation structure
On 2025-Apr-14, Daniil Davydov wrote:
On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slpmcf@gmail.com> wrote:
The first thing we both noticed is that the macro calls a function
that won't be available without an additional header. This seems a
bit inconvenient.
I think this is a fact of life. We don't have an automated check for
such patterns anymore, so the users of the macro will have to include
the other file. In any case I would rather they include utils/rel.h
themselves, than forcing rel.h into every single file that includes
bufmgr.h.
Well, I rebased the patch onto the latest `master`
(b51f86e49a7f119004c0ce5d0be89cdf98309141) and noticed that we don't
need to include `rel.h` in `localbuf.c` directly anymore, because
`#include lmgr.h` was added in memutils.h
I guess it solves this issue. Please, see v3 patch.
Not anymore.
I edited your comments a bit. What do you think of this version?
I don't think this is backpatchable material, mainly because you said
you don't see any situations in which relcache invalidations would
occur in the middle of this. Can you can cause a problem to occur by
adding an untimely invalidation somewhere without this patch, which is
fixed by this patch?
BTW how does this interact with SMgrRelation pinning?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees." (E. Dijkstra)
Attachments:
v4-0001-Make-smgr-access-in-BufferManagerRelation-safe-in.patchtext/x-diff; charset=utf-8Download
From 12ec4ff2a9e0bd6fb0510ac6308480c8d8cf6ef9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Sun, 19 Oct 2025 12:47:38 +0200
Subject: [PATCH v4] Make smgr access in BufferManagerRelation safe in relcache
inval
Author: Daniil Davydov <3danissimo@gmail.com>
Reviewed-by: Stepan Neretin <slpmcf@gmail.com>
Discussion: https://postgr.es/m/CAJDiXgj3FNzAhV+jjPqxMs3jz=OgPohsoXFj_fh-L+nS+13CKQ@mail.gmail.com
---
src/backend/storage/buffer/bufmgr.c | 59 ++++++++++++---------------
src/backend/storage/buffer/localbuf.c | 10 +++--
src/include/storage/bufmgr.h | 15 +++++--
3 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index edf17ce3ea1..e8544acb784 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -883,14 +883,11 @@ ExtendBufferedRelBy(BufferManagerRelation bmr,
uint32 *extended_by)
{
Assert((bmr.rel != NULL) != (bmr.smgr != NULL));
- Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
+ Assert(bmr.smgr == NULL || bmr.relpersistence != '\0');
Assert(extend_by > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == '\0')
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
return ExtendBufferedRelCommon(bmr, fork, strategy, flags,
extend_by, InvalidBlockNumber,
@@ -919,14 +916,11 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
Buffer buffers[64];
Assert((bmr.rel != NULL) != (bmr.smgr != NULL));
- Assert(bmr.smgr == NULL || bmr.relpersistence != 0);
+ Assert(bmr.smgr == NULL || bmr.relpersistence != '\0');
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
- if (bmr.smgr == NULL)
- {
- bmr.smgr = RelationGetSmgr(bmr.rel);
+ if (bmr.relpersistence == '\0')
bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
- }
/*
* If desired, create the file if it doesn't exist. If
@@ -934,15 +928,15 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* an smgrexists call.
*/
if ((flags & EB_CREATE_FORK_IF_NEEDED) &&
- (bmr.smgr->smgr_cached_nblocks[fork] == 0 ||
- bmr.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
- !smgrexists(bmr.smgr, fork))
+ (BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == 0 ||
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
+ !smgrexists(BMR_GET_SMGR(bmr), fork))
{
LockRelationForExtension(bmr.rel, ExclusiveLock);
/* recheck, fork might have been created concurrently */
- if (!smgrexists(bmr.smgr, fork))
- smgrcreate(bmr.smgr, fork, flags & EB_PERFORMING_RECOVERY);
+ if (!smgrexists(BMR_GET_SMGR(bmr), fork))
+ smgrcreate(BMR_GET_SMGR(bmr), fork, flags & EB_PERFORMING_RECOVERY);
UnlockRelationForExtension(bmr.rel, ExclusiveLock);
}
@@ -952,13 +946,13 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
/*
* Estimate how many pages we'll need to extend by. This avoids acquiring
* unnecessarily many victim buffers.
*/
- current_size = smgrnblocks(bmr.smgr, fork);
+ current_size = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Since no-one else can be looking at the page contents yet, there is no
@@ -1002,7 +996,7 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
if (buffer == InvalidBuffer)
{
Assert(extended_by == 0);
- buffer = ReadBuffer_common(bmr.rel, bmr.smgr, bmr.relpersistence,
+ buffer = ReadBuffer_common(bmr.rel, BMR_GET_SMGR(bmr), bmr.relpersistence,
fork, extend_to - 1, mode, strategy);
}
@@ -2540,10 +2534,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
BlockNumber first_block;
TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
extend_by);
if (bmr.relpersistence == RELPERSISTENCE_TEMP)
@@ -2557,10 +2551,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
*extended_by = extend_by;
TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,
- bmr.smgr->smgr_rlocator.locator.spcOid,
- bmr.smgr->smgr_rlocator.locator.dbOid,
- bmr.smgr->smgr_rlocator.locator.relNumber,
- bmr.smgr->smgr_rlocator.backend,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
+ BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
*extended_by,
first_block);
@@ -2626,9 +2620,9 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
* kernel.
*/
if (flags & EB_CLEAR_SIZE_CACHE)
- bmr.smgr->smgr_cached_nblocks[fork] = InvalidBlockNumber;
+ BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] = InvalidBlockNumber;
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
/*
* Now that we have the accurate relation size, check if the caller wants
@@ -2666,7 +2660,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork).str,
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str,
MaxBlockNumber)));
/*
@@ -2688,7 +2682,8 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ResourceOwnerEnlarge(CurrentResourceOwner);
ReservePrivateRefCountEntry();
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork,
+ first_block + i);
hash = BufTableHashCode(&tag);
partition_lock = BufMappingPartitionLock(hash);
@@ -2730,7 +2725,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR,
(errmsg("unexpected data beyond EOF in block %u of relation \"%s\"",
existing_hdr->tag.blockNum,
- relpath(bmr.smgr->smgr_rlocator, fork).str)));
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str)));
/*
* We *must* do smgr[zero]extend before succeeding, else the page
@@ -2787,7 +2782,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
*
* We don't need to set checksum for all-zero pages.
*/
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
/*
* Release the file-extension lock; it's now OK for someone else to extend
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 04fef13409b..15aac7d1c9f 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -25,6 +25,7 @@
#include "utils/guc_hooks.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
+#include "utils/rel.h"
#include "utils/resowner.h"
@@ -372,7 +373,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
MemSet(buf_block, 0, BLCKSZ);
}
- first_block = smgrnblocks(bmr.smgr, fork);
+ first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
if (extend_upto != InvalidBlockNumber)
{
@@ -391,7 +392,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks",
- relpath(bmr.smgr->smgr_rlocator, fork).str,
+ relpath(BMR_GET_SMGR(bmr)->smgr_rlocator, fork).str,
MaxBlockNumber)));
for (uint32 i = 0; i < extend_by; i++)
@@ -408,7 +409,8 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
/* in case we need to pin an existing buffer below */
ResourceOwnerEnlarge(CurrentResourceOwner);
- InitBufferTag(&tag, &bmr.smgr->smgr_rlocator.locator, fork, first_block + i);
+ InitBufferTag(&tag, &BMR_GET_SMGR(bmr)->smgr_rlocator.locator, fork,
+ first_block + i);
hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, &tag, HASH_ENTER, &found);
@@ -456,7 +458,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
io_start = pgstat_prepare_io_time(track_io_timing);
/* actually extend relation */
- smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
+ smgrzeroextend(BMR_GET_SMGR(bmr), fork, first_block, extend_by, false);
pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
io_start, 1, extend_by * BLCKSZ);
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 3f37b294af6..b5f8f3c5d42 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -98,8 +98,11 @@ typedef struct SMgrRelationData *SMgrRelation;
/*
* Some functions identify relations either by relation or smgr +
- * relpersistence. Used via the BMR_REL()/BMR_SMGR() macros below. This
- * allows us to use the same function for both recovery and normal operation.
+ * relpersistence, initialized via the BMR_REL()/BMR_SMGR() macros below.
+ * This allows us to use the same function for both recovery and normal
+ * operation. When BMR_REL is used, it's not valid to cache its rd_smgr here,
+ * because our pointer would be obsolete in case of relcache invalidation.
+ * For simplicity, use BMR_GET_SMGR to read the smgr.
*/
typedef struct BufferManagerRelation
{
@@ -108,8 +111,12 @@ typedef struct BufferManagerRelation
char relpersistence;
} BufferManagerRelation;
-#define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel})
-#define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence})
+#define BMR_REL(p_rel) \
+ ((BufferManagerRelation){.rel = p_rel})
+#define BMR_SMGR(p_smgr, p_relpersistence) \
+ ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence})
+#define BMR_GET_SMGR(bmr) \
+ (RelationIsValid((bmr).rel) ? RelationGetSmgr((bmr).rel) : (bmr).smgr)
/* Zero out page if reading fails. */
#define READ_BUFFERS_ZERO_ON_ERROR (1 << 0)
--
2.47.3
Re: Accessing an invalid pointer in BufferManagerRelation structure
Hi,
On Sun, Oct 19, 2025 at 6:32 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
I edited your comments a bit. What do you think of this version?
Thanks for looking into this! All added changes LGTM.
I don't think this is backpatchable material, mainly because you said
you don't see any situations in which relcache invalidations would
occur in the middle of this. Can you can cause a problem to occur by
adding an untimely invalidation somewhere without this patch, which is
fixed by this patch?
Yep, by now (if I did not miss anything) described problem cannot occur even
with debug_discard_caches parameter on. But one of our clients had faced
this problem, because we have some differences in source code. In the first
letter I described scenario, when the occurrence of a problem becomes possible :
***
if (just for example) we add acquiring of new lock into ExtendBufferedRelLocal
or ExtendBufferedRelShared, relation cache will be invalidated (inside
AcceptInvalidationMessages).
***
BTW how does this interact with SMgrRelation pinning?
Hm, maybe I don't fully understand the question. Smgr pinning is one of the side
effects of BMR_GET_SMGR (in case we use this macro with
invalidated/uninitialized
rd_smgr entry).
Without patch pincount cannot save us from undesirable invalidation,
because it guards
from smgr_destroy, not from smgr_release.
--
Best regards,
Daniil Davydov
Re: Accessing an invalid pointer in BufferManagerRelation structure
On 2025-Oct-21, Daniil Davydov wrote:
On Sun, Oct 19, 2025 at 6:32 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
I edited your comments a bit. What do you think of this version?
Thanks for looking into this! All added changes LGTM.
Okay, pushed, thanks.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/