Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

On Fri, Aug 5, 2022 at 2:59 AM Andres Freund <andres@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