From f10f0ae420ee62400876ab34dca2c09c20dcd030 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 12 Jul 2021 17:01:29 -0400 Subject: [PATCH] Replace RelationOpenSmgr() with RelationGetSmgr(). 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 --- contrib/amcheck/verify_nbtree.c | 3 +- contrib/bloom/blinsert.c | 6 +-- contrib/pg_prewarm/autoprewarm.c | 4 +- contrib/pg_prewarm/pg_prewarm.c | 5 +-- contrib/pg_visibility/pg_visibility.c | 6 +-- src/backend/access/gist/gistbuild.c | 14 +++--- src/backend/access/hash/hashpage.c | 4 +- src/backend/access/heap/heapam_handler.c | 7 ++- src/backend/access/heap/rewriteheap.c | 15 ++----- src/backend/access/heap/visibilitymap.c | 53 ++++++++++++----------- src/backend/access/nbtree/nbtree.c | 6 +-- src/backend/access/nbtree/nbtsort.c | 14 ++---- src/backend/access/spgist/spginsert.c | 14 +++--- src/backend/access/table/tableam.c | 7 +-- src/backend/catalog/heap.c | 2 - src/backend/catalog/index.c | 5 +-- src/backend/catalog/storage.c | 46 +++++++++++++------- src/backend/commands/tablecmds.c | 7 ++- src/backend/storage/buffer/bufmgr.c | 24 +++------- src/backend/storage/freespace/freespace.c | 52 +++++++++++++--------- src/include/utils/rel.h | 35 ++++++++++----- 21 files changed, 164 insertions(+), 165 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index fdfc320e84..d19f73127c 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -301,8 +301,7 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed, bool heapkeyspace, allequalimage; - RelationOpenSmgr(indrel); - if (!smgrexists(indrel->rd_smgr, MAIN_FORKNUM)) + if (!smgrexists(RelationGetSmgr(indrel), MAIN_FORKNUM)) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("index \"%s\" lacks a main relation fork", diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c index c34a640d1c..23661d1105 100644 --- a/contrib/bloom/blinsert.c +++ b/contrib/bloom/blinsert.c @@ -177,9 +177,9 @@ blbuildempty(Relation index) * this even when wal_level=minimal. */ PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO); - smgrwrite(index->rd_smgr, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO, + smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BLOOM_METAPAGE_BLKNO, (char *) metapage, true); - log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM, + log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM, BLOOM_METAPAGE_BLKNO, metapage, true); /* @@ -187,7 +187,7 @@ blbuildempty(Relation index) * write did not go through shared_buffers and therefore a concurrent * checkpoint may have moved the redo pointer past our xlog record. */ - smgrimmedsync(index->rd_smgr, INIT_FORKNUM); + smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM); } /* diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index b3f73ea92d..0289ea657c 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -514,15 +514,13 @@ autoprewarm_database_main(Datum main_arg) old_blk->filenode != blk->filenode || old_blk->forknum != blk->forknum) { - RelationOpenSmgr(rel); - /* * smgrexists is not safe for illegal forknum, hence check whether * the passed forknum is valid before using it in smgrexists. */ if (blk->forknum > InvalidForkNumber && blk->forknum <= MAX_FORKNUM && - smgrexists(rel->rd_smgr, blk->forknum)) + smgrexists(RelationGetSmgr(rel), blk->forknum)) nblocks = RelationGetNumberOfBlocksInFork(rel, blk->forknum); else nblocks = 0; diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c index 48d0132a0d..0043823974 100644 --- a/contrib/pg_prewarm/pg_prewarm.c +++ b/contrib/pg_prewarm/pg_prewarm.c @@ -109,8 +109,7 @@ pg_prewarm(PG_FUNCTION_ARGS) aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid)); /* Check that the fork exists. */ - RelationOpenSmgr(rel); - if (!smgrexists(rel->rd_smgr, forkNumber)) + if (!smgrexists(RelationGetSmgr(rel), forkNumber)) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("fork \"%s\" does not exist for this relation", @@ -178,7 +177,7 @@ pg_prewarm(PG_FUNCTION_ARGS) for (block = first_block; block <= last_block; ++block) { CHECK_FOR_INTERRUPTS(); - smgrread(rel->rd_smgr, forkNumber, block, blockbuffer.data); + smgrread(RelationGetSmgr(rel), forkNumber, block, blockbuffer.data); ++blocks_done; } } diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index c6d983183d..b5362edcee 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -391,14 +391,14 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS) /* Only some relkinds have a visibility map */ check_relation_relkind(rel); - RelationOpenSmgr(rel); - rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber; + /* Forcibly reset cached file size */ + RelationGetSmgr(rel)->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber; block = visibilitymap_prepare_truncate(rel, 0); if (BlockNumberIsValid(block)) { fork = VISIBILITYMAP_FORKNUM; - smgrtruncate(rel->rd_smgr, &fork, 1, &block); + smgrtruncate(RelationGetSmgr(rel), &fork, 1, &block); } if (RelationNeedsWAL(rel)) diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c index f46a42197c..baad28c09f 100644 --- a/src/backend/access/gist/gistbuild.c +++ b/src/backend/access/gist/gistbuild.c @@ -409,8 +409,7 @@ gist_indexsortbuild(GISTBuildState *state) * replaced with the real root page at the end. */ page = palloc0(BLCKSZ); - RelationOpenSmgr(state->indexrel); - smgrextend(state->indexrel->rd_smgr, MAIN_FORKNUM, GIST_ROOT_BLKNO, + smgrextend(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, GIST_ROOT_BLKNO, page, true); state->pages_allocated++; state->pages_written++; @@ -450,10 +449,9 @@ gist_indexsortbuild(GISTBuildState *state) gist_indexsortbuild_flush_ready_pages(state); /* Write out the root */ - RelationOpenSmgr(state->indexrel); PageSetLSN(pagestate->page, GistBuildLSN); PageSetChecksumInplace(pagestate->page, GIST_ROOT_BLKNO); - smgrwrite(state->indexrel->rd_smgr, MAIN_FORKNUM, GIST_ROOT_BLKNO, + smgrwrite(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, GIST_ROOT_BLKNO, pagestate->page, true); if (RelationNeedsWAL(state->indexrel)) log_newpage(&state->indexrel->rd_node, MAIN_FORKNUM, GIST_ROOT_BLKNO, @@ -562,8 +560,6 @@ gist_indexsortbuild_flush_ready_pages(GISTBuildState *state) if (state->ready_num_pages == 0) return; - RelationOpenSmgr(state->indexrel); - for (int i = 0; i < state->ready_num_pages; i++) { Page page = state->ready_pages[i]; @@ -575,7 +571,8 @@ gist_indexsortbuild_flush_ready_pages(GISTBuildState *state) PageSetLSN(page, GistBuildLSN); PageSetChecksumInplace(page, blkno); - smgrextend(state->indexrel->rd_smgr, MAIN_FORKNUM, blkno, page, true); + smgrextend(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, blkno, page, + true); state->pages_written++; } @@ -860,7 +857,8 @@ gistBuildCallback(Relation index, */ if ((buildstate->buildMode == GIST_BUFFERING_AUTO && buildstate->indtuples % BUFFERING_MODE_SWITCH_CHECK_STEP == 0 && - effective_cache_size < smgrnblocks(index->rd_smgr, MAIN_FORKNUM)) || + effective_cache_size < smgrnblocks(RelationGetSmgr(index), + MAIN_FORKNUM)) || (buildstate->buildMode == GIST_BUFFERING_STATS && buildstate->indtuples >= BUFFERING_MODE_TUPLE_SIZE_STATS_TARGET)) { diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c index c5c2382b36..b730025356 100644 --- a/src/backend/access/hash/hashpage.c +++ b/src/backend/access/hash/hashpage.c @@ -1024,9 +1024,9 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks) zerobuf.data, true); - RelationOpenSmgr(rel); PageSetChecksumInplace(page, lastblock); - smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf.data, false); + smgrextend(RelationGetSmgr(rel), MAIN_FORKNUM, lastblock, zerobuf.data, + false); return true; } diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 1b8b640012..beb8f20708 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -625,7 +625,6 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode) SMgrRelation dstrel; dstrel = smgropen(*newrnode, rel->rd_backend); - RelationOpenSmgr(rel); /* * Since we copy the file directly without looking at the shared buffers, @@ -645,14 +644,14 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode) RelationCreateStorage(*newrnode, rel->rd_rel->relpersistence); /* copy main fork */ - RelationCopyStorage(rel->rd_smgr, dstrel, MAIN_FORKNUM, + RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM, rel->rd_rel->relpersistence); /* copy those extra forks that exist */ for (ForkNumber forkNum = MAIN_FORKNUM + 1; forkNum <= MAX_FORKNUM; forkNum++) { - if (smgrexists(rel->rd_smgr, forkNum)) + if (smgrexists(RelationGetSmgr(rel), forkNum)) { smgrcreate(dstrel, forkNum, false); @@ -664,7 +663,7 @@ heapam_relation_copy_data(Relation rel, const RelFileNode *newrnode) (rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED && forkNum == INIT_FORKNUM)) log_smgrcreate(newrnode, forkNum); - RelationCopyStorage(rel->rd_smgr, dstrel, forkNum, + RelationCopyStorage(RelationGetSmgr(rel), dstrel, forkNum, rel->rd_rel->relpersistence); } } diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c index 1aff62cd42..986a776bbd 100644 --- a/src/backend/access/heap/rewriteheap.c +++ b/src/backend/access/heap/rewriteheap.c @@ -326,9 +326,8 @@ end_heap_rewrite(RewriteState state) PageSetChecksumInplace(state->rs_buffer, state->rs_blockno); - RelationOpenSmgr(state->rs_new_rel); - smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, state->rs_blockno, - (char *) state->rs_buffer, true); + smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM, + state->rs_blockno, (char *) state->rs_buffer, true); } /* @@ -339,11 +338,7 @@ end_heap_rewrite(RewriteState state) * wrote before the checkpoint. */ if (RelationNeedsWAL(state->rs_new_rel)) - { - /* for an empty table, this could be first smgr access */ - RelationOpenSmgr(state->rs_new_rel); - smgrimmedsync(state->rs_new_rel->rd_smgr, MAIN_FORKNUM); - } + smgrimmedsync(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM); logical_end_heap_rewrite(state); @@ -695,11 +690,9 @@ raw_heap_insert(RewriteState state, HeapTuple tup) * need for smgr to schedule an fsync for this write; we'll do it * ourselves in end_heap_rewrite. */ - RelationOpenSmgr(state->rs_new_rel); - PageSetChecksumInplace(page, state->rs_blockno); - smgrextend(state->rs_new_rel->rd_smgr, MAIN_FORKNUM, + smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM, state->rs_blockno, (char *) page, true); state->rs_blockno++; diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index e198df65d8..4720b35ee5 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -455,13 +455,11 @@ visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks) elog(DEBUG1, "vm_truncate %s %d", RelationGetRelationName(rel), nheapblocks); #endif - RelationOpenSmgr(rel); - /* * If no visibility map has been created yet for this relation, there's * nothing to truncate. */ - if (!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM)) + if (!smgrexists(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM)) return InvalidBlockNumber; /* @@ -528,7 +526,7 @@ visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks) else newnblocks = truncBlock; - if (smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM) <= newnblocks) + if (smgrnblocks(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM) <= newnblocks) { /* nothing to do, the file was already smaller than requested size */ return InvalidBlockNumber; @@ -547,30 +545,29 @@ static Buffer vm_readbuf(Relation rel, BlockNumber blkno, bool extend) { Buffer buf; + SMgrRelation reln; /* - * We might not have opened the relation at the smgr level yet, or we - * might have been forced to close it by a sinval message. The code below - * won't necessarily notice relation extension immediately when extend = - * false, so we rely on sinval messages to ensure that our ideas about the - * size of the map aren't too far out of date. + * 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. */ - RelationOpenSmgr(rel); + reln = RelationGetSmgr(rel); /* * If we haven't cached the size of the visibility map fork yet, check it * first. */ - if (rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) + if (reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) { - if (smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM)) - smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM); + if (smgrexists(reln, VISIBILITYMAP_FORKNUM)) + smgrnblocks(reln, VISIBILITYMAP_FORKNUM); else - rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0; + reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = 0; } /* Handle requests beyond EOF */ - if (blkno >= rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM]) + if (blkno >= reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM]) { if (extend) vm_extend(rel, blkno + 1); @@ -618,6 +615,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks) { BlockNumber vm_nblocks_now; PGAlignedBlock pg; + SMgrRelation reln; PageInit((Page) pg.data, BLCKSZ, 0); @@ -633,29 +631,32 @@ vm_extend(Relation rel, BlockNumber vm_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 file first if it doesn't exist. If smgr_vm_nblocks is * positive then it must exist, no need for an smgrexists call. */ - if ((rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 || - rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) && - !smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM)) - smgrcreate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, false); + if ((reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == 0 || + reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] == InvalidBlockNumber) && + !smgrexists(reln, VISIBILITYMAP_FORKNUM)) + smgrcreate(reln, VISIBILITYMAP_FORKNUM, false); /* Invalidate cache so that smgrnblocks() asks the kernel. */ - rel->rd_smgr->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber; - vm_nblocks_now = smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM); + reln->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber; + vm_nblocks_now = smgrnblocks(reln, VISIBILITYMAP_FORKNUM); /* Now extend the file */ while (vm_nblocks_now < vm_nblocks) { PageSetChecksumInplace((Page) pg.data, vm_nblocks_now); - smgrextend(rel->rd_smgr, VISIBILITYMAP_FORKNUM, vm_nblocks_now, - pg.data, false); + smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now, pg.data, false); vm_nblocks_now++; } @@ -666,7 +667,7 @@ vm_extend(Relation rel, BlockNumber vm_nblocks) * to keep checking for creation or extension of the file, which happens * infrequently. */ - CacheInvalidateSmgr(rel->rd_smgr->smgr_rnode); + CacheInvalidateSmgr(reln->smgr_rnode); UnlockRelationForExtension(rel, ExclusiveLock); } diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c index 1360ab80c1..be23395dfb 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -163,9 +163,9 @@ btbuildempty(Relation index) * this even when wal_level=minimal. */ PageSetChecksumInplace(metapage, BTREE_METAPAGE); - smgrwrite(index->rd_smgr, INIT_FORKNUM, BTREE_METAPAGE, + smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BTREE_METAPAGE, (char *) metapage, true); - log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM, + log_newpage(&RelationGetSmgr(index)->smgr_rnode.node, INIT_FORKNUM, BTREE_METAPAGE, metapage, true); /* @@ -173,7 +173,7 @@ btbuildempty(Relation index) * write did not go through shared_buffers and therefore a concurrent * checkpoint may have moved the redo pointer past our xlog record. */ - smgrimmedsync(index->rd_smgr, INIT_FORKNUM); + smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM); } /* diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index 5fa6ea8ad9..54c8eb1289 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -637,9 +637,6 @@ _bt_blnewpage(uint32 level) static void _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno) { - /* Ensure rd_smgr is open (could have been closed by relcache flush!) */ - RelationOpenSmgr(wstate->index); - /* XLOG stuff */ if (wstate->btws_use_wal) { @@ -659,7 +656,7 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno) if (!wstate->btws_zeropage) wstate->btws_zeropage = (Page) palloc0(BLCKSZ); /* don't set checksum for all-zero page */ - smgrextend(wstate->index->rd_smgr, MAIN_FORKNUM, + smgrextend(RelationGetSmgr(wstate->index), MAIN_FORKNUM, wstate->btws_pages_written++, (char *) wstate->btws_zeropage, true); @@ -674,14 +671,14 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno) if (blkno == wstate->btws_pages_written) { /* extending the file... */ - smgrextend(wstate->index->rd_smgr, MAIN_FORKNUM, blkno, + smgrextend(RelationGetSmgr(wstate->index), MAIN_FORKNUM, blkno, (char *) page, true); wstate->btws_pages_written++; } else { /* overwriting a block we zero-filled before */ - smgrwrite(wstate->index->rd_smgr, MAIN_FORKNUM, blkno, + smgrwrite(RelationGetSmgr(wstate->index), MAIN_FORKNUM, blkno, (char *) page, true); } @@ -1428,10 +1425,7 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2) * still not be on disk when the crash occurs. */ if (wstate->btws_use_wal) - { - RelationOpenSmgr(wstate->index); - smgrimmedsync(wstate->index->rd_smgr, MAIN_FORKNUM); - } + smgrimmedsync(RelationGetSmgr(wstate->index), MAIN_FORKNUM); } /* diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c index 1af0af7da2..cc4394b1c8 100644 --- a/src/backend/access/spgist/spginsert.c +++ b/src/backend/access/spgist/spginsert.c @@ -169,27 +169,27 @@ spgbuildempty(Relation index) * replayed. */ PageSetChecksumInplace(page, SPGIST_METAPAGE_BLKNO); - smgrwrite(index->rd_smgr, INIT_FORKNUM, SPGIST_METAPAGE_BLKNO, + smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_METAPAGE_BLKNO, (char *) page, true); - log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM, + log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM, SPGIST_METAPAGE_BLKNO, page, true); /* Likewise for the root page. */ SpGistInitPage(page, SPGIST_LEAF); PageSetChecksumInplace(page, SPGIST_ROOT_BLKNO); - smgrwrite(index->rd_smgr, INIT_FORKNUM, SPGIST_ROOT_BLKNO, + smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_ROOT_BLKNO, (char *) page, true); - log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM, + log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM, SPGIST_ROOT_BLKNO, page, true); /* Likewise for the null-tuples root page. */ SpGistInitPage(page, SPGIST_LEAF | SPGIST_NULLS); PageSetChecksumInplace(page, SPGIST_NULL_BLKNO); - smgrwrite(index->rd_smgr, INIT_FORKNUM, SPGIST_NULL_BLKNO, + smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_NULL_BLKNO, (char *) page, true); - log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM, + log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM, SPGIST_NULL_BLKNO, page, true); /* @@ -197,7 +197,7 @@ spgbuildempty(Relation index) * writes did not go through shared buffers and therefore a concurrent * checkpoint may have moved the redo pointer past our xlog record. */ - smgrimmedsync(index->rd_smgr, INIT_FORKNUM); + smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM); } /* diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c index 5ea5bdd810..66f0f84386 100644 --- a/src/backend/access/table/tableam.c +++ b/src/backend/access/table/tableam.c @@ -629,17 +629,14 @@ table_block_relation_size(Relation rel, ForkNumber forkNumber) { uint64 nblocks = 0; - /* Open it at the smgr level if not already done */ - RelationOpenSmgr(rel); - /* InvalidForkNumber indicates returning the size for all forks */ if (forkNumber == InvalidForkNumber) { for (int i = 0; i < MAX_FORKNUM; i++) - nblocks += smgrnblocks(rel->rd_smgr, i); + nblocks += smgrnblocks(RelationGetSmgr(rel), i); } else - nblocks = smgrnblocks(rel->rd_smgr, forkNumber); + nblocks = smgrnblocks(RelationGetSmgr(rel), forkNumber); return nblocks * BLCKSZ; } diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 09370a8a5a..83746d3fd9 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -415,8 +415,6 @@ heap_create(const char *relname, */ if (create_storage) { - RelationOpenSmgr(rel); - switch (rel->rd_rel->relkind) { case RELKIND_VIEW: diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 50b7a16bce..26bfa74ce7 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2982,10 +2982,9 @@ index_build(Relation heapRelation, * relfilenode won't change, and nothing needs to be done here. */ if (indexRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED && - !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM)) + !smgrexists(RelationGetSmgr(indexRelation), INIT_FORKNUM)) { - RelationOpenSmgr(indexRelation); - smgrcreate(indexRelation->rd_smgr, INIT_FORKNUM, false); + smgrcreate(RelationGetSmgr(indexRelation), INIT_FORKNUM, false); indexRelation->rd_indam->ambuildempty(indexRelation); } diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c index cba7a9ada0..c5ad28d71f 100644 --- a/src/backend/catalog/storage.c +++ b/src/backend/catalog/storage.c @@ -282,16 +282,16 @@ RelationTruncate(Relation rel, BlockNumber nblocks) ForkNumber forks[MAX_FORKNUM]; BlockNumber blocks[MAX_FORKNUM]; int nforks = 0; - - /* Open it at the smgr level if not already done */ - RelationOpenSmgr(rel); + SMgrRelation reln; /* - * Make sure smgr_targblock etc aren't pointing somewhere past new end + * Make sure smgr_targblock etc aren't pointing somewhere past new end. + * (Note: don't rely on this reln pointer below this loop.) */ - rel->rd_smgr->smgr_targblock = InvalidBlockNumber; + reln = RelationGetSmgr(rel); + reln->smgr_targblock = InvalidBlockNumber; for (int i = 0; i <= MAX_FORKNUM; ++i) - rel->rd_smgr->smgr_cached_nblocks[i] = InvalidBlockNumber; + reln->smgr_cached_nblocks[i] = InvalidBlockNumber; /* Prepare for truncation of MAIN fork of the relation */ forks[nforks] = MAIN_FORKNUM; @@ -299,7 +299,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) nforks++; /* Prepare for truncation of the FSM if it exists */ - fsm = smgrexists(rel->rd_smgr, FSM_FORKNUM); + fsm = smgrexists(RelationGetSmgr(rel), FSM_FORKNUM); if (fsm) { blocks[nforks] = FreeSpaceMapPrepareTruncateRel(rel, nblocks); @@ -312,7 +312,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) } /* Prepare for truncation of the visibility map too if it exists */ - vm = smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM); + vm = smgrexists(RelationGetSmgr(rel), VISIBILITYMAP_FORKNUM); if (vm) { blocks[nforks] = visibilitymap_prepare_truncate(rel, nblocks); @@ -364,7 +364,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) } /* Do the real work to truncate relation forks */ - smgrtruncate(rel->rd_smgr, forks, nforks, blocks); + smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks); /* * Update upper-level FSM pages to account for the truncation. This is @@ -389,9 +389,9 @@ RelationPreTruncate(Relation rel) if (!pendingSyncHash) return; - RelationOpenSmgr(rel); - pending = hash_search(pendingSyncHash, &(rel->rd_smgr->smgr_rnode.node), + pending = hash_search(pendingSyncHash, + &(RelationGetSmgr(rel)->smgr_rnode.node), HASH_FIND, NULL); if (pending) pending->is_truncated = true; @@ -403,6 +403,12 @@ RelationPreTruncate(Relation rel) * Note that this requires that there is no dirty data in shared buffers. If * it's possible that there are, callers need to flush those using * e.g. FlushRelationBuffers(rel). + * + * Also note that this is frequently called via locutions such as + * RelationCopyStorage(RelationGetSmgr(rel), ...); + * That's safe only because we perform only smgr and WAL operations here. + * If we invoked anything else, a relcache flush could cause our SMgrRelation + * argument to become a dangling pointer. */ void RelationCopyStorage(SMgrRelation src, SMgrRelation dst, @@ -445,13 +451,23 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst, if (!PageIsVerifiedExtended(page, blkno, PIV_LOG_WARNING | PIV_REPORT_STAT)) + { + /* + * For paranoia's sake, capture the file path before invoking the + * ereport machinery. This guards against the possibility of a + * relcache flush caused by, e.g., an errcontext callback. + * (errcontext callbacks shouldn't be risking any such thing, but + * people have been known to forget that rule.) + */ + char *relpath = relpathbackend(src->smgr_rnode.node, + src->smgr_rnode.backend, + forkNum); + ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), errmsg("invalid page in block %u of relation %s", - blkno, - relpathbackend(src->smgr_rnode.node, - src->smgr_rnode.backend, - forkNum)))); + blkno, relpath))); + } /* * WAL-log the copied page. Unfortunately we don't know what kind of a diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 03dfd2e7fa..dcf14b9dc0 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -14151,7 +14151,6 @@ index_copy_data(Relation rel, RelFileNode newrnode) SMgrRelation dstrel; dstrel = smgropen(newrnode, rel->rd_backend); - RelationOpenSmgr(rel); /* * Since we copy the file directly without looking at the shared buffers, @@ -14171,14 +14170,14 @@ index_copy_data(Relation rel, RelFileNode newrnode) RelationCreateStorage(newrnode, rel->rd_rel->relpersistence); /* copy main fork */ - RelationCopyStorage(rel->rd_smgr, dstrel, MAIN_FORKNUM, + RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM, rel->rd_rel->relpersistence); /* copy those extra forks that exist */ for (ForkNumber forkNum = MAIN_FORKNUM + 1; forkNum <= MAX_FORKNUM; forkNum++) { - if (smgrexists(rel->rd_smgr, forkNum)) + if (smgrexists(RelationGetSmgr(rel), forkNum)) { smgrcreate(dstrel, forkNum, false); @@ -14190,7 +14189,7 @@ index_copy_data(Relation rel, RelFileNode newrnode) (rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED && forkNum == INIT_FORKNUM)) log_smgrcreate(&newrnode, forkNum); - RelationCopyStorage(rel->rd_smgr, dstrel, forkNum, + RelationCopyStorage(RelationGetSmgr(rel), dstrel, forkNum, rel->rd_rel->relpersistence); } } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 4b296a22c4..86ef607ff3 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -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); } diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c index 8c12dda238..09d4b16067 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++; } diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index 77d176a934..b4faa1c123 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -24,6 +24,7 @@ #include "rewrite/prs2lock.h" #include "storage/block.h" #include "storage/relfilenode.h" +#include "storage/smgr.h" #include "utils/relcache.h" #include "utils/reltrigger.h" @@ -53,8 +54,7 @@ typedef LockInfoData *LockInfo; typedef struct RelationData { RelFileNode rd_node; /* relation physical identifier */ - /* use "struct" here to avoid needing to include smgr.h: */ - struct SMgrRelationData *rd_smgr; /* cached file handle, or NULL */ + SMgrRelation rd_smgr; /* cached file handle, or NULL */ int rd_refcnt; /* reference count */ BackendId rd_backend; /* owning backend id, if temporary relation */ bool rd_islocaltemp; /* rel is a temp rel of this session */ @@ -528,14 +528,25 @@ typedef struct ViewOptions ((relation)->rd_rel->relfilenode == InvalidOid)) /* - * RelationOpenSmgr - * Open the relation at the smgr level, if not already done. + * RelationGetSmgr + * Returns smgr file handle for a relation, opening it if needed. + * + * Very little code is authorized to touch rel->rd_smgr directly. Instead + * use this function to fetch its value. + * + * Note: since a relcache flush can cause the file handle to be closed again, + * it's unwise to hold onto the pointer returned by this function for any + * long period. Recommended practice is to just re-execute RelationGetSmgr + * each time you need to access the SMgrRelation. It's quite cheap in + * comparison to whatever an smgr function is going to do. */ -#define RelationOpenSmgr(relation) \ - do { \ - if ((relation)->rd_smgr == NULL) \ - smgrsetowner(&((relation)->rd_smgr), smgropen((relation)->rd_node, (relation)->rd_backend)); \ - } while (0) +static inline SMgrRelation +RelationGetSmgr(Relation rel) +{ + if (unlikely(rel->rd_smgr == NULL)) + smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_node, rel->rd_backend)); + return rel->rd_smgr; +} /* * RelationCloseSmgr @@ -557,7 +568,8 @@ typedef struct ViewOptions * Fetch relation's current insertion target block. * * Returns InvalidBlockNumber if there is no current target block. Note - * that the target block status is discarded on any smgr-level invalidation. + * that the target block status is discarded on any smgr-level invalidation, + * so there's no need to re-open the smgr handle if it's not currently open. */ #define RelationGetTargetBlock(relation) \ ( (relation)->rd_smgr != NULL ? (relation)->rd_smgr->smgr_targblock : InvalidBlockNumber ) @@ -568,8 +580,7 @@ typedef struct ViewOptions */ #define RelationSetTargetBlock(relation, targblock) \ do { \ - RelationOpenSmgr(relation); \ - (relation)->rd_smgr->smgr_targblock = (targblock); \ + RelationGetSmgr(relation)->smgr_targblock = (targblock); \ } while (0) /*