On Fri, Aug 5, 2022 at 2:59 AM Andres Freund <and...@anarazel.de> wrote: > > Hi, > > On 2022-08-03 16:45:23 +0530, Dilip Kumar wrote: > > Another version of the patch which closes the smgr at the end using > > smgrcloserellocator() and I have also added a commit message. > > What's the motivation behind the explicit close? > > > > @@ -258,8 +258,8 @@ ScanSourceDatabasePgClass(Oid tbid, Oid dbid, char > > *srcpath) > > Page page; > > List *rlocatorlist = NIL; > > LockRelId relid; > > - Relation rel; > > Snapshot snapshot; > > + SMgrRelation smgr; > > BufferAccessStrategy bstrategy; > > > > /* Get pg_class relfilenumber. */ > > @@ -276,16 +276,9 @@ 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); > > + smgr = smgropen(rlocator, InvalidBackendId); > > + nblocks = smgrnblocks(smgr, MAIN_FORKNUM); > > + smgrclose(smgr); > > Why are you opening and then closing again? Part of the motivation for the > question is that a local SMgrRelation variable may lead to it being used > further, opening up interrupt processing issues.
Yeah okay, I think there is no reason to close, in the previous version I had like below and I think that's a better idea. nblocks = smgrnblocks(smgropen(rlocator, InvalidBackendId), MAIN_FORKNUM) > > > + rlocator.locator = src_rlocator; > > + smgrcloserellocator(rlocator); > > + > > + rlocator.locator = dst_rlocator; > > + smgrcloserellocator(rlocator); > > As mentioned above, it's not clear to me why this is now done... > > Otherwise looks good to me. 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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
