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
From 828e798d2a4d42aa901f4c02e9a0c76ebb057c41 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davy...@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
