Horton

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

John Naylor <john.naylor@enterprisedb.com>

View thread View on postgr.es
From: John Naylor <john.naylor@enterprisedb.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Heikki Linnakangas <hlinnaka@iki.fi>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-11-23T16:59:00Z
Lists: pgsql-hackers 
Hi,

I've looked over this patch set and email thread a couple times, and I
don't see anything amiss, but I'm also not terribly familiar with the
subsystems this part of the code relies on. I haven't yet tried to stress
test with a large database, but it seems like a good idea to do so.

I have a couple comments and questions:

0006:

+ * XXX We can optimize RelationMapOidToFileenodeForDatabase API
+ * so that instead of reading the relmap file every time, it can
+ * save it in a temporary variable and use it for subsequent
+ * calls.  Then later reset it once we're done or at the
+ * transaction end.

Do we really need to consider optimizing here? Only a handful of relations
will be found in the relmap, right?

+ * Once we start copying files from the source database, we need to be able
+ * to clean 'em up if we fail.  Use an ENSURE block to make sure this
+ * happens.  (This is not a 100% solution, because of the possibility of
+ * failure during transaction commit after we leave this routine, but it
+ * should handle most scenarios.)

This comment in master started with

- * Once we start copying subdirectories, we need to be able to clean 'em

Is the distinction important enough to change this comment? Also, is "most
scenarios" still true with the patch? I haven't read into how ENSURE works.

Same with this comment change, seems fine the way it was:

- * Use an ENSURE block to make sure we remove the debris if the copy fails
- * (eg, due to out-of-disk-space).  This is not a 100% solution, because
- * of the possibility of failure during transaction commit, but it should
- * handle most scenarios.
+ * Use an ENSURE block to make sure we remove the debris if the copy fails.
+ * This is not a 100% solution, because of the possibility of failure
+ * during transaction commit, but it should handle most scenarios.

And do we need additional tests? Maybe we don't, but it seems good to make
sure.

I haven't looked at 0007, and I have no opinion on which approach is better.

-- 
John Naylor
EDB: http://www.enterprisedb.com

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.