Replace RelationOpenSmgr() with RelationGetSmgr().

This is a back-patch of the v15-era commit f10f0ae42 into older
supported branches.  The idea is to design out bugs in which an
ill-timed relcache flush clears rel->rd_smgr partway through
some code sequence that wasn't expecting that.  We had another
report today of a corner case that reliably crashes v14 under
debug_discard_caches (nee CLOBBER_CACHE_ALWAYS), and therefore
would crash once in a blue moon in the field.  We're unlikely
to get rid of all such code paths unless we adopt the more
rigorous coding rules instituted by f10f0ae42.  Therefore,
even though this is a bit invasive, it's time to back-patch.
Some comfort can be taken in the fact that f10f0ae42 has been
in v15 for 16 months without problems.

I left the RelationOpenSmgr macro present in the back branches,
even though no core code should use it anymore, in order to not break
third-party extensions in minor releases.  Such extensions might opt
to start using RelationGetSmgr instead, to reduce their code
differential between v15 and earlier branches.  This carries a hazard
of failing to compile against headers from existing minor releases.
However, once compiled the extension should work fine even with such
releases, because RelationGetSmgr is a "static inline" function so
it creates no link-time dependency.  So depending on distribution
practices, that might be an OK tradeoff.

Per report from Spyridon Dimitrios Agathos.  Original patch
by Amul Sul.

Discussion: https://postgr.es/m/CAFM5RaqdgyusQvmWkyPYaWMwoK5gigdtW-7HcgHgOeAw7mqJ_Q@mail.gmail.com
Discussion: https://postgr.es/m/CANiYTQsU7yMFpQYnv=BrcRVqK_3U3mtAzAsJCaqtzsDHfsUbdQ@mail.gmail.com

1 files changed, 31 insertions, 21 deletions

diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 8c12dda2380..09d4b16067d 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -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++;
 	}