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

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.


> +    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.

Greetings,

Andres Freund