Horton

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

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, Maciek Sakrejda <m.sakrejda@gmail.com>, Bruce Momjian <bruce@momjian.us>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Andrew Dunstan <andrew@dunslane.net>, Heikki Linnakangas <hlinnaka@iki.fi>, pgsql-hackers@lists.postgresql.org
Date: 2022-08-04T21:29:32Z
Lists: pgsql-hackers 
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

Commits

  1. When using the WAL-logged CREATE DATABASE strategy, bulk extend.

  2. Avoid using a fake relcache entry to own an SmgrRelation.

  3. Fix data-corruption hazard in WAL-logged CREATE DATABASE.

  4. initdb: When running CREATE DATABASE, use STRATEGY = WAL_COPY.

  5. Simplify a needlessly-complicated regular expression.

  6. In 020_createdb.pl, change order of command-line arguments.

  7. Add new block-by-block strategy for CREATE DATABASE.

  8. Fix replay of create database records on standby

  9. Refactor code for reading and writing relation map files.

  10. Replace RelationOpenSmgr() with RelationGetSmgr().

  11. Refactor the fsync queue for wider use.