database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-08-25 20:23:07 +03:00
Code Activity

Replace RelationOpenSmgr() with RelationGetSmgr().

Browse Source 
The idea behind this patch is to design out bugs like the one fixed
by commit 9d523119f.  Previously, once one did RelationOpenSmgr(rel),
it was considered okay to access rel->rd_smgr directly for some
not-very-clear interval.  But since that pointer will be cleared by
relcache flushes, we had bugs arising from overreliance on a previous
RelationOpenSmgr call still being effective.

Now, very little code except that in rel.h and relcache.c should ever
touch the rd_smgr field directly.  The normal coding rule is to use
RelationGetSmgr(rel) and not expect the result to be valid for longer
than one smgr function call.  There are a couple of places where using
the function every single time seemed like overkill, but they are now
annotated with large warning comments.

Amul Sul, after an idea of mine.

Discussion: https://postgr.es/m/CANiYTQsU7yMFpQYnv=BrcRVqK_3U3mtAzAsJCaqtzsDHfsUbdQ@mail.gmail.com
This commit is contained in:
Tom Lane
2021-07-12 17:01:29 -04:00
parent 5b60cf35f5
commit f10f0ae420
21 changed files with 164 additions and 165 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -589,9 +589,6 @@ PrefetchBuffer(Relation reln, ForkNumber forkNum, BlockNumber blockNum)
Assert(RelationIsValid(reln));
Assert(BlockNumberIsValid(blockNum));
/* Open it at the smgr level if not already done */
RelationOpenSmgr(reln);
if (RelationUsesLocalBuffers(reln))
{
/* see comments in ReadBufferExtended */
@@ -601,12 +598,12 @@ PrefetchBuffer(Relation reln, ForkNumber forkNum, BlockNumber blockNum)
errmsg("cannot access temporary tables of other sessions")));
/* pass it off to localbuf.c */
return PrefetchLocalBuffer(reln->rd_smgr, forkNum, blockNum);
return PrefetchLocalBuffer(RelationGetSmgr(reln), forkNum, blockNum);
}
else
{
/* pass it to the shared buffer version */
return PrefetchSharedBuffer(reln->rd_smgr, forkNum, blockNum);
return PrefetchSharedBuffer(RelationGetSmgr(reln), forkNum, blockNum);
}
}
@@ -747,9 +744,6 @@ ReadBufferExtended(Relation reln, ForkNumber forkNum, BlockNumber blockNum,
bool hit;
Buffer buf;
/* Open it at the smgr level if not already done */
RelationOpenSmgr(reln);
/*
* Reject attempts to read non-local temporary relations; we would be
* likely to get wrong data since we have no visibility into the owning
@@ -765,7 +759,7 @@ ReadBufferExtended(Relation reln, ForkNumber forkNum, BlockNumber blockNum,
* miss.
*/
pgstat_count_buffer_read(reln);
buf = ReadBuffer_common(reln->rd_smgr, reln->rd_rel->relpersistence,
buf = ReadBuffer_common(RelationGetSmgr(reln), reln->rd_rel->relpersistence,
forkNum, blockNum, mode, strategy, &hit);
if (hit)
pgstat_count_buffer_hit(reln);
@@ -2949,10 +2943,7 @@ RelationGetNumberOfBlocksInFork(Relation relation, ForkNumber forkNum)
case RELKIND_SEQUENCE:
case RELKIND_INDEX:
case RELKIND_PARTITIONED_INDEX:
/* Open it at the smgr level if not already done */
RelationOpenSmgr(relation);
return smgrnblocks(relation->rd_smgr, forkNum);
return smgrnblocks(RelationGetSmgr(relation), forkNum);
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
@@ -3527,9 +3518,6 @@ FlushRelationBuffers(Relation rel)
int i;
BufferDesc *bufHdr;
/* Open rel at the smgr level if not already done */
RelationOpenSmgr(rel);
if (RelationUsesLocalBuffers(rel))
{
for (i = 0; i < NLocBuffer; i++)
@@ -3554,7 +3542,7 @@ FlushRelationBuffers(Relation rel)
PageSetChecksumInplace(localpage, bufHdr->tag.blockNum);
smgrwrite(rel->rd_smgr,
smgrwrite(RelationGetSmgr(rel),
bufHdr->tag.forkNum,
bufHdr->tag.blockNum,
localpage,
@@ -3595,7 +3583,7 @@ FlushRelationBuffers(Relation rel)
{
PinBuffer_Locked(bufHdr);
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
FlushBuffer(bufHdr, rel->rd_smgr);
FlushBuffer(bufHdr, RelationGetSmgr(rel));
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
UnpinBuffer(bufHdr, true);
}

52
src/backend/storage/freespace/freespace.c
View File

@@ -265,13 +265,11 @@ FreeSpaceMapPrepareTruncateRel(Relation rel, BlockNumber nblocks)
uint16 first_removed_slot;
Buffer buf;
RelationOpenSmgr(rel);
/*
* If no FSM has been created yet for this relation, there's nothing to
* truncate.
*/
if (!smgrexists(rel->rd_smgr, FSM_FORKNUM))
if (!smgrexists(RelationGetSmgr(rel), FSM_FORKNUM))
return InvalidBlockNumber;
/* Get the location in the FSM of the first removed heap block */
@@ -317,7 +315,7 @@ FreeSpaceMapPrepareTruncateRel(Relation rel, BlockNumber nblocks)
else
{
new_nfsmblocks = fsm_logical_to_physical(first_removed_address);
if (smgrnblocks(rel->rd_smgr, FSM_FORKNUM) <= new_nfsmblocks)
if (smgrnblocks(RelationGetSmgr(rel), FSM_FORKNUM) <= new_nfsmblocks)
return InvalidBlockNumber; /* nothing to do; the FSM was already
* smaller */
}
@@ -532,8 +530,14 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
{
BlockNumber blkno = fsm_logical_to_physical(addr);
Buffer buf;
SMgrRelation reln;
RelationOpenSmgr(rel);
/*
* Caution: re-using this smgr pointer could fail if the relcache entry
* gets closed. It's safe as long as we only do smgr-level operations
* between here and the last use of the pointer.
*/
reln = RelationGetSmgr(rel);
/*
* If we haven't cached the size of the FSM yet, check it first. Also
@@ -541,19 +545,19 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
* value might be stale. (We send smgr inval messages on truncation, but
* not on extension.)
*/
if (rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM] == InvalidBlockNumber ||
blkno >= rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM])
if (reln->smgr_cached_nblocks[FSM_FORKNUM] == InvalidBlockNumber ||
blkno >= reln->smgr_cached_nblocks[FSM_FORKNUM])
{
/* Invalidate the cache so smgrnblocks asks the kernel. */
rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM] = InvalidBlockNumber;
if (smgrexists(rel->rd_smgr, FSM_FORKNUM))
smgrnblocks(rel->rd_smgr, FSM_FORKNUM);
reln->smgr_cached_nblocks[FSM_FORKNUM] = InvalidBlockNumber;
if (smgrexists(reln, FSM_FORKNUM))
smgrnblocks(reln, FSM_FORKNUM);
else
rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM] = 0;
reln->smgr_cached_nblocks[FSM_FORKNUM] = 0;
}
/* Handle requests beyond EOF */
if (blkno >= rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM])
if (blkno >= reln->smgr_cached_nblocks[FSM_FORKNUM])
{
if (extend)
fsm_extend(rel, blkno + 1);
@@ -603,6 +607,7 @@ fsm_extend(Relation rel, BlockNumber fsm_nblocks)
{
BlockNumber fsm_nblocks_now;
PGAlignedBlock pg;
SMgrRelation reln;
PageInit((Page) pg.data, BLCKSZ, 0);
@@ -618,28 +623,33 @@ fsm_extend(Relation rel, BlockNumber fsm_nblocks)
*/
LockRelationForExtension(rel, ExclusiveLock);
/* Might have to re-open if a cache flush happened */
RelationOpenSmgr(rel);
/*
* Caution: re-using this smgr pointer could fail if the relcache entry
* gets closed. It's safe as long as we only do smgr-level operations
* between here and the last use of the pointer.
*/
reln = RelationGetSmgr(rel);
/*
* Create the FSM file first if it doesn't exist. If
* smgr_cached_nblocks[FSM_FORKNUM] is positive then it must exist, no
* need for an smgrexists call.
*/
if ((rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM] == 0 ||
rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM] == InvalidBlockNumber) &&
!smgrexists(rel->rd_smgr, FSM_FORKNUM))
smgrcreate(rel->rd_smgr, FSM_FORKNUM, false);
if ((reln->smgr_cached_nblocks[FSM_FORKNUM] == 0 ||
reln->smgr_cached_nblocks[FSM_FORKNUM] == InvalidBlockNumber) &&
!smgrexists(reln, FSM_FORKNUM))
smgrcreate(reln, FSM_FORKNUM, false);
/* Invalidate cache so that smgrnblocks() asks the kernel. */
rel->rd_smgr->smgr_cached_nblocks[FSM_FORKNUM] = InvalidBlockNumber;
fsm_nblocks_now = smgrnblocks(rel->rd_smgr, FSM_FORKNUM);
reln->smgr_cached_nblocks[FSM_FORKNUM] = InvalidBlockNumber;
fsm_nblocks_now = smgrnblocks(reln, FSM_FORKNUM);
/* Extend as needed. */
while (fsm_nblocks_now < fsm_nblocks)
{
PageSetChecksumInplace((Page) pg.data, fsm_nblocks_now);
smgrextend(rel->rd_smgr, FSM_FORKNUM, fsm_nblocks_now,
smgrextend(reln, FSM_FORKNUM, fsm_nblocks_now,
pg.data, false);
fsm_nblocks_now++;
}