database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-10-25 13:17:41 +03:00
Code Activity

Make smgr access for a BufferManagerRelation safer in relcache inval

Browse Source 
Currently there's no bug, because we have no code path where we
invalidate relcache entries where it'd cause a problem.  But it's more
robust to do it this way in case we introduce such a path later, as some
Postgres forks reportedly already have.

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
This commit is contained in:
Álvaro Herrera
2025-10-21 10:51:55 +03:00
parent 9fd29d7ff4
commit b7cc6474e9
3 changed files with 44 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

59
src/backend/storage/buffer/bufmgr.c
View File

@@ -883,14 +883,11 @@ ExtendBufferedRelBy(BufferManagerRelation bmr,
uint32 *extended_by) uint32 *extended_by)
{ {
Assert((bmr.rel != NULL) != (bmr.smgr != NULL)); 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); Assert(extend_by > 0);
if (bmr.smgr == NULL) if (bmr.relpersistence == '\0')
{
bmr.smgr = RelationGetSmgr(bmr.rel);
bmr.relpersistence = bmr.rel->rd_rel->relpersistence; bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
}
return ExtendBufferedRelCommon(bmr, fork, strategy, flags, return ExtendBufferedRelCommon(bmr, fork, strategy, flags,
extend_by, InvalidBlockNumber, extend_by, InvalidBlockNumber,
@@ -919,14 +916,11 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
Buffer buffers[64]; Buffer buffers[64];
Assert((bmr.rel != NULL) != (bmr.smgr != NULL)); 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); Assert(extend_to != InvalidBlockNumber && extend_to > 0);
if (bmr.smgr == NULL) if (bmr.relpersistence == '\0')
{
bmr.smgr = RelationGetSmgr(bmr.rel);
bmr.relpersistence = bmr.rel->rd_rel->relpersistence; bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
}
/* /*
* If desired, create the file if it doesn't exist. If * If desired, create the file if it doesn't exist. If
@@ -934,15 +928,15 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* an smgrexists call. * an smgrexists call.
*/ */
if ((flags & EB_CREATE_FORK_IF_NEEDED) && if ((flags & EB_CREATE_FORK_IF_NEEDED) &&
(bmr.smgr->smgr_cached_nblocks[fork] == 0 || (BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == 0 ||
bmr.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber) && BMR_GET_SMGR(bmr)->smgr_cached_nblocks[fork] == InvalidBlockNumber) &&
!smgrexists(bmr.smgr, fork)) !smgrexists(BMR_GET_SMGR(bmr), fork))
{ {
LockRelationForExtension(bmr.rel, ExclusiveLock); LockRelationForExtension(bmr.rel, ExclusiveLock);
/* recheck, fork might have been created concurrently */ /* recheck, fork might have been created concurrently */
if (!smgrexists(bmr.smgr, fork)) if (!smgrexists(BMR_GET_SMGR(bmr), fork))
smgrcreate(bmr.smgr, fork, flags & EB_PERFORMING_RECOVERY); smgrcreate(BMR_GET_SMGR(bmr), fork, flags & EB_PERFORMING_RECOVERY);
UnlockRelationForExtension(bmr.rel, ExclusiveLock); UnlockRelationForExtension(bmr.rel, ExclusiveLock);
} }
@@ -952,13 +946,13 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
* kernel. * kernel.
*/ */
if (flags & EB_CLEAR_SIZE_CACHE) 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 * Estimate how many pages we'll need to extend by. This avoids acquiring
* unnecessarily many victim buffers. * 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 * 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) if (buffer == InvalidBuffer)
{ {
Assert(extended_by == 0); 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); fork, extend_to - 1, mode, strategy);
} }
@@ -2540,10 +2534,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
BlockNumber first_block; BlockNumber first_block;
TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork, TRACE_POSTGRESQL_BUFFER_EXTEND_START(fork,
bmr.smgr->smgr_rlocator.locator.spcOid, BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
bmr.smgr->smgr_rlocator.locator.dbOid, BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
bmr.smgr->smgr_rlocator.locator.relNumber, BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
bmr.smgr->smgr_rlocator.backend, BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
extend_by); extend_by);
if (bmr.relpersistence == RELPERSISTENCE_TEMP) if (bmr.relpersistence == RELPERSISTENCE_TEMP)
@@ -2557,10 +2551,10 @@ ExtendBufferedRelCommon(BufferManagerRelation bmr,
*extended_by = extend_by; *extended_by = extend_by;
TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork, TRACE_POSTGRESQL_BUFFER_EXTEND_DONE(fork,
bmr.smgr->smgr_rlocator.locator.spcOid, BMR_GET_SMGR(bmr)->smgr_rlocator.locator.spcOid,
bmr.smgr->smgr_rlocator.locator.dbOid, BMR_GET_SMGR(bmr)->smgr_rlocator.locator.dbOid,
bmr.smgr->smgr_rlocator.locator.relNumber, BMR_GET_SMGR(bmr)->smgr_rlocator.locator.relNumber,
bmr.smgr->smgr_rlocator.backend, BMR_GET_SMGR(bmr)->smgr_rlocator.backend,
*extended_by, *extended_by,
first_block); first_block);
@@ -2626,9 +2620,9 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
* kernel. * kernel.
*/ */
if (flags & EB_CLEAR_SIZE_CACHE) 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 * Now that we have the accurate relation size, check if the caller wants
@@ -2666,7 +2660,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks", 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))); MaxBlockNumber)));
/* /*
@@ -2688,7 +2682,8 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ResourceOwnerEnlarge(CurrentResourceOwner); ResourceOwnerEnlarge(CurrentResourceOwner);
ReservePrivateRefCountEntry(); 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); hash = BufTableHashCode(&tag);
partition_lock = BufMappingPartitionLock(hash); partition_lock = BufMappingPartitionLock(hash);
@@ -2730,7 +2725,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
ereport(ERROR, ereport(ERROR,
(errmsg("unexpected data beyond EOF in block %u of relation \"%s\"", (errmsg("unexpected data beyond EOF in block %u of relation \"%s\"",
existing_hdr->tag.blockNum, 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 * 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. * 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 * Release the file-extension lock; it's now OK for someone else to extend

10
src/backend/storage/buffer/localbuf.c
View File

@@ -25,6 +25,7 @@
#include "utils/guc_hooks.h" #include "utils/guc_hooks.h"
#include "utils/memdebug.h" #include "utils/memdebug.h"
#include "utils/memutils.h" #include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/resowner.h" #include "utils/resowner.h"
@@ -372,7 +373,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
MemSet(buf_block, 0, BLCKSZ); MemSet(buf_block, 0, BLCKSZ);
} }
first_block = smgrnblocks(bmr.smgr, fork); first_block = smgrnblocks(BMR_GET_SMGR(bmr), fork);
if (extend_upto != InvalidBlockNumber) if (extend_upto != InvalidBlockNumber)
{ {
@@ -391,7 +392,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("cannot extend relation %s beyond %u blocks", 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))); MaxBlockNumber)));
for (uint32 i = 0; i < extend_by; i++) 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 */ /* in case we need to pin an existing buffer below */
ResourceOwnerEnlarge(CurrentResourceOwner); 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 *) hresult = (LocalBufferLookupEnt *)
hash_search(LocalBufHash, &tag, HASH_ENTER, &found); hash_search(LocalBufHash, &tag, HASH_ENTER, &found);
@@ -456,7 +458,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
io_start = pgstat_prepare_io_time(track_io_timing); io_start = pgstat_prepare_io_time(track_io_timing);
/* actually extend relation */ /* 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, pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
io_start, 1, extend_by * BLCKSZ); io_start, 1, extend_by * BLCKSZ);

15
src/include/storage/bufmgr.h
View File

@@ -98,8 +98,11 @@ typedef struct SMgrRelationData *SMgrRelation;
/* /*
* Some functions identify relations either by relation or smgr + * Some functions identify relations either by relation or smgr +
* relpersistence. Used via the BMR_REL()/BMR_SMGR() macros below. This * relpersistence, initialized via the BMR_REL()/BMR_SMGR() macros below.
* allows us to use the same function for both recovery and normal operation. * 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 typedef struct BufferManagerRelation
{ {
@@ -108,8 +111,12 @@ typedef struct BufferManagerRelation
char relpersistence; char relpersistence;
} BufferManagerRelation; } BufferManagerRelation;
#define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel}) #define BMR_REL(p_rel) \
#define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence}) ((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. */ /* Zero out page if reading fails. */
#define READ_BUFFERS_ZERO_ON_ERROR (1 << 0) #define READ_BUFFERS_ZERO_ON_ERROR (1 << 0)