From 968adb96f7440386e5ee70f66fbd7495068a09e6 Mon Sep 17 00:00:00 2001 From: Paul Guo 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