Re: Regression test PANICs with master-standby setup on samemachine - Mailing list pgsql-hackers

At Tue, 23 Apr 2019 10:05:03 -0700, Andres Freund <andres@anarazel.de> wrote in
<20190423170503.uw5jxrujqlozg23l@alap3.anarazel.de>
> Hi,
> 
> On 2019-04-23 16:08:18 +0900, Michael Paquier wrote:
> > On Mon, Apr 22, 2019 at 11:00:03PM -0700, Andres Freund wrote:
> > > FWIW, I think the right fix for this is to simply drop the requirement
> > > that tablespace paths need to be absolute. It's not buying us anything,
> > > it's just making things more complicated. We should just do a simple
> > > check against the tablespace being inside PGDATA, and leave it at
> > > that. Yes, that can be tricked, but so can the current system.
> > 
> > convert_and_check_filename() checks after that already, mostly.  For
> > TAP tests I am not sure that this would help much though as all the
> > nodes of a given test use the same root path for their data folders,
> > so you cannot just use "../hoge/" as location.
> 
> I don't see the problem here.  Putting the primary and standby PGDATAs
> into a subdirectory that also can contain a relatively referenced
> tablespace seems trivial?
> 
> > I'm not We already generate a warning when a tablespace is in a data
> > folder, as this causes issues with recursion lookups of base backups.
> > What do you mean in this case?  Forbidding the behavior?  -- Michael
> 
> I mostly am talking about replacing
> 
> Oid
> CreateTableSpace(CreateTableSpaceStmt *stmt)
> {
> ...
>     /*
>      * Allowing relative paths seems risky
>      *
>      * this also helps us ensure that location is not empty or whitespace
>      */
>     if (!is_absolute_path(location))
>         ereport(ERROR,
>                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>                  errmsg("tablespace location must be an absolute path")));
> 
> with a check that forces relative paths to be outside of PGDATA (baring
> symlinks). As far as I can tell convert_and_check_filename() would check
> just about the opposite.

We need to adjust relative path between PGDATA-based and
pg_tblspc based. The attached first patch does that.

- I'm not sure it is OK to use getcwd this way.

The second attached is TAP change to support tablespaces using
relative tablespaces. One issue here is is_in_data_directory
canonicalizes DataDir on-the-fly. It is needed when DataDir
contains '/./' or such. I think the canonicalization should be
done far earlier.

- This is tentative or sample. I'll visit the current discussion thread.

The third is test for this issue.

- Tablespace handling gets easier.

The fourth is the fix for the issue here.

- Not all possible simliar issue is not checked.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 714702a6abfe1987eab98020c799b845917af156 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 24 Apr 2019 11:50:41 +0900
Subject: [PATCH 1/4] Allow relative tablespace location paths

Currently relative paths are not allowed as tablespace directory. That
makes tests about tablespaces bothersome but doens't prevent them
points into the data directory perfectly. This patch allows relative
direcotry based on the data directory as tablespace directory. Then
makes the check more strict.
---
 src/backend/access/transam/xlog.c         |  8 ++++
 src/backend/commands/tablespace.c         | 76 ++++++++++++++++++++++++-------
 src/backend/replication/basebackup.c      |  7 +++
 src/backend/utils/adt/misc.c              |  7 +++
 src/bin/pg_basebackup/pg_basebackup.c     | 56 +++++++++++++++++++++--
 src/test/regress/input/tablespace.source  |  3 ++
 src/test/regress/output/tablespace.source |  5 +-
 7 files changed, 142 insertions(+), 20 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c00b63c751..abc2fd951f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10419,6 +10419,14 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
             }
             linkpath[rllen] = '\0';
 
+            /*
+             * In the relative path cases, the link target is always prefixed
+             * by "../" to convert from data directory-based. So we just do
+             * the reverse here.
+             */
+            if (strncmp(s, "../", 3) == 0)
+                s += 3;
+
             /*
              * Add the escape character '\\' before newline in a string to
              * ensure that we can distinguish between the newline in the
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 3784ea4b4f..66a70871e6 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -94,6 +94,42 @@ static void create_tablespace_directories(const char *location,
                               const Oid tablespaceoid);
 static bool destroy_tablespace_directories(Oid tablespaceoid, bool redo);
 
+/*
+ * Check if the path is in the data directory strictly.
+ */
+static bool
+is_in_data_directory(const char *path)
+{
+    char cwd[MAXPGPATH];
+    char abspath[MAXPGPATH];
+    char absdatadir[MAXPGPATH];
+
+    getcwd(cwd, MAXPGPATH);
+    if (chdir(path) < 0)
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("invalid directory \"%s\": %m", path)));
+
+    /* getcwd is defined as returning absolute path */
+    getcwd(abspath, MAXPGPATH);
+
+    /* DataDir needs to be canonicalized */
+    if (chdir(DataDir))
+        ereport(FATAL,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("could not chdir to the data directory \"%s\": %m",
+                        DataDir)));
+    getcwd(absdatadir, MAXPGPATH);
+
+    /* this must succeed */
+    if (chdir(cwd))
+        ereport(FATAL,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("could not chdir to the current working directory \"%s\": %m",
+                     cwd)));
+
+    return path_is_prefix_of_path(absdatadir, abspath);
+}
 
 /*
  * Each database using a table space is isolated into its own name space
@@ -267,35 +303,26 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
                 (errcode(ERRCODE_INVALID_NAME),
                  errmsg("tablespace location cannot contain single quotes")));
 
-    /*
-     * Allowing relative paths seems risky
-     *
-     * this also helps us ensure that location is not empty or whitespace
-     */
-    if (!is_absolute_path(location))
+    /* Reject tablespaces in the data directory. */
+    if (is_in_data_directory(location))
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-                 errmsg("tablespace location must be an absolute path")));
+                 errmsg("tablespace location must not be inside the data directory")));
 
     /*
      * Check that location isn't too long. Remember that we're going to append
-     * 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'.  FYI, we never actually
+     * 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'.  In the relative path case, we
+     * attach "../" at the beginning of the path. FYI, we never actually
      * reference the whole path here, but MakePGDirectory() uses the first two
      * parts.
      */
-    if (strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 +
+    if (3 + strlen(location) + 1 + strlen(TABLESPACE_VERSION_DIRECTORY) + 1 +
         OIDCHARS + 1 + OIDCHARS + 1 + FORKNAMECHARS + 1 + OIDCHARS > MAXPGPATH)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                  errmsg("tablespace location \"%s\" is too long",
                         location)));
 
-    /* Warn if the tablespace is in the data directory. */
-    if (path_is_prefix_of_path(DataDir, location))
-        ereport(WARNING,
-                (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-                 errmsg("tablespace location should not be inside the data directory")));
-
     /*
      * Disallow creation of tablespaces named "pg_xxx"; we reserve this
      * namespace for system purposes.
@@ -570,7 +597,9 @@ static void
 create_tablespace_directories(const char *location, const Oid tablespaceoid)
 {
     char       *linkloc;
+    char       *linktarget;
     char       *location_with_version_dir;
+    bool        free_linktarget = false;
     struct stat st;
 
     linkloc = psprintf("pg_tblspc/%u", tablespaceoid);
@@ -639,13 +668,28 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid)
 
     /*
      * Create the symlink under PGDATA
+
+     * Relative tablespace location is based on PGDATA directory. The symlink
+     * is created in PGDATA/pg_tblspc. So adjust relative paths by attaching
+     * "../" at the head.
      */
-    if (symlink(location, linkloc) < 0)
+    if (is_absolute_path(location))
+        linktarget = unconstify(char *, location);
+    else
+    {
+        linktarget = psprintf("../%s", location);
+        free_linktarget = true;
+    }
+
+    if (symlink(linktarget, linkloc) < 0)
         ereport(ERROR,
                 (errcode_for_file_access(),
                  errmsg("could not create symbolic link \"%s\": %m",
                         linkloc)));
 
+    if (free_linktarget)
+        pfree(linktarget);
+
     pfree(linkloc);
     pfree(location_with_version_dir);
 }
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 36dcb28754..82ff4703d9 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1238,6 +1238,13 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
                                 pathbuf)));
             linkpath[rllen] = '\0';
 
+            /*
+             * Relative link target is always prefixed by "../". We need to
+             * remove it to obtain a relative tablespace directory.
+             */
+            if (strncmp(linkpath, "../", 3) == 0)
+                memmove(linkpath, linkpath + 3, strlen(linkpath) + 1 - 3);
+
             size += _tarWriteHeader(pathbuf + basepathlen + 1, linkpath,
                                     &statbuf, sizeonly);
 #else
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index d330a88e3c..59f987e5ae 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -330,6 +330,13 @@ pg_tablespace_location(PG_FUNCTION_ARGS)
                         sourcepath)));
     targetpath[rllen] = '\0';
 
+    /*
+     * Relative target paths are always prefixed by "../". We need to remove
+     * it to obtain tablespace directory.
+     */
+    if (strncmp(targetpath, "../", 3) == 0)
+        PG_RETURN_TEXT_P(cstring_to_text(targetpath + 3));
+
     PG_RETURN_TEXT_P(cstring_to_text(targetpath));
 #else
     ereport(ERROR,
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1a735b8046..9fb73bfd8a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1399,9 +1399,20 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
     if (basetablespace)
         strlcpy(current_path, basedir, sizeof(current_path));
     else
-        strlcpy(current_path,
-                get_tablespace_mapping(PQgetvalue(res, rownum, 1)),
-                sizeof(current_path));
+    {
+        const char *path = get_tablespace_mapping(PQgetvalue(res, rownum, 1));
+
+        if (is_absolute_path(path))
+            strlcpy(current_path, path, sizeof(current_path));
+        else
+        {
+            /* Relative tablespace locations need to be prefixed by basedir. */
+            strlcpy(current_path, basedir, sizeof(current_path));
+            if (current_path[strlen(current_path) - 1] != '/')
+                strlcat(current_path, "/", sizeof(current_path));
+            strlcat(current_path, path, sizeof(current_path));
+        }
+    }
 
     /*
      * Get the COPY data
@@ -1525,15 +1536,34 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
                      * are, you can call it an undocumented feature that you
                      * can map them too.)
                      */
+                    bool free_path = false;
+
                     filename[strlen(filename) - 1] = '\0';    /* Remove trailing slash */
 
                     mapped_tblspc_path = get_tablespace_mapping(©buf[157]);
+                    if (!is_absolute_path(mapped_tblspc_path))
+                    {
+                        /*
+                         * Convert relative tablespace location based on data
+                         * directory into path link target based on pg_tblspc
+                         * directory.
+                         */
+                        char *p = malloc(3 + strlen(mapped_tblspc_path) + 1);
+
+                        strcpy(p, "../");
+                        strcat(p, mapped_tblspc_path);
+                        mapped_tblspc_path = p;
+                        free_path = true;
+                    }
                     if (symlink(mapped_tblspc_path, filename) != 0)
                     {
                         pg_log_error("could not create symbolic link from \"%s\" to \"%s\": %m",
                                      filename, mapped_tblspc_path);
                         exit(1);
                     }
+
+                    if (free_path)
+                        free(unconstify(char *, mapped_tblspc_path));
                 }
                 else
                 {
@@ -1957,8 +1987,28 @@ BaseBackup(void)
         if (format == 'p' && !PQgetisnull(res, i, 1))
         {
             char       *path = unconstify(char *, get_tablespace_mapping(PQgetvalue(res, i, 1)));
+            bool        freeit = false;
+
+            if (!is_absolute_path(path))
+            {
+                /*
+                 * Relative tablespace locations need to be converted
+                 * attaching basedir.
+                 */
+                char *p = malloc(strlen(basedir) + 1 + strlen(path) + 1);
+
+                strcpy(p, basedir);
+                if (p[strlen(p) - 1] != '/')
+                    strcat(p, "/");
+                strcat(p, path);
+                path = p;
+                freeit = true;
+            }
 
             verify_dir_is_empty_or_create(path, &made_tablespace_dirs, &found_tablespace_dirs);
+
+            if (freeit)
+                free(path);
         }
     }
 
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 14ce0e7e04..cf596d607f 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -125,6 +125,9 @@ SELECT COUNT(*) FROM testschema.atable;        -- checks heap
 -- Will fail with bad path
 CREATE TABLESPACE regress_badspace LOCATION '/no/such/location';
 
+-- Will fail with data directory
+CREATE TABLESPACE regress_badspace LOCATION '.';
+
 -- No such tablespace
 CREATE TABLE bar (i int) TABLESPACE regress_nosuchspace;
 
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 8ebe08b9b2..c404d599f0 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -257,7 +257,10 @@ SELECT COUNT(*) FROM testschema.atable;        -- checks heap
 
 -- Will fail with bad path
 CREATE TABLESPACE regress_badspace LOCATION '/no/such/location';
-ERROR:  directory "/no/such/location" does not exist
+ERROR:  invalid directory "/no/such/location": No such file or directory
+-- Will fail with data directory
+CREATE TABLESPACE regress_badspace LOCATION '.';
+ERROR:  tablespace location must not be inside the data directory
 -- No such tablespace
 CREATE TABLE bar (i int) TABLESPACE regress_nosuchspace;
 ERROR:  tablespace "regress_nosuchspace" does not exist
-- 
2.16.3

From 4855a9c7b0b5f67b3c6fba667c5c1892eb58199c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Mon, 22 Apr 2019 20:58:53 +0900
Subject: [PATCH 2/4] Allow TAP test to excecise tablespace.

To perform tablespace related checks, this patch lets
PostgresNode::backup have a new parameter "tablespace_mapping", and
make init_from_backup handle capable to handle a backup created using
tablespace_mapping.
---
 src/test/perl/PostgresNode.pm  | 45 +++++++++++++++++++++++++++++++++++++-----
 src/test/perl/RecursiveCopy.pm | 38 +++++++++++++++++++++++++++++++----
 2 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 76874141c5..e951acc461 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -157,6 +157,7 @@ sub new
         _host    => $pghost,
         _basedir => "$TestLib::tmp_check/t_${testname}_${name}_data",
         _name    => $name,
+        _tablespaces => [],
         _logfile_generation => 0,
         _logfile_base       => "$TestLib::log_path/${testname}_${name}",
         _logfile            => "$TestLib::log_path/${testname}_${name}.log"
@@ -342,6 +343,26 @@ sub backup_dir
 
 =pod
 
+=item $node->make_tablespace_dir()
+
+make a tablespace directory 
+
+=cut
+
+sub make_tablespace_dir
+{
+    my ($self, $name) = @_;
+    my $basedir = $self->basedir;
+
+    die "tablespace name contains '/'" if ($name =~ m#/#);
+    my $reldir = "../$name";
+    mkdir $self->data_dir() . "/". $reldir;
+    push($self->{_tablespaces}, $reldir);
+    return $reldir;
+}
+
+=pod
+
 =item $node->info()
 
 Return a string containing human-readable diagnostic information (paths, etc)
@@ -540,13 +561,27 @@ target server since it isn't done by default.
 
 sub backup
 {
-    my ($self, $backup_name) = @_;
-    my $backup_path = $self->backup_dir . '/' . $backup_name;
+    my ($self, $backup_name, %params) = @_;
+    my $backup_path = $self->backup_dir . '/' . $backup_name . '/' . "pgdata";
     my $name        = $self->name;
+    my @rest = ();
+
+    if (defined $params{tablespace_mapping})
+    {
+        foreach my $p (split /,/, $params{tablespace_mapping})
+        {
+            push(@rest, "--tablespace-mapping=$p");
+        }
+    }
+
+    foreach my $p ($self->{_tablespaces})
+    {
+        mkdir "$backup_path/$p";
+    }
 
     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;
 }
@@ -592,7 +627,7 @@ sub backup_fs_cold
 sub _backup_fs
 {
     my ($self, $backup_name, $hot) = @_;
-    my $backup_path = $self->backup_dir . '/' . $backup_name;
+    my $backup_path = $self->backup_dir . '/' . $backup_name . '/' . "pgdata";
     my $port        = $self->port;
     my $name        = $self->name;
 
@@ -655,7 +690,7 @@ unconditionally set to enable replication connections.
 sub init_from_backup
 {
     my ($self, $root_node, $backup_name, %params) = @_;
-    my $backup_path = $root_node->backup_dir . '/' . $backup_name;
+    my $backup_path = $root_node->backup_dir . '/' . $backup_name . '/' . "pgdata";
     my $host        = $self->host;
     my $port        = $self->port;
     my $node_name   = $self->name;
diff --git a/src/test/perl/RecursiveCopy.pm b/src/test/perl/RecursiveCopy.pm
index baf5d0ac63..f165db7348 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,43 @@ 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 $rpath = my $target = readlink($srcpath);
+
+        #convert to pgdata-based if relative
+        $rpath =~ s#^\.\./##; 
+
+        my $srcrealdir = "$base_src_dir/$rpath";
+        my $dstrealdir = "$base_dest_dir/$rpath";
+
+        mkdir $dstrealdir;
+        opendir(my $dh, $srcrealdir);
+        while (readdir $dh)
+        {
+            next if (/^\.\.?$/);
+            my $spath = "$srcrealdir/$_";
+            my $dpath = "$dstrealdir/$_";
+
+            copypath($spath, $dpath);
+        }
+        closedir $dh;
+
+        symlink $target, $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)
-- 
2.16.3

From 51e5cb0996b71ee3cc32ea00e0abd3184df7e82d Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Mon, 22 Apr 2019 20:10:25 +0900
Subject: [PATCH 3/4] Add check for recovery failure caused by tablespace.

Removal of a tablespace on master can cause recovery failure on
standby. This patch adds the check for the case.
---
 src/test/recovery/t/011_crash_recovery.pl | 45 ++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl
index 5dc52412ca..40cf1d0cc3 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 => 4;
 }
 
 my $node = get_new_node('master');
@@ -66,3 +66,46 @@ 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 = $node_master->make_tablespace_dir('ts1');
+$node_master->safe_psql('postgres', "CREATE TABLESPACE ts1 LOCATION '$tspdir'");
+
+# Take backup
+my $backup_name = 'my_backup';
+$node_master->backup($backup_name);
+my $node_standby = get_new_node('standby');
+$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'));
+system("ls -l ". $node_standby->data_dir() . "/../ts1");
+$node_standby->stop('immediate');
+
+# Should restart ignoring directory creation error.
+is($node_standby->start(fail_ok => 1), 1);
-- 
2.16.3

From 51a80429c5b7e67779071400e43b0c7fc08c857c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Mon, 22 Apr 2019 20:59:15 +0900
Subject: [PATCH 4/4] Fix failure of standby startup caused by tablespace
 removal

When standby restarts after a crash after drop of a tablespace,
there's a possibility that recovery fails trying an object-creation in
already removed tablespace directory. Allow recovery to continue by
ignoring the error if not reaching consistency point.
---
 src/backend/access/transam/xlogutils.c | 34 ++++++++++++++++++++++++++++++++++
 src/backend/commands/tablespace.c      | 12 ++++++------
 src/backend/storage/file/copydir.c     | 12 +++++++-----
 src/include/access/xlogutils.h         |  1 +
 4 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 10a663bae6..75cdb882cd 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -522,6 +522,40 @@ XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
     return buffer;
 }
 
+/*
+ * XLogMakePGDirectory
+ *
+ * There is a possibility that WAL replay causes an error by creation of a
+ * directory under a directory removed before the previous crash. Issuing
+ * ERROR prevents the caller from continuing recovery.
+ *
+ * To prevent that case, this function issues WARNING instead of ERROR on
+ * error if consistency is not reached yet.
+ */
+int
+XLogMakePGDirectory(const char *directoryName)
+{
+    int ret;
+
+    ret = MakePGDirectory(directoryName);
+
+    if (ret != 0)
+    {
+        int elevel = ERROR;
+
+        /* Don't issue ERROR for this failure before reaching consistency. */
+        if (InRecovery && !reachedConsistency)
+            elevel = WARNING;
+
+        ereport(elevel,
+                (errcode_for_file_access(),
+                 errmsg("could not create directory \"%s\": %m", directoryName)));
+        return ret;
+    }
+
+    return 0;
+}
+
 /*
  * Struct actually returned by XLogFakeRelcacheEntry, though the declared
  * return type is Relation.
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 66a70871e6..c9fb2af015 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -303,12 +303,6 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
                 (errcode(ERRCODE_INVALID_NAME),
                  errmsg("tablespace location cannot contain single quotes")));
 
-    /* Reject tablespaces in the data directory. */
-    if (is_in_data_directory(location))
-        ereport(ERROR,
-                (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-                 errmsg("tablespace location must not be inside the data directory")));
-
     /*
      * Check that location isn't too long. Remember that we're going to append
      * 'PG_XXX/<dboid>/<relid>_<fork>.<nnn>'.  In the relative path case, we
@@ -323,6 +317,12 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
                  errmsg("tablespace location \"%s\" is too long",
                         location)));
 
+    /* Reject tablespaces in the data directory. */
+    if (is_in_data_directory(location))
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                 errmsg("tablespace location must not be inside the data directory")));
+
     /*
      * Disallow creation of tablespaces named "pg_xxx"; we reserve this
      * namespace for system purposes.
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 30f6200a86..0216270dd3 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -22,11 +22,11 @@
 #include <unistd.h>
 #include <sys/stat.h>
 
+#include "access/xlogutils.h"
 #include "storage/copydir.h"
 #include "storage/fd.h"
 #include "miscadmin.h"
 #include "pgstat.h"
-
 /*
  * copydir: copy a directory
  *
@@ -41,10 +41,12 @@ copydir(char *fromdir, char *todir, bool recurse)
     char        fromfile[MAXPGPATH * 2];
     char        tofile[MAXPGPATH * 2];
 
-    if (MakePGDirectory(todir) != 0)
-        ereport(ERROR,
-                (errcode_for_file_access(),
-                 errmsg("could not create directory \"%s\": %m", todir)));
+    /*
+     * We might have to skip copydir to continue recovery. See the function
+     * for details.
+     */
+    if (XLogMakePGDirectory(todir) != 0)
+        return;
 
     xldir = AllocateDir(fromdir);
 
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index 0ab5ba62f5..46a7596315 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -43,6 +43,7 @@ extern XLogRedoAction XLogReadBufferForRedoExtended(XLogReaderState *record,
 
 extern Buffer XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
                        BlockNumber blkno, ReadBufferMode mode);
+extern int XLogMakePGDirectory(const char *directoryName);
 
 extern Relation CreateFakeRelcacheEntry(RelFileNode rnode);
 extern void FreeFakeRelcacheEntry(Relation fakerel);
-- 
2.16.3