From 12ec4ff2a9e0bd6fb0510ac6308480c8d8cf6ef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= 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 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