Skip to content

Commit 32d5a49

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 aaf28c9 commit 32d5a49

File tree

21 files changed

+171
-162
lines changed

21 files changed

+171
-162
lines changed

‎contrib/amcheck/verify_nbtree.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,7 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed,
322322
bool heapkeyspace,
323323
allequalimage;
324324

325-
RelationOpenSmgr(indrel);
326-
if (!smgrexists(indrel->rd_smgr, MAIN_FORKNUM))
325+
if (!smgrexists(RelationGetSmgr(indrel), MAIN_FORKNUM))
327326
ereport(ERROR,
328327
(errcode(ERRCODE_INDEX_CORRUPTED),
329328
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
@@ -177,17 +177,17 @@ blbuildempty(Relation index)
177177
* this even when wal_level=minimal.
178178
*/
179179
PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
180-
smgrwrite(index->rd_smgr, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
180+
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
181181
(char *) metapage, true);
182-
log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
182+
log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM,
183183
BLOOM_METAPAGE_BLKNO, metapage, true);
184184

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

193193
/*

‎contrib/pg_prewarm/autoprewarm.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -514,15 +514,13 @@ autoprewarm_database_main(Datum main_arg)
514514
old_blk->filenode != blk->filenode ||
515515
old_blk->forknum != blk->forknum)
516516
{
517-
RelationOpenSmgr(rel);
518-
519517
/*
520518
* smgrexists is not safe for illegal forknum, hence check whether
521519
* the passed forknum is valid before using it in smgrexists.
522520
*/
523521
if (blk->forknum > InvalidForkNumber &&
524522
blk->forknum <= MAX_FORKNUM &&
525-
smgrexists(rel->rd_smgr, blk->forknum))
523+
smgrexists(RelationGetSmgr(rel), blk->forknum))
526524
nblocks = RelationGetNumberOfBlocksInFork(rel, blk->forknum);
527525
else
528526
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: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -391,14 +391,14 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
391391
/* Only some relkinds have a visibility map */
392392
check_relation_relkind(rel);
393393

394-
RelationOpenSmgr(rel);
395-
rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
394+
/* Forcibly reset cached file size */
395+
RelationGetSmgr(rel)->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
396396

397397
block = visibilitymap_prepare_truncate(rel, 0);
398398
if (BlockNumberIsValid(block))
399399
{
400400
fork = VISIBILITYMAP_FORKNUM;
401-
smgrtruncate(rel->rd_smgr, &fork, 1, &block);
401+
smgrtruncate(RelationGetSmgr(rel), &fork, 1, &block);
402402
}
403403

404404
if (RelationNeedsWAL(rel))

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

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -409,8 +409,7 @@ gist_indexsortbuild(GISTBuildState *state)
409409
* replaced with the real root page at the end.
410410
*/
411411
page = palloc0(BLCKSZ);
412-
RelationOpenSmgr(state->indexrel);
413-
smgrextend(state->indexrel->rd_smgr, MAIN_FORKNUM, GIST_ROOT_BLKNO,
412+
smgrextend(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, GIST_ROOT_BLKNO,
414413
page, true);
415414
state->pages_allocated++;
416415
state->pages_written++;
@@ -450,10 +449,9 @@ gist_indexsortbuild(GISTBuildState *state)
450449
gist_indexsortbuild_flush_ready_pages(state);
451450

452451
/* Write out the root */
453-
RelationOpenSmgr(state->indexrel);
454452
PageSetLSN(pagestate->page, GistBuildLSN);
455453
PageSetChecksumInplace(pagestate->page, GIST_ROOT_BLKNO);
456-
smgrwrite(state->indexrel->rd_smgr, MAIN_FORKNUM, GIST_ROOT_BLKNO,
454+
smgrwrite(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, GIST_ROOT_BLKNO,
457455
pagestate->page, true);
458456
if (RelationNeedsWAL(state->indexrel))
459457
log_newpage(&state->indexrel->rd_node, MAIN_FORKNUM, GIST_ROOT_BLKNO,
@@ -472,10 +470,7 @@ gist_indexsortbuild(GISTBuildState *state)
472470
* still not be on disk when the crash occurs.
473471
*/
474472
if (RelationNeedsWAL(state->indexrel))
475-
{
476-
RelationOpenSmgr(state->indexrel);
477-
smgrimmedsync(state->indexrel->rd_smgr, MAIN_FORKNUM);
478-
}
473+
smgrimmedsync(RelationGetSmgr(state->indexrel), MAIN_FORKNUM);
479474
}
480475

481476
/*
@@ -577,8 +572,6 @@ gist_indexsortbuild_flush_ready_pages(GISTBuildState *state)
577572
if (state->ready_num_pages == 0)
578573
return;
579574

580-
RelationOpenSmgr(state->indexrel);
581-
582575
for (int i = 0; i < state->ready_num_pages; i++)
583576
{
584577
Page page = state->ready_pages[i];
@@ -590,7 +583,8 @@ gist_indexsortbuild_flush_ready_pages(GISTBuildState *state)
590583

591584
PageSetLSN(page, GistBuildLSN);
592585
PageSetChecksumInplace(page, blkno);
593-
smgrextend(state->indexrel->rd_smgr, MAIN_FORKNUM, blkno, page, true);
586+
smgrextend(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, blkno, page,
587+
true);
594588

595589
state->pages_written++;
596590
}
@@ -875,7 +869,8 @@ gistBuildCallback(Relation index,
875869
*/
876870
if ((buildstate->buildMode == GIST_BUFFERING_AUTO &&
877871
buildstate->indtuples % BUFFERING_MODE_SWITCH_CHECK_STEP == 0 &&
878-
effective_cache_size < smgrnblocks(index->rd_smgr, MAIN_FORKNUM)) ||
872+
effective_cache_size < smgrnblocks(RelationGetSmgr(index),
873+
MAIN_FORKNUM)) ||
879874
(buildstate->buildMode == GIST_BUFFERING_STATS &&
880875
buildstate->indtuples >= BUFFERING_MODE_TUPLE_SIZE_STATS_TARGET))
881876
{

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,9 +1028,9 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
10281028
zerobuf.data,
10291029
true);
10301030

1031-
RelationOpenSmgr(rel);
10321031
PageSetChecksumInplace(page, lastblock);
1033-
smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf.data, false);
1032+
smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data,
1033+
false);
10341034

10351035
return true;
10361036
}

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,6 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode)
628628
SMgrRelation dstrel;
629629

630630
dstrel = smgropen(*newrnode, rel->rd_backend);
631-
RelationOpenSmgr(rel);
632631

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

650649
/* copy main fork */
651-
RelationCopyStorage(rel->rd_smgr, dstrel, MAIN_FORKNUM,
650+
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
652651
rel->rd_rel->relpersistence);
653652

654653
/* copy those extra forks that exist */
655654
for (ForkNumber forkNum = MAIN_FORKNUM + 1;
656655
forkNum <= MAX_FORKNUM; forkNum++)
657656
{
658-
if (smgrexists(rel->rd_smgr, forkNum))
657+
if (smgrexists(RelationGetSmgr(rel), forkNum))
659658
{
660659
smgrcreate(dstrel, forkNum, false);
661660

@@ -667,7 +666,7 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode)
667666
(rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
668667
forkNum == INIT_FORKNUM))
669668
log_smgrcreate(newrnode, forkNum);
670-
RelationCopyStorage(rel->rd_smgr, dstrel, forkNum,
669+
RelationCopyStorage(RelationGetSmgr(rel), dstrel, forkNum,
671670
rel->rd_rel->relpersistence);
672671
}
673672
}

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

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -326,9 +326,8 @@ end_heap_rewrite(RewriteState state)
326326

327327
PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
328328

329-
RelationOpenSmgr(state->rs_new_rel);
330-
smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno,
331-
(char *) state->rs_buffer, true);
329+
smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,
330+
state->rs_blockno, (char *) state->rs_buffer, true);
332331
}
333332

334333
/*
@@ -339,11 +338,7 @@ end_heap_rewrite(RewriteState state)
339338
* wrote before the checkpoint.
340339
*/
341340
if (RelationNeedsWAL(state->rs_new_rel))
342-
{
343-
/* for an empty table, this could be first smgr access */
344-
RelationOpenSmgr(state->rs_new_rel);
345-
smgrimmedsync(state->rs_new_rel->rd_smgr, MAIN_FORKNUM);
346-
}
341+
smgrimmedsync(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM);
347342

348343
logical_end_heap_rewrite(state);
349344

@@ -695,11 +690,9 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
695690
* need for smgr to schedule an fsync for this write; we'll do it
696691
* ourselves in end_heap_rewrite.
697692
*/
698-
RelationOpenSmgr(state->rs_new_rel);
699-
700693
PageSetChecksumInplace(page, state->rs_blockno);
701694

702-
smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM,
695+
smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,
703696
state->rs_blockno, (char *) page, true);
704697

705698
state->rs_blockno++;

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

Lines changed: 27 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -455,13 +455,11 @@ visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks)
455455
elog(DEBUG1, "vm_truncate %s %d", RelationGetRelationName(rel), nheapblocks);
456456
#endif
457457

458-
RelationOpenSmgr(rel);
459-
460458
/*
461459
* If no visibility map has been created yet for this relation, there's
462460
* nothing to truncate.
463461
*/
464-
if (!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
462+
if (!smgrexists(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM))
465463
return InvalidBlockNumber;
466464

467465
/*
@@ -528,7 +526,7 @@ visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks)
528526
else
529527
newnblocks = truncBlock;
530528

531-
if (smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM) <= newnblocks)
529+
if (smgrnblocks(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM) <= newnblocks)
532530
{
533531
/* nothing to do, the file was already smaller than requested size */
534532
return InvalidBlockNumber;
@@ -547,30 +545,29 @@ static Buffer
547545
vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
548546
{
549547
Buffer buf;
548+
SMgrRelation reln;
550549

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

560557
/*
561558
* If we haven't cached the size of the visibility map fork yet, check it
562559
* first.
563560
*/
564-
if (rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber)
561+
if (reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber)
565562
{
566-
if (smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
567-
smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
563+
if (smgrexists(reln, VISIBILITYMAP_FORKNUM))
564+
smgrnblocks(reln, VISIBILITYMAP_FORKNUM);
568565
else
569-
rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0;
566+
reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0;
570567
}
571568

572569
/* Handle requests beyond EOF */
573-
if (blkno >= rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM])
570+
if (blkno >= reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM])
574571
{
575572
if (extend)
576573
vm_extend(rel, blkno + 1);
@@ -618,6 +615,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
618615
{
619616
BlockNumber vm_nblocks_now;
620617
PGAlignedBlock pg;
618+
SMgrRelation reln;
621619

622620
PageInit((Page) pg.data, BLCKSZ, 0);
623621

@@ -633,29 +631,32 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
633631
*/
634632
LockRelationForExtension(rel, ExclusiveLock);
635633

636-
/* Might have to re-open if a cache flush happened */
637-
RelationOpenSmgr(rel);
634+
/*
635+
* Caution: re-using this smgr pointer could fail if the relcache entry
636+
* gets closed. It's safe as long as we only do smgr-level operations
637+
* between here and the last use of the pointer.
638+
*/
639+
reln = RelationGetSmgr(rel);
638640

639641
/*
640642
* Create the file first if it doesn't exist. If smgr_vm_nblocks is
641643
* positive then it must exist, no need for an smgrexists call.
642644
*/
643-
if ((rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 ||
644-
rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) &&
645-
!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
646-
smgrcreate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, false);
645+
if ((reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 ||
646+
reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) &&
647+
!smgrexists(reln, VISIBILITYMAP_FORKNUM))
648+
smgrcreate(reln, VISIBILITYMAP_FORKNUM, false);
647649

648650
/* Invalidate cache so that smgrnblocks() asks the kernel. */
649-
rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
650-
vm_nblocks_now = smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
651+
reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
652+
vm_nblocks_now = smgrnblocks(reln, VISIBILITYMAP_FORKNUM);
651653

652654
/* Now extend the file */
653655
while (vm_nblocks_now < vm_nblocks)
654656
{
655657
PageSetChecksumInplace((Page) pg.data, vm_nblocks_now);
656658

657-
smgrextend(rel->rd_smgr, VISIBILITYMAP_FORKNUM, vm_nblocks_now,
658-
pg.data, false);
659+
smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now, pg.data, false);
659660
vm_nblocks_now++;
660661
}
661662

@@ -666,7 +667,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
666667
* to keep checking for creation or extension of the file, which happens
667668
* infrequently.
668669
*/
669-
CacheInvalidateSmgr(rel->rd_smgr->smgr_rnode);
670+
CacheInvalidateSmgr(reln->smgr_rnode);
670671

671672
UnlockRelationForExtension(rel, ExclusiveLock);
672673
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,17 +163,17 @@ btbuildempty(Relation index)
163163
* this even when wal_level=minimal.
164164
*/
165165
PageSetChecksumInplace(metapage, BTREE_METAPAGE);
166-
smgrwrite(index->rd_smgr, INIT_FORKNUM, BTREE_METAPAGE,
166+
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BTREE_METAPAGE,
167167
(char *) metapage, true);
168-
log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
168+
log_newpage(&RelationGetSmgr(index)->smgr_rnode.node, INIT_FORKNUM,
169169
BTREE_METAPAGE, metapage, true);
170170

171171
/*
172172
* An immediate sync is required even if we xlog'd the page, because the
173173
* write did not go through shared_buffers and therefore a concurrent
174174
* checkpoint may have moved the redo pointer past our xlog record.
175175
*/
176-
smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
176+
smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM);
177177
}
178178

179179
/*

0 commit comments

Comments
 (0)