On Fri, Aug 5, 2022 at 10:43 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> Yeah maybe it is not necessary to close as these unowned smgr will
> automatically get closed on the transaction end. Actually the
> previous person of the patch had both these comments fixed. The
> reason for explicitly closing it is that I have noticed that most of
> the places we explicitly close the smgr where we do smgropen e.g.
> index_copy_data(), heapam_relation_copy_data() OTOH some places we
> don't close it e.g. IssuePendingWritebacks(). So now I think that in
> our case better we do not close it because I do not like this specific
> code at the end to close the smgr.
PFA patches for different problems discussed in the thread
0001 - Fix the problem of skipping the empty block and buffer lock on
source buffer
0002 - Remove fake relcache entry (same as 0001-BugfixInWalLogCreateDB.patch)
0003 - Optimization to avoid extending block by block
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 8a71e5dd10ff65d250815dc17f8f64212c2e57b0 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.ku...@enterprisedb.com>
Date: Fri, 5 Aug 2022 11:25:23 +0530
Subject: [PATCH v2 3/3] Optimize copy storage from source to destination
Instead of extending block at a time directly bulkextend the destination
relation and then just perform the block level copy.
---
src/backend/storage/buffer/bufmgr.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index b488306..b7df980 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3710,6 +3710,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
Page srcPage;
Page dstPage;
bool use_wal;
+ char buffer[BLCKSZ];
BlockNumber nblocks;
BlockNumber blkno;
BufferAccessStrategy bstrategy_src;
@@ -3730,6 +3731,14 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
if (nblocks == 0)
return;
+ /*
+ * Bulk extend the destination relation of the same size as the source
+ * relation before starting to copy block by block.
+ */
+ memset(buffer, 0, BLCKSZ);
+ smgrextend(smgropen(dstlocator, InvalidBackendId), forkNum, nblocks - 1,
+ buffer, true);
+
/* This is a bulk operation, so use buffer access strategies. */
bstrategy_src = GetAccessStrategy(BAS_BULKREAD);
bstrategy_dst = GetAccessStrategy(BAS_BULKWRITE);
@@ -3748,7 +3757,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
srcPage = BufferGetPage(srcBuf);
/* Use P_NEW to extend the destination relation. */
- dstBuf = ReadBufferWithoutRelcache(dstlocator, forkNum, P_NEW,
+ dstBuf = ReadBufferWithoutRelcache(dstlocator, forkNum, blkno,
RBM_NORMAL, bstrategy_dst,
permanent);
LockBuffer(dstBuf, BUFFER_LOCK_EXCLUSIVE);
--
1.8.3.1
From 85d17fb7c91fdb6b40f09d00e9a5606ba2c90e57 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.ku...@enterprisedb.com>
Date: Fri, 5 Aug 2022 10:59:18 +0530
Subject: [PATCH v2 2/3] Avoid setting the fake relcache entry as smgr owner
MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
During CREATE DATABASE, we are not connected to the source and the
destination DB so in order to operate on the storage we are using
FakeRelCacheEntry and by using that we are calling RelationGetSmgr().
So the problem is that this function will set the temporary
FakeRelCacheEntry as an owner of the smgr. Now if there is any
error before we close the FakeRelCacheEntry then the memory of the
fake relcache entry will be released at the transaction abort but
the smgr will survive the transaction. So now smgr is pointing
to some already release memory and it will have random behavior
when we try to access the smgr next time.
For fixing the issue instead of using the FakeRelCacheEntry, directly
call the smgropen() but do not keep the reference to the smgr.
So every time call smgropen() whenever we need it. This is required to
ensure that we do not access the smgr pointer which might have already
been closed during interrupt processing.
---
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 9f990a8..88d4fe1 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 7f992c3..b488306 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);
@@ -3746,7 +3748,7 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
srcPage = BufferGetPage(srcBuf);
/* 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);
@@ -3784,8 +3786,6 @@ void
CreateAndCopyRelationData(RelFileLocator src_rlocator,
RelFileLocator dst_rlocator, bool permanent)
{
- Relation src_rel;
- Relation dst_rel;
char relpersistence;
/* Set the relpersistence. */
@@ -3793,16 +3793,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
@@ -3811,15 +3801,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
@@ -3829,14 +3820,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
From fb4bd9f9aff0c51e5f576742a84b40f7cebb6872 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.ku...@enterprisedb.com>
Date: Fri, 5 Aug 2022 10:51:04 +0530
Subject: [PATCH v2 1/3] Assorted bug fixes while coying the storage during
create database
While copying the storage the code is skipping the new/empty pages
which could create corrupted storage as that could have broken ctid
links and ther such issue. Also fix the missing buffer lock on the
destination buffer.
---
src/backend/storage/buffer/bufmgr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 6b30138..7f992c3 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3741,23 +3741,20 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
srcBuf = ReadBufferWithoutRelcache(src->rd_locator, forkNum, blkno,
RBM_NORMAL, bstrategy_src,
permanent);
+
+ LockBuffer(srcBuf, BUFFER_LOCK_SHARE);
srcPage = BufferGetPage(srcBuf);
- if (PageIsNew(srcPage) || PageIsEmpty(srcPage))
- {
- ReleaseBuffer(srcBuf);
- continue;
- }
/* Use P_NEW to extend the destination relation. */
dstBuf = ReadBufferWithoutRelcache(dst->rd_locator, forkNum, P_NEW,
RBM_NORMAL, bstrategy_dst,
permanent);
LockBuffer(dstBuf, BUFFER_LOCK_EXCLUSIVE);
+ dstPage = BufferGetPage(dstBuf);
START_CRIT_SECTION();
/* Copy page data from the source to the destination. */
- dstPage = BufferGetPage(dstBuf);
memcpy(dstPage, srcPage, BLCKSZ);
MarkBufferDirty(dstBuf);
@@ -3767,8 +3764,8 @@ RelationCopyStorageUsingBuffer(Relation src, Relation dst, ForkNumber forkNum,
END_CRIT_SECTION();
+ UnlockReleaseBuffer(srcBuf);
UnlockReleaseBuffer(dstBuf);
- ReleaseBuffer(srcBuf);
}
}
--
1.8.3.1
