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