On Wed, Aug 3, 2022 at 12:00 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> Okay, so AtEOXact_SMgr will only get rid of unowned smgr and ours are
> owned by a fake relcache and whose lifetime is just portal memory
> context which will go away at the transaction end. So as Andres
> suggested options could be that a) we catch the error and do
> FreeFakeRelcacheEntry. b) directly use smgropen instead of
> RelationGetSmgr because actually, we do not want the owner to be set
> for these smgrs.
>
> I think option b) looks better to me, I will prepare a patch and test
> whether the error goes away with that or not.
>
Here is the patch which directly uses smgropen instead of using fake
relcache entry. We don't preserve the smgr pointer and whenever
required we again call the smgropen.
With this patch it resolved the problem for me at least what I was
able to reproduce. I was able to reproduce the WARNING messages that
Robert got as well as the valgrind error that Justin got and with this
patch both are resolved.
@Justin can you help in verifying the original issue?
Another alternative could be that continue using fake relcache entry
but instead of RelationGetSmgr() create some new function which
doesn't set the owner in the smgr.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From aa1f6ff66f1c4bc41ffbc066a5543e7030a4501f Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.ku...@enterprisedb.com>
Date: Wed, 3 Aug 2022 13:28:46 +0530
Subject: [PATCH] BugfixInWalLogCreateDB
---
src/backend/commands/dbcommands.c | 12 +----------
src/backend/storage/buffer/bufmgr.c | 43 +++++++++++++------------------------
2 files changed, 16 insertions(+), 39 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 7bc53f3..0423831 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -258,7 +258,6 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
Page page;
List *rlocatorlist = NIL;
LockRelId relid;
- Relation rel;
Snapshot snapshot;
BufferAccessStrategy bstrategy;
@@ -276,16 +275,7 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char *srcpath)
rlocator.dbOid = dbid;
rlocator.relNumber = relfilenumber;
- /*
- * 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(rlocator);
- nblocks = smgrnblocks(RelationGetSmgr(rel), MAIN_FORKNUM);
- FreeFakeRelcacheEntry(rel);
+ nblocks = smgrnblocks(smgropen(rlocator, InvalidBackendId), MAIN_FORKNUM);
/* 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 6b30138..5f6e242 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -487,9 +487,9 @@ static void FindAndDropRelationBuffers(RelFileLocator rlocator,
ForkNumber forkNum,
BlockNumber nForkBlock,
BlockNumber firstDelBlock);
-static void RelationCopyStorageUsingBuffer(Relation src, Relation dst,
- ForkNumber forkNum,
- bool isunlogged);
+static void RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
+ RelFileLocator dstlocator,
+ ForkNumber forkNum, bool permanent);
static void AtProcExit_Buffers(int code, Datum arg);
static void CheckForBufferLeaks(void);
static int rlocator_comparator(const void *p1, const void *p2);
@@ -3701,8 +3701,9 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
* --------------------------------------------------------------------
*/
static void
-RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
- bool permanent)
+RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
+ RelFileLocator dstlocator,
+ ForkNumber forkNum, bool permanent)
{
Buffer srcBuf;
Buffer dstBuf;
@@ -3722,7 +3723,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(srclocator, InvalidBackendId),
+ forkNum);
/* Nothing to copy; just return. */
if (nblocks == 0)
@@ -3738,7 +3740,7 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
CHECK_FOR_INTERRUPTS();
/* Read block from source relation. */
- srcBuf = ReadBufferWithoutRelcache(src->rd_locator, forkNum, blkno,
+ srcBuf = ReadBufferWithoutRelcache(srclocator, forkNum, blkno,
RBM_NORMAL, bstrategy_src,
permanent);
srcPage = BufferGetPage(srcBuf);
@@ -3749,7 +3751,7 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
}
/* Use P_NEW to extend the destination relation. */
- dstBuf = ReadBufferWithoutRelcache(dst->rd_locator, forkNum, P_NEW,
+ dstBuf = ReadBufferWithoutRelcache(dstlocator, forkNum, P_NEW,
RBM_NORMAL, bstrategy_dst,
permanent);
LockBuffer(dstBuf, BUFFER_LOCK_EXCLUSIVE);
@@ -3787,8 +3789,6 @@ void
CreateAndCopyRelationData(RelFileLocator src_rlocator,
RelFileLocator dst_rlocator, bool permanent)
{
- Relation src_rel;
- Relation dst_rel;
char relpersistence;
/* Set the relpersistence. */
@@ -3796,16 +3796,6 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator,
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_rlocator);
- dst_rel = CreateFakeRelcacheEntry(dst_rlocator);
-
- /*
* Create and copy all forks of the relation. During create database we
* have a separate cleanup mechanism which deletes complete database
* directory. Therefore, each individual relation doesn't need to be
@@ -3814,15 +3804,16 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator,
RelationCreateStorage(dst_rlocator, relpersistence, false);
/* copy main fork. */
- RelationCopyStorageUsingBuffer(src_rel, dst_rel, MAIN_FORKNUM, permanent);
+ RelationCopyStorageUsingBuffer(src_rlocator, dst_rlocator, 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_rlocator, InvalidBackendId), forkNum))
{
- smgrcreate(RelationGetSmgr(dst_rel), forkNum, false);
+ smgrcreate(smgropen(dst_rlocator, InvalidBackendId), forkNum, false);
/*
* WAL log creation if the relation is persistent, or this is the
@@ -3832,14 +3823,10 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator,
log_smgrcreate(&dst_rlocator, forkNum);
/* Copy a fork's data, block by block. */
- RelationCopyStorageUsingBuffer(src_rel, dst_rel, forkNum,
+ RelationCopyStorageUsingBuffer(src_rlocator, dst_rlocator, forkNum,
permanent);
}
}
-
- /* Release fake relcache entries. */
- FreeFakeRelcacheEntry(src_rel);
- FreeFakeRelcacheEntry(dst_rel);
}
/* ---------------------------------------------------------------------
--
1.8.3.1
