Avoid using a fake relcache entry to own an SmgrRelation.

If an error occurs before we close the fake relcache entry, the the
fake relcache entry will be destroyed by the SmgrRelation will
survive until end of transaction. Its smgr_owner pointer ends up
pointing to already-freed memory.

The original reason for using a fake relcache entry here was to try
to avoid reusing an SMgrRelation across a relevant invalidation. To
avoid that problem, just call smgropen() again each time we need a
reference to it. Hopefully someday we will come up with a more
elegant approach, but accessing uninitialized memory is bad so let's
do this for now.

Dilip Kumar, reviewed by Andres Freund and Tom Lane. Report by
Justin Pryzby.

Discussion: http://postgr.es/m/20220802175043[email protected]
Discussion: http://postgr.es/m/CAFiTN-vSFeE6_W9z698XNtFROOA_nSqUXWqLcG0emob_kJ+dEQ@mail.gmail.com

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 9a563b239c6f5724cad63e6ddcd409fa301d514d..6f836865e15f0e4d32020d52536fec5724d309f0 100644 (file)
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -257,8 +257,8 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
    Page        page;
    List       *rnodelist = NIL;
    LockRelId   relid;
-   Relation    rel;
    Snapshot    snapshot;
+   SMgrRelation    smgr;
    BufferAccessStrategy bstrategy;
 
    /* Get pg_class relfilenode. */
@@ -275,16 +275,9 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
    rnode.dbNode = dbid;
    rnode.relNode = relfilenode;
 
-   /*
-    * We can't use a real relcache entry for a relation in some other
-    * database, but since we're only going to access the fields related to
-    * physical storage, a fake one is good enough. If we didn't do this and
-    * used the smgr layer directly, we would have to worry about
-    * invalidations.
-    */
-   rel = CreateFakeRelcacheEntry(rnode);
-   nblocks = smgrnblocks(RelationGetSmgr(rel), MAIN_FORKNUM);
-   FreeFakeRelcacheEntry(rel);
+   smgr = smgropen(rnode, InvalidBackendId);
+   nblocks = smgrnblocks(smgr, MAIN_FORKNUM);
+   smgrclose(smgr);
 
    /* Use a buffer access strategy since this is a bulk read operation. */
    bstrategy = GetAccessStrategy(BAS_BULKREAD);

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 8aabf5991b0e78017823ffc25789150235d5e7f6..43d3c8caaa68893c7db39561550a3decde677933 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -487,9 +487,9 @@ static void FindAndDropRelFileNodeBuffers(RelFileNode rnode,
                                          ForkNumber forkNum,
                                          BlockNumber nForkBlock,
                                          BlockNumber firstDelBlock);
-static void RelationCopyStorageUsingBuffer(Relation src, Relation dst,
-                                          ForkNumber forkNum,
-                                          bool isunlogged);
+static void RelationCopyStorageUsingBuffer(RelFileNode srcnode,
+                                          RelFileNode dstnode,
+                                          ForkNumber forkNum, bool permanent);
 static void AtProcExit_Buffers(int code, Datum arg);
 static void CheckForBufferLeaks(void);
 static int rnode_comparator(const void *p1, const void *p2);
@@ -3702,8 +3702,9 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
  * --------------------------------------------------------------------
  */
 static void
-RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
-                              bool permanent)
+RelationCopyStorageUsingBuffer(RelFileNode srcnode,
+                              RelFileNode dstnode,
+                              ForkNumber forkNum, bool permanent)
 {
    Buffer      srcBuf;
    Buffer      dstBuf;
@@ -3723,7 +3724,8 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
    use_wal = XLogIsNeeded() && (permanent || forkNum == INIT_FORKNUM);
 
    /* Get number of blocks in the source relation. */
-   nblocks = smgrnblocks(RelationGetSmgr(src), forkNum);
+   nblocks = smgrnblocks(smgropen(srcnode, InvalidBackendId),
+                         forkNum);
 
    /* Nothing to copy; just return. */
    if (nblocks == 0)
@@ -3739,14 +3741,14 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
        CHECK_FOR_INTERRUPTS();
 
        /* Read block from source relation. */
-       srcBuf = ReadBufferWithoutRelcache(src->rd_node, forkNum, blkno,
+       srcBuf = ReadBufferWithoutRelcache(srcnode, forkNum, blkno,
                                           RBM_NORMAL, bstrategy_src,
                                           permanent);
        LockBuffer(srcBuf, BUFFER_LOCK_SHARE);
        srcPage = BufferGetPage(srcBuf);
 
        /* Use P_NEW to extend the destination relation. */
-       dstBuf = ReadBufferWithoutRelcache(dst->rd_node, forkNum, P_NEW,
+       dstBuf = ReadBufferWithoutRelcache(dstnode, forkNum, P_NEW,
                                           RBM_NORMAL, bstrategy_dst,
                                           permanent);
        LockBuffer(dstBuf, BUFFER_LOCK_EXCLUSIVE);
@@ -3784,24 +3786,13 @@ void
 CreateAndCopyRelationData(RelFileNode src_rnode, RelFileNode dst_rnode,
                          bool permanent)
 {
-   Relation    src_rel;
-   Relation    dst_rel;
+   RelFileNodeBackend rnode;
    char        relpersistence;
 
    /* Set the relpersistence. */
    relpersistence = permanent ?
        RELPERSISTENCE_PERMANENT : RELPERSISTENCE_UNLOGGED;
 
-   /*
-    * We can't use a real relcache entry for a relation in some other
-    * database, but since we're only going to access the fields related to
-    * physical storage, a fake one is good enough. If we didn't do this and
-    * used the smgr layer directly, we would have to worry about
-    * invalidations.
-    */
-   src_rel = CreateFakeRelcacheEntry(src_rnode);
-   dst_rel = CreateFakeRelcacheEntry(dst_rnode);
-
    /*
     * Create and copy all forks of the relation.  During create database we
     * have a separate cleanup mechanism which deletes complete database
@@ -3811,15 +3802,16 @@ CreateAndCopyRelationData(RelFileNode src_rnode, RelFileNode dst_rnode,
    RelationCreateStorage(dst_rnode, relpersistence, false);
 
    /* copy main fork. */
-   RelationCopyStorageUsingBuffer(src_rel, dst_rel, MAIN_FORKNUM, permanent);
+   RelationCopyStorageUsingBuffer(src_rnode, dst_rnode, MAIN_FORKNUM,
+                                  permanent);
 
    /* copy those extra forks that exist */
    for (ForkNumber forkNum = MAIN_FORKNUM + 1;
         forkNum <= MAX_FORKNUM; forkNum++)
    {
-       if (smgrexists(RelationGetSmgr(src_rel), forkNum))
+       if (smgrexists(smgropen(src_rnode, InvalidBackendId), forkNum))
        {
-           smgrcreate(RelationGetSmgr(dst_rel), forkNum, false);
+           smgrcreate(smgropen(dst_rnode, InvalidBackendId), forkNum, false);
 
            /*
             * WAL log creation if the relation is persistent, or this is the
@@ -3829,14 +3821,19 @@ CreateAndCopyRelationData(RelFileNode src_rnode, RelFileNode dst_rnode,
                log_smgrcreate(&dst_rnode, forkNum);
 
            /* Copy a fork's data, block by block. */
-           RelationCopyStorageUsingBuffer(src_rel, dst_rel, forkNum,
+           RelationCopyStorageUsingBuffer(src_rnode, dst_rnode, forkNum,
                                           permanent);
        }
    }
 
-   /* Release fake relcache entries. */
-   FreeFakeRelcacheEntry(src_rel);
-   FreeFakeRelcacheEntry(dst_rel);
+   /* close source and destination smgr if exists. */
+   rnode.backend = InvalidBackendId;
+
+   rnode.node = src_rnode;
+   smgrclosenode(rnode);
+
+   rnode.node = dst_rnode;
+   smgrclosenode(rnode);
 }
 
 /* ---------------------------------------------------------------------