Hi,
On Wed, Mar 26, 2025 at 2:14 PM Stepan Neretin <slp...@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
From a1c572652e69f13b954ec3a915ed55c1ec11c772 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davy...@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
