v3-0001-skip-copydir-if-either-src-directory-or-dst-direc.patch

From 968adb96f7440386e5ee70f66fbd7495068a09e6 Mon Sep 17 00:00:00 2001
From: Paul Guo <paulguo@gmail.com>
Date: Tue, 30 Apr 2019 13:30:49 +0800
Subject: [PATCH v3] skip copydir() if either src directory or dst directory is
 missing due to re-redoing create database but the tablespace is dropped.

Also correct dbase_desc() so that related xlog description is not misleading.

Add tap tests for the previous tablespace patch.

One of the test and related change in PostgresNode.pm are actually
from community (Kyotaro HORIGUCHI). I added another test and modified
PostgresNode.pm further to suport my added test.

This patch uses the log/forget mechanism to avoid bad ignoring.
---
 src/backend/access/rmgrdesc/dbasedesc.c   |  14 ++-
 src/backend/access/transam/xlog.c         |   4 +
 src/backend/commands/dbcommands.c         | 107 +++++++++++++++++++++-
 src/include/commands/dbcommands.h         |   2 +
 src/test/perl/PostgresNode.pm             |  13 ++-
 src/test/perl/RecursiveCopy.pm            |  33 ++++++-
 src/test/recovery/t/011_crash_recovery.pl |  99 +++++++++++++++++++-
 7 files changed, 259 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/rmgrdesc/dbasedesc.c b/src/backend/access/rmgrdesc/dbasedesc.c
index c7d60ce10d..35092ffb0e 100644
--- a/src/backend/access/rmgrdesc/dbasedesc.c
+++ b/src/backend/access/rmgrdesc/dbasedesc.c
@@ -23,21 +23,25 @@ dbase_desc(StringInfo buf, XLogReaderState *record)
 {
 	char	   *rec = XLogRecGetData(record);
 	uint8		info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
+	char	   *dbpath1, *dbpath2;
 
 	if (info == XLOG_DBASE_CREATE)
 	{
 		xl_dbase_create_rec *xlrec = (xl_dbase_create_rec *) rec;
 
-		appendStringInfo(buf, "copy dir %u/%u to %u/%u",
-						 xlrec->src_tablespace_id, xlrec->src_db_id,
-						 xlrec->tablespace_id, xlrec->db_id);
+		dbpath1 = GetDatabasePath(xlrec->src_db_id,  xlrec->src_tablespace_id);
+		dbpath2 = GetDatabasePath(xlrec->db_id, xlrec->tablespace_id);
+		appendStringInfo(buf, "copy dir %s to %s", dbpath1, dbpath2);
+		pfree(dbpath2);
+		pfree(dbpath1);
 	}
 	else if (info == XLOG_DBASE_DROP)
 	{
 		xl_dbase_drop_rec *xlrec = (xl_dbase_drop_rec *) rec;
 
-		appendStringInfo(buf, "dir %u/%u",
-						 xlrec->tablespace_id, xlrec->db_id);
+		dbpath1 = GetDatabasePath(xlrec->db_id, xlrec->tablespace_id);
+		appendStringInfo(buf, "dir %s", dbpath1);
+		pfree(dbpath1);
 	}
 }
 
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e08320e829..5137a75efd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -40,6 +40,7 @@
 #include "catalog/pg_control.h"
 #include "catalog/pg_database.h"
 #include "commands/tablespace.h"
+#include "commands/dbcommands.h"
 #include "common/controldata_utils.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -7842,6 +7843,9 @@ CheckRecoveryConsistency(void)
 		 */
 		XLogCheckInvalidPages();
 
+		/* Check whether some missing directories are unexpected. */
+		CheckMissingDirs4DbaseRedo();
+
 		reachedConsistency = true;
 		ereport(LOG,
 				(errmsg("consistent recovery state reached at %X/%X",
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 15207bf75a..bc7ab3084e 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -45,6 +45,7 @@
 #include "commands/defrem.h"
 #include "commands/seclabel.h"
 #include "commands/tablespace.h"
+#include "common/file_perm.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -92,6 +93,68 @@ static void remove_dbtablespaces(Oid db_id);
 static bool check_db_file_conflict(Oid db_id);
 static int	errdetail_busy_db(int notherbackends, int npreparedxacts);
 
+static void log_missing_directory(char *dir);
+static void forget_missing_directory(char *dir);
+
+/*
+ * During XLOG replay, we may see either src directory or dst directory
+ * is missing during copying directory when creating database in dbase_redo()
+ * if the related tablespace was later dropped but we do re-redoing in
+ * recovery after abnormal shutdown. We do simply ignore copying in
+ * dbase_redo() but log those directories in memory and then sanity check
+ * the potential bug or bad user behaviors.
+ *
+ * We use List for simplicity since this should be ok for most cases - the
+ * list should be not long in usual case.
+ */
+static List *missing_dirs_dbase_redo = NIL;
+
+void
+CheckMissingDirs4DbaseRedo()
+{
+	ListCell *lc;
+
+	if (missing_dirs_dbase_redo == NIL)
+		return;
+
+	foreach(lc, missing_dirs_dbase_redo)
+	{
+		char *dir_entry = (char *) lfirst(lc);
+
+		elog(LOG, "Directory \"%s\" was missing during directory copying "
+			 "when replaying 'database create'", dir_entry);
+	}
+
+	elog(PANIC, "WAL replay was wrong due to previous missing directories");
+}
+
+static void
+log_missing_directory(char *dir)
+{
+	elog(DEBUG2, "Logging missing directory for dbase_redo(): \"%s\"", dir);
+	missing_dirs_dbase_redo = lappend(missing_dirs_dbase_redo, pstrdup(dir));
+}
+
+static void
+forget_missing_directory(char *dir)
+{
+	ListCell *prev, *lc;
+
+	prev = NULL;
+	foreach(lc, missing_dirs_dbase_redo)
+	{
+		char *dir_entry = (char *) lfirst(lc);
+
+		if (strcmp(dir_entry, dir) == 0)
+		{
+			missing_dirs_dbase_redo = list_delete_cell(missing_dirs_dbase_redo, lc, prev);
+			elog(DEBUG2, "forgetting missing directory for dbase_redo(): \"%s\"", dir);
+			return;
+		}
+
+		prev = lc;
+	}
+}
 
 /*
  * CREATE DATABASE
@@ -2089,7 +2152,9 @@ dbase_redo(XLogReaderState *record)
 		xl_dbase_create_rec *xlrec = (xl_dbase_create_rec *) XLogRecGetData(record);
 		char	   *src_path;
 		char	   *dst_path;
+		char	   *parent_path;
 		struct stat st;
+		bool	    do_copydir = true;
 
 		src_path = GetDatabasePath(xlrec->src_db_id, xlrec->src_tablespace_id);
 		dst_path = GetDatabasePath(xlrec->db_id, xlrec->tablespace_id);
@@ -2107,6 +2172,43 @@ dbase_redo(XLogReaderState *record)
 						(errmsg("some useless files may be left behind in old database directory \"%s\"",
 								dst_path)));
 		}
+		else
+		{
+			/*
+			 * It is possible that the tablespace was previously dropped, but
+			 * we are re-redoing database create with that tablespace after
+			 * an abnormal shutdown (e.g. immediate shutdown). In that case,
+			 * the directory are missing, we simply skip the copydir step.
+			 */
+			parent_path = pstrdup(dst_path);
+			get_parent_directory(parent_path);
+			if (!(stat(parent_path, &st) == 0 && S_ISDIR(st.st_mode)))
+			{
+				do_copydir = false;
+				log_missing_directory(dst_path);
+				ereport(WARNING,
+						(errmsg("Skip creating database directory \"%s\". "
+								"The dest tablespace may have been removed "
+								"before abnormal shutdown. If the removal "
+								"is illegal after later checking we will panic.",
+								parent_path)));
+			}
+			pfree(parent_path);
+		}
+
+		/* src directory is possibly missing during redo also. */
+		if (!(stat(src_path, &st) == 0 && S_ISDIR(st.st_mode)))
+		{
+			do_copydir = false;
+			log_missing_directory(src_path);
+			ereport(WARNING,
+					(errmsg("Skip creating database directory based on "
+							"\"%s\". The src tablespace may have been "
+							"removed before abnormal shutdown. If the removal "
+							"is illegal after later checking we will panic.",
+							src_path)));
+
+		}
 
 		/*
 		 * Force dirty buffers out to disk, to ensure source database is
@@ -2119,7 +2221,8 @@ dbase_redo(XLogReaderState *record)
 		 *
 		 * We don't need to copy subdirectories
 		 */
-		copydir(src_path, dst_path, false);
+		if (do_copydir)
+			copydir(src_path, dst_path, false);
 	}
 	else if (info == XLOG_DBASE_DROP)
 	{
@@ -2162,6 +2265,8 @@ dbase_redo(XLogReaderState *record)
 					(errmsg("some useless files may be left behind in old database directory \"%s\"",
 							dst_path)));
 
+		forget_missing_directory(dst_path);
+
 		if (InHotStandby)
 		{
 			/*
diff --git a/src/include/commands/dbcommands.h b/src/include/commands/dbcommands.h
index 28bf21153d..4893e0e289 100644
--- a/src/include/commands/dbcommands.h
+++ b/src/include/commands/dbcommands.h
@@ -19,6 +19,8 @@
 #include "lib/stringinfo.h"
 #include "nodes/parsenodes.h"
 
+extern void CheckMissingDirs4DbaseRedo(void);
+
 extern Oid	createdb(ParseState *pstate, const CreatedbStmt *stmt);
 extern void dropdb(const char *dbname, bool missing_ok);
 extern ObjectAddress RenameDatabase(const char *oldname, const char *newname);
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 8d5ad6bc16..e5b465dafd 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -551,13 +551,22 @@ target server since it isn't done by default.
 
 sub backup
 {
-	my ($self, $backup_name) = @_;
+	my ($self, $backup_name, %params) = @_;
 	my $backup_path = $self->backup_dir . '/' . $backup_name;
 	my $name        = $self->name;
+	my @rest = ();
+
+	if (defined $params{tablespace_mappings})
+	{
+		my @ts_mappings = split(/,/, $params{tablespace_mappings});
+		foreach my $elem (@ts_mappings) {
+			push(@rest, '--tablespace-mapping='.$elem);
+		}
+	}
 
 	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
 	TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-h',
-		$self->host, '-p', $self->port, '--no-sync');
+		$self->host, '-p', $self->port, '--no-sync', @rest);
 	print "# Backup finished\n";
 	return;
 }
diff --git a/src/test/perl/RecursiveCopy.pm b/src/test/perl/RecursiveCopy.pm
index baf5d0ac63..c912ce412d 100644
--- a/src/test/perl/RecursiveCopy.pm
+++ b/src/test/perl/RecursiveCopy.pm
@@ -22,6 +22,7 @@ use warnings;
 use Carp;
 use File::Basename;
 use File::Copy;
+use TestLib;
 
 =pod
 
@@ -97,14 +98,38 @@ sub _copypath_recurse
 	# invoke the filter and skip all further operation if it returns false
 	return 1 unless &$filterfn($curr_path);
 
-	# Check for symlink -- needed only on source dir
-	# (note: this will fall through quietly if file is already gone)
-	croak "Cannot operate on symlink \"$srcpath\"" if -l $srcpath;
-
 	# Abort if destination path already exists.  Should we allow directories
 	# to exist already?
 	croak "Destination path \"$destpath\" already exists" if -e $destpath;
 
+	# Check for symlink -- needed only on source dir
+	# (note: this will fall through quietly if file is already gone)
+	if (-l $srcpath)
+	{
+		croak "Cannot operate on symlink \"$srcpath\""
+		  if ($srcpath !~ /\/(pg_tblspc\/[0-9]+)$/);
+
+		# We have mapped tablespaces. Copy them individually
+		my $linkname = $1;
+		my $tmpdir = TestLib::tempdir;
+		my $dstrealdir = TestLib::real_dir($tmpdir);
+		my $srcrealdir = readlink($srcpath);
+
+		opendir(my $dh, $srcrealdir);
+		while (readdir $dh)
+		{
+			next if (/^\.\.?$/);
+			my $spath = "$srcrealdir/$_";
+			my $dpath = "$dstrealdir/$_";
+
+			copypath($spath, $dpath);
+		}
+		closedir $dh;
+
+		symlink $dstrealdir, $destpath;
+		return 1;
+	}
+
 	# If this source path is a file, simply copy it to destination with the
 	# same name and we're done.
 	if (-f $srcpath)
diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl
index 5dc52412ca..fad6a64d2a 100644
--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -15,7 +15,7 @@ if ($Config{osname} eq 'MSWin32')
 }
 else
 {
-	plan tests => 3;
+	plan tests => 5;
 }
 
 my $node = get_new_node('master');
@@ -66,3 +66,100 @@ is($node->safe_psql('postgres', qq[SELECT txid_status('$xid');]),
 	'aborted', 'xid is aborted after crash');
 
 $tx->kill_kill;
+
+# Ensure that tablespace removal doesn't cause error while recoverying
+# the preceding create datbase or objects.
+
+my $node_master = get_new_node('master2');
+$node_master->init(allows_streaming => 1);
+$node_master->start;
+
+# Create tablespace
+my $tspDir_master = TestLib::tempdir;
+my $realTSDir_master = TestLib::real_dir($tspDir_master);
+$node_master->safe_psql('postgres', "CREATE TABLESPACE ts1 LOCATION '$realTSDir_master'");
+
+my $tspDir_standby = TestLib::tempdir;
+my $realTSDir_standby = TestLib::real_dir($tspDir_standby);
+
+# Take backup
+my $backup_name = 'my_backup';
+$node_master->backup($backup_name,
+					 tablespace_mappings =>
+					   "$realTSDir_master=$realTSDir_standby");
+my $node_standby = get_new_node('standby2');
+$node_standby->init_from_backup($node_master, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# Make sure connection is made
+$node_master->poll_query_until(
+	'postgres', 'SELECT count(*) = 1 FROM pg_stat_replication');
+
+# Make sure to perform restartpoint after tablespace creation
+$node_master->wait_for_catchup($node_standby, 'replay',
+							   $node_master->lsn('replay'));
+$node_standby->safe_psql('postgres', 'CHECKPOINT');
+
+# Do immediate shutdown just after a sequence of CREAT DATABASE / DROP
+# DATABASE / DROP TABLESPACE. This leaves a CREATE DATBASE WAL record
+# that is to be applied to already-removed tablespace.
+$node_master->safe_psql('postgres',
+						q[CREATE DATABASE db1 WITH TABLESPACE ts1;
+						  DROP DATABASE db1;
+						  DROP TABLESPACE ts1;]);
+$node_master->wait_for_catchup($node_standby, 'replay',
+							   $node_master->lsn('replay'));
+$node_standby->stop('immediate');
+
+# Should restart ignoring directory creation error.
+is($node_standby->start(fail_ok => 1), 1);
+
+
+# Ensure that tablespace removal doesn't cause error while recoverying
+# the preceding create datbase or objects.
+
+$node_master = get_new_node('master3');
+$node_master->init(allows_streaming => 1);
+$node_master->start;
+
+# Create tablespace
+$tspDir_master = TestLib::tempdir;
+$realTSDir_master = TestLib::real_dir($tspDir_master);
+mkdir "$realTSDir_master/1";
+mkdir "$realTSDir_master/2";
+$node_master->safe_psql('postgres', "CREATE TABLESPACE ts1 LOCATION '$realTSDir_master/1'");
+$node_master->safe_psql('postgres', "CREATE TABLESPACE ts2 LOCATION '$realTSDir_master/2'");
+
+$tspDir_standby = TestLib::tempdir;
+$realTSDir_standby = TestLib::real_dir($tspDir_standby);
+
+# Take backup
+$backup_name = 'my_backup';
+$node_master->backup($backup_name,
+					 tablespace_mappings =>
+					   "$realTSDir_master/1=$realTSDir_standby/1,$realTSDir_master/2=$realTSDir_standby/2");
+$node_standby = get_new_node('standby3');
+$node_standby->init_from_backup($node_master, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# Make sure connection is made
+$node_master->poll_query_until(
+	'postgres', 'SELECT count(*) = 1 FROM pg_stat_replication');
+
+$node_master->safe_psql('postgres', "CREATE DATABASE db1 TABLESPACE ts1");
+
+# Make sure to perform restartpoint after tablespace creation
+$node_master->wait_for_catchup($node_standby, 'replay',
+							   $node_master->lsn('replay'));
+$node_standby->safe_psql('postgres', 'CHECKPOINT');
+
+# Do immediate shutdown ...
+$node_master->safe_psql('postgres',
+						q[ALTER DATABASE db1 SET TABLESPACE ts2;
+						  DROP TABLESPACE ts1;]);
+$node_master->wait_for_catchup($node_standby, 'replay',
+							   $node_master->lsn('replay'));
+$node_standby->stop('immediate');
+
+# Should restart ignoring directory creation error.
+is($node_standby->start(fail_ok => 1), 1);
-- 
2.17.2