Skip to content

Commit e21856f

Browse files
tglsfdctglsfdc
committed
Replace RelationOpenSmgr() with RelationGetSmgr().
This is a back-patch of the v15-era commit f10f0ae 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 f10f0ae. Therefore, even though this is a bit invasive, it's time to back-patch. Some comfort can be taken in the fact that f10f0ae 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 parent 36dd007 commit e21856f

File tree

21 files changed

+173
-148
lines changed

21 files changed

+173
-148
lines changed

‎contrib/amcheck/verify_nbtree.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -301,8 +301,7 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
301301
{
302302
bool heapkeyspace;
303303

304-
RelationOpenSmgr(indrel);
305-
if (!smgrexists(indrel->rd_smgr, MAIN_FORKNUM))
304+
if (!smgrexists(RelationGetSmgr(indrel), MAIN_FORKNUM))
306305
ereport(ERROR,
307306
(errcode(ERRCODE_INDEX_CORRUPTED),
308307
errmsg("index \"%s\" lacks a main relation fork",

‎contrib/bloom/blinsert.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,17 +179,17 @@ blbuildempty(Relation index)
179179
* this even when wal_level=minimal.
180180
*/
181181
PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
182-
smgrwrite(index->rd_smgr, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
182+
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
183183
(char *) metapage, true);
184-
log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
184+
log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM,
185185
BLOOM_METAPAGE_BLKNO, metapage, true);
186186

187187
/*
188188
* An immediate sync is required even if we xlog'd the page, because the
189189
* write did not go through shared_buffers and therefore a concurrent
190190
* checkpoint may have moved the redo pointer past our xlog record.
191191
*/
192-
smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
192+
smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM);
193193
}
194194

195195
/*

‎contrib/pg_prewarm/autoprewarm.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -518,15 +518,13 @@ autoprewarm_database_main(Datum main_arg)
518518
old_blk->filenode != blk->filenode ||
519519
old_blk->forknum != blk->forknum)
520520
{
521-
RelationOpenSmgr(rel);
522-
523521
/*
524522
* smgrexists is not safe for illegal forknum, hence check whether
525523
* the passed forknum is valid before using it in smgrexists.
526524
*/
527525
if (blk->forknum > InvalidForkNumber &&
528526
blk->forknum <= MAX_FORKNUM &&
529-
smgrexists(rel->rd_smgr, blk->forknum))
527+
smgrexists(RelationGetSmgr(rel), blk->forknum))
530528
nblocks = RelationGetNumberOfBlocksInFork(rel, blk->forknum);
531529
else
532530
nblocks = 0;

‎contrib/pg_prewarm/pg_prewarm.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,7 @@ pg_prewarm(PG_FUNCTION_ARGS)
109109
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
110110

111111
/* Check that the fork exists. */
112-
RelationOpenSmgr(rel);
113-
if (!smgrexists(rel->rd_smgr, forkNumber))
112+
if (!smgrexists(RelationGetSmgr(rel), forkNumber))
114113
ereport(ERROR,
115114
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
116115
errmsg("fork \"%s\" does not exist for this relation",
@@ -178,7 +177,7 @@ pg_prewarm(PG_FUNCTION_ARGS)
178177
for (block = first_block; block <= last_block; ++block)
179178
{
180179
CHECK_FOR_INTERRUPTS();
181-
smgrread(rel->rd_smgr, forkNumber, block, blockbuffer.data);
180+
smgrread(RelationGetSmgr(rel), forkNumber, block, blockbuffer.data);
182181
++blocks_done;
183182
}
184183
}

‎contrib/pg_visibility/pg_visibility.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -389,8 +389,8 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
389389
/* Only some relkinds have a visibility map */
390390
check_relation_relkind(rel);
391391

392-
RelationOpenSmgr(rel);
393-
rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
392+
/* Forcibly reset cached file size */
393+
RelationGetSmgr(rel)->smgr_vm_nblocks = InvalidBlockNumber;
394394

395395
visibilitymap_truncate(rel, 0);
396396

‎src/backend/access/gist/gistbuild.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -514,7 +514,7 @@ gistBuildCallback(Relation index,
514514
*/
515515
if ((buildstate->bufferingMode == GIST_BUFFERING_AUTO &&
516516
buildstate->indtuples % BUFFERING_MODE_SWITCH_CHECK_STEP == 0 &&
517-
effective_cache_size < smgrnblocks(index->rd_smgr, MAIN_FORKNUM)) ||
517+
effective_cache_size < smgrnblocks(RelationGetSmgr(index), MAIN_FORKNUM)) ||
518518
(buildstate->bufferingMode == GIST_BUFFERING_STATS &&
519519
buildstate->indtuples >= BUFFERING_MODE_TUPLE_SIZE_STATS_TARGET))
520520
{

‎src/backend/access/hash/hashpage.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,9 +1031,9 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
10311031
zerobuf.data,
10321032
true);
10331033

1034-
RelationOpenSmgr(rel);
10351034
PageSetChecksumInplace(page, lastblock);
1036-
smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf.data, false);
1035+
smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data,
1036+
false);
10371037

10381038
return true;
10391039
}

‎src/backend/access/heap/heapam.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9098,8 +9098,7 @@ heap_sync(Relation rel)
90989098

90999099
/* main heap */
91009100
FlushRelationBuffers(rel);
9101-
/* FlushRelationBuffers will have opened rd_smgr */
9102-
smgrimmedsync(rel->rd_smgr, MAIN_FORKNUM);
9101+
smgrimmedsync(RelationGetSmgr(rel), MAIN_FORKNUM);
91039102

91049103
/* FSM is not critical, don't bother syncing it */
91059104

@@ -9110,7 +9109,7 @@ heap_sync(Relation rel)
91109109

91119110
toastrel = table_open(rel->rd_rel->reltoastrelid, AccessShareLock);
91129111
FlushRelationBuffers(toastrel);
9113-
smgrimmedsync(toastrel->rd_smgr, MAIN_FORKNUM);
9112+
smgrimmedsync(RelationGetSmgr(toastrel), MAIN_FORKNUM);
91149113
table_close(toastrel, AccessShareLock);
91159114
}
91169115
}

‎src/backend/access/heap/heapam_handler.c

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -643,7 +643,6 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode)
643643
SMgrRelation dstrel;
644644

645645
dstrel = smgropen(*newrnode, rel->rd_backend);
646-
RelationOpenSmgr(rel);
647646

648647
/*
649648
* Since we copy the file directly without looking at the shared buffers,
@@ -663,14 +662,14 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode)
663662
RelationCreateStorage(*newrnode, rel->rd_rel->relpersistence);
664663

665664
/* copy main fork */
666-
RelationCopyStorage(rel->rd_smgr, dstrel, MAIN_FORKNUM,
665+
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
667666
rel->rd_rel->relpersistence);
668667

669668
/* copy those extra forks that exist */
670669
for (ForkNumber forkNum = MAIN_FORKNUM + 1;
671670
forkNum <= MAX_FORKNUM; forkNum++)
672671
{
673-
if (smgrexists(rel->rd_smgr, forkNum))
672+
if (smgrexists(RelationGetSmgr(rel), forkNum))
674673
{
675674
smgrcreate(dstrel, forkNum, false);
676675

@@ -682,7 +681,7 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode)
682681
(rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
683682
forkNum == INIT_FORKNUM))
684683
log_smgrcreate(newrnode, forkNum);
685-
RelationCopyStorage(rel->rd_smgr, dstrel, forkNum,
684+
RelationCopyStorage(RelationGetSmgr(rel), dstrel, forkNum,
686685
rel->rd_rel->relpersistence);
687686
}
688687
}
@@ -2045,18 +2044,23 @@ static uint64
20452044
heapam_relation_size(Relation rel, ForkNumber forkNumber)
20462045
{
20472046
uint64 nblocks = 0;
2047+
SMgrRelation reln;
20482048

2049-
/* Open it at the smgr level if not already done */
2050-
RelationOpenSmgr(rel);
2049+
/*
2050+
* Caution: re-using this smgr pointer could fail if the relcache entry
2051+
* gets closed. It's safe as long as we only do smgr-level operations
2052+
* between here and the last use of the pointer.
2053+
*/
2054+
reln = RelationGetSmgr(rel);
20512055

20522056
/* InvalidForkNumber indicates returning the size for all forks */
20532057
if (forkNumber == InvalidForkNumber)
20542058
{
20552059
for (int i = 0; i < MAX_FORKNUM; i++)
2056-
nblocks += smgrnblocks(rel->rd_smgr, i);
2060+
nblocks += smgrnblocks(reln, i);
20572061
}
20582062
else
2059-
nblocks = smgrnblocks(rel->rd_smgr, forkNumber);
2063+
nblocks = smgrnblocks(reln, forkNumber);
20602064

20612065
return nblocks * BLCKSZ;
20622066
}

‎src/backend/access/heap/rewriteheap.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -336,11 +336,10 @@ end_heap_rewrite(RewriteState state)
336336
state->rs_blockno,
337337
state->rs_buffer,
338338
true);
339-
RelationOpenSmgr(state->rs_new_rel);
340339

341340
PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
342341

343-
smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
342+
smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM, state->rs_blockno,
344343
(char *) state->rs_buffer, true);
345344
}
346345

@@ -708,11 +707,9 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
708707
* fsync for this write; we'll do it ourselves in
709708
* end_heap_rewrite.
710709
*/
711-
RelationOpenSmgr(state->rs_new_rel);
712-
713710
PageSetChecksumInplace(page, state->rs_blockno);
714711

715-
smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
712+
smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,
716713
state->rs_blockno, (char *) page, true);
717714

718715
state->rs_blockno++;

‎src/backend/access/heap/visibilitymap.c

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -452,13 +452,11 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
452452
elog(DEBUG1, "vm_truncate %s %d", RelationGetRelationName(rel), nheapblocks);
453453
#endif
454454

455-
RelationOpenSmgr(rel);
456-
457455
/*
458456
* If no visibility map has been created yet for this relation, there's
459457
* nothing to truncate.
460458
*/
461-
if (!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
459+
if (!smgrexists(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM))
462460
return;
463461

464462
/*
@@ -525,14 +523,14 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
525523
else
526524
newnblocks = truncBlock;
527525

528-
if (smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM) <= newnblocks)
526+
if (smgrnblocks(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM) <= newnblocks)
529527
{
530528
/* nothing to do, the file was already smaller than requested size */
531529
return;
532530
}
533531

534532
/* Truncate the unused VM pages, and send smgr inval message */
535-
smgrtruncate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, newnblocks);
533+
smgrtruncate(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM, newnblocks);
536534

537535
/*
538536
* We might as well update the local smgr_vm_nblocks setting. smgrtruncate
@@ -554,31 +552,30 @@ static Buffer
554552
vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
555553
{
556554
Buffer buf;
555+
SMgrRelation reln;
557556

558557
/*
559-
* We might not have opened the relation at the smgr level yet, or we
560-
* might have been forced to close it by a sinval message. The code below
561-
* won't necessarily notice relation extension immediately when extend =
562-
* false, so we rely on sinval messages to ensure that our ideas about the
563-
* size of the map aren't too far out of date.
558+
* Caution: re-using this smgr pointer could fail if the relcache entry
559+
* gets closed. It's safe as long as we only do smgr-level operations
560+
* between here and the last use of the pointer.
564561
*/
565-
RelationOpenSmgr(rel);
562+
reln = RelationGetSmgr(rel);
566563

567564
/*
568565
* If we haven't cached the size of the visibility map fork yet, check it
569566
* first.
570567
*/
571-
if (rel->rd_smgr->smgr_vm_nblocks == InvalidBlockNumber)
568+
if (reln->smgr_vm_nblocks == InvalidBlockNumber)
572569
{
573-
if (smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
574-
rel->rd_smgr->smgr_vm_nblocks = smgrnblocks(rel->rd_smgr,
575-
VISIBILITYMAP_FORKNUM);
570+
if (smgrexists(reln, VISIBILITYMAP_FORKNUM))
571+
reln->smgr_vm_nblocks = smgrnblocks(reln,
572+
VISIBILITYMAP_FORKNUM);
576573
else
577-
rel->rd_smgr->smgr_vm_nblocks = 0;
574+
reln->smgr_vm_nblocks = 0;
578575
}
579576

580577
/* Handle requests beyond EOF */
581-
if (blkno >= rel->rd_smgr->smgr_vm_nblocks)
578+
if (blkno >= reln->smgr_vm_nblocks)
582579
{
583580
if (extend)
584581
vm_extend(rel, blkno + 1);
@@ -626,6 +623,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
626623
{
627624
BlockNumber vm_nblocks_now;
628625
PGAlignedBlock pg;
626+
SMgrRelation reln;
629627

630628
PageInit((Page) pg.data, BLCKSZ, 0);
631629

@@ -641,26 +639,30 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
641639
*/
642640
LockRelationForExtension(rel, ExclusiveLock);
643641

644-
/* Might have to re-open if a cache flush happened */
645-
RelationOpenSmgr(rel);
642+
/*
643+
* Caution: re-using this smgr pointer could fail if the relcache entry
644+
* gets closed. It's safe as long as we only do smgr-level operations
645+
* between here and the last use of the pointer.
646+
*/
647+
reln = RelationGetSmgr(rel);
646648

647649
/*
648650
* Create the file first if it doesn't exist. If smgr_vm_nblocks is
649651
* positive then it must exist, no need for an smgrexists call.
650652
*/
651-
if ((rel->rd_smgr->smgr_vm_nblocks == 0 ||
652-
rel->rd_smgr->smgr_vm_nblocks == InvalidBlockNumber) &&
653-
!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
654-
smgrcreate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, false);
653+
if ((reln->smgr_vm_nblocks == 0 ||
654+
reln->smgr_vm_nblocks == InvalidBlockNumber) &&
655+
!smgrexists(reln, VISIBILITYMAP_FORKNUM))
656+
smgrcreate(reln, VISIBILITYMAP_FORKNUM, false);
655657

656-
vm_nblocks_now = smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
658+
vm_nblocks_now = smgrnblocks(reln, VISIBILITYMAP_FORKNUM);
657659

658660
/* Now extend the file */
659661
while (vm_nblocks_now < vm_nblocks)
660662
{
661663
PageSetChecksumInplace((Page) pg.data, vm_nblocks_now);
662664

663-
smgrextend(rel->rd_smgr, VISIBILITYMAP_FORKNUM, vm_nblocks_now,
665+
smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now,
664666
pg.data, false);
665667
vm_nblocks_now++;
666668
}
@@ -672,10 +674,10 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
672674
* to keep checking for creation or extension of the file, which happens
673675
* infrequently.
674676
*/
675-
CacheInvalidateSmgr(rel->rd_smgr->smgr_rnode);
677+
CacheInvalidateSmgr(reln->smgr_rnode);
676678

677679
/* Update local cache with the up-to-date size */
678-
rel->rd_smgr->smgr_vm_nblocks = vm_nblocks_now;
680+
reln->smgr_vm_nblocks = vm_nblocks_now;
679681

680682
UnlockRelationForExtension(rel, ExclusiveLock);
681683
}

‎src/backend/access/nbtree/nbtree.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -170,17 +170,17 @@ btbuildempty(Relation index)
170170
* this even when wal_level=minimal.
171171
*/
172172
PageSetChecksumInplace(metapage, BTREE_METAPAGE);
173-
smgrwrite(index->rd_smgr, INIT_FORKNUM, BTREE_METAPAGE,
173+
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BTREE_METAPAGE,
174174
(char *) metapage, true);
175-
log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
175+
log_newpage(&RelationGetSmgr(index)->smgr_rnode.node, INIT_FORKNUM,
176176
BTREE_METAPAGE, metapage, true);
177177

178178
/*
179179
* An immediate sync is required even if we xlog'd the page, because the
180180
* write did not go through shared_buffers and therefore a concurrent
181181
* checkpoint may have moved the redo pointer past our xlog record.
182182
*/
183-
smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
183+
smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM);
184184
}
185185

186186
/*

0 commit comments

Comments
 (0)