Re: Regression test PANICs with master-standby setup on samemachine

Hello.

At Wed, 24 Apr 2019 13:23:04 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20190424.132304.40676137.horiguchi.kyotaro@lab.ntt.co.jp>
> > > 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. Another 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.
> 
> The second attached is TAP change to support tablespaces using
> relative tablespaces.
> 
> - 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.

This is new version. Adjusted pg_basebackup's behavior to allow
relative mappings. But..

This is apparently out of a bug fix.  What should I do with it?

Should we applying only 0004 (after further checking) or
something as bug fix, then register the rest for v13?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 16b158756a8e51279604bf9f8995b9392052b274 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/5] 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        | 180 +++++++++++++++++++++++----
 src/bin/pg_basebackup/t/010_pg_basebackup.pl |  17 ++-
 src/test/regress/input/tablespace.source     |   3 +
 src/test/regress/output/tablespace.source    |   5 +-
 8 files changed, 258 insertions(+), 45 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..0cd7a89c56 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -269,26 +269,6 @@ tablespace_list_append(const char *arg)
         exit(1);
     }
 
-    /*
-     * This check isn't absolutely necessary.  But all tablespaces are created
-     * with absolute directories, so specifying a non-absolute path here would
-     * just never match, possibly confusing users.  It's also good to be
-     * consistent with the new_dir check.
-     */
-    if (!is_absolute_path(cell->old_dir))
-    {
-        pg_log_error("old directory is not an absolute path in tablespace mapping: %s",
-                     cell->old_dir);
-        exit(1);
-    }
-
-    if (!is_absolute_path(cell->new_dir))
-    {
-        pg_log_error("new directory is not an absolute path in tablespace mapping: %s",
-                     cell->new_dir);
-        exit(1);
-    }
-
     /*
      * Comparisons done with these values should involve similarly
      * canonicalized path values.  This is particularly sensitive on Windows
@@ -1399,9 +1379,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 +1516,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 +1967,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);
         }
     }
 
@@ -2163,6 +2193,104 @@ BaseBackup(void)
         pg_log_info("base backup completed");
 }
 
+/* functions to avoid duplicate error handling  */
+static void
+simple_chdir(const char *path)
+{
+    if (chdir(path) < 0)
+    {
+        pg_log_error("chdir failed to \"%s\": %m", path);
+        exit(1);
+    }
+}
+
+static void
+simple_getcwd(char *buf, int len)
+{
+    if (getcwd(buf, len) < 0)
+    {
+        pg_log_error("getcwd failed: %m");
+        exit(1);
+    }
+}
+
+/* Sanity check of tablespace mappings */
+static void
+check_tablespace_mappings(void)
+{
+    TablespaceListCell *cell;
+    char cwd[MAXPGPATH];
+    char canonbase[MAXPGPATH];
+
+    simple_getcwd(cwd, MAXPGPATH);
+
+    /* canonicalize basedir */
+    simple_chdir(basedir);
+    simple_getcwd(canonbase, MAXPGPATH);
+
+    for (cell = tablespace_dirs.head ; cell ; cell = cell->next)
+    {
+        char path[MAXPGPATH];
+
+        if (is_absolute_path(cell->new_dir))
+            strlcpy(path, cell->new_dir, MAXPGPATH);
+        else
+        {
+            /* construct absolute path of relative new_dir */
+            path[0] = 0;
+            if (!is_absolute_path(basedir))
+            {
+                strlcpy(path, cwd, MAXPGPATH);
+                if (path[strlen(path) - 1] != '/')
+                    strlcat(path, "/", MAXPGPATH);
+            }
+
+            strlcat(path, basedir, MAXPGPATH);
+            if (path[strlen(path) - 1] != '/')
+                strlcat(path, "/", MAXPGPATH);
+            strlcat(path, cell->new_dir, MAXPGPATH);
+        }
+
+        /* strip off trailing separators */
+        while (path[strlen(path) - 1] == '/')
+            path[strlen(path) - 1] = 0;
+
+        /*
+         * We allow new_dir is a nonexistent path. Even in that case, the
+         * parent directory must be exist. If chdir failed, go up one step
+         * then recheck then repeat.
+         */
+        while (chdir(path) < 0)
+        {
+            /* only one step up is needed */
+            if (errno == ENOENT)
+            {
+                /* Find the last separator */
+                char *p = strrchr(path, '/');
+
+                /* We cannot go up further */
+                if (p == NULL || p == path)
+                    break;
+
+                *p = 0;
+                continue;
+            }
+
+            pg_log_error("invalid new_dir \"%s\" of mapping for \"%s\": %m",
+                         cell->new_dir, cell->old_dir);
+            exit(1);
+        }
+
+        simple_getcwd(path, MAXPGPATH);
+        if (strncmp(canonbase, path, strlen(canonbase)) == 0)
+        {
+            pg_log_error("new_dir \"%s\" of mapping for \"%s\" is inside target data directory \"%s\"", cell->new_dir,
cell->old_dir,canonbase);
 
+            exit(1);
+        }
+    }
+
+    simple_chdir(cwd);
+}
 
 int
 main(int argc, char **argv)
@@ -2513,6 +2641,12 @@ main(int argc, char **argv)
     if (format == 'p' || strcmp(basedir, "-") != 0)
         verify_dir_is_empty_or_create(basedir, &made_new_pgdata, &found_existing_pgdata);
 
+    /*
+     * Sanity check of tablespace mapping. This can be perform after creating
+     * basedir.
+     */
+    check_tablespace_mappings();
+
     /* determine remote server's xlog segment size */
     if (!RetrieveWalSegSize(conn))
         exit(1);
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 33869fecc9..bd1a92df36 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -6,7 +6,7 @@ use File::Basename qw(basename dirname);
 use File::Path qw(rmtree);
 use PostgresNode;
 use TestLib;
-use Test::More tests => 106;
+use Test::More tests => 108;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -186,12 +186,19 @@ $node->command_fails(
         "-T/foo=/bar=/baz"
     ],
     '-T with multiple = fails');
+$node->command_ok(
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo=../bar/baz/foo" ],
+    '-T with old directory not absolute succeeds');
+rmtree("$tempdir/backup_foo");
 $node->command_fails(
-    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo=/bar" ],
-    '-T with old directory not absolute fails');
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=$tempdir/backup_foo/bar" ],
+    '-T with new absolute directory inside data dir fails');
 $node->command_fails(
-    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=bar" ],
-    '-T with new directory not absolute fails');
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=../backup_foo/bar/baz" ],
+    '-T with new relative directory inside data dir fails');
+$node->command_fails(
+    [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-T/foo=/foo/bar" ],
+    '-T with new directory under nonexistent directory fails');
 $node->command_fails(
     [ 'pg_basebackup', '-D', "$tempdir/backup_foo", '-Fp', "-Tfoo" ],
     '-T with invalid format fails');
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 007a366f3a51b02bdbce3f505b26c0340c47b67f 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/5] 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 743bd557db00be16e5d59324b82b62ccf49e87ab 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/5] 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 bc97e195f21af5d715d85424efc21fcbe8bb902c 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/5] 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

From d699571de1d0fe3665d28026bcbe720e8f6ba738 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 24 Apr 2019 16:02:47 +0900
Subject: [PATCH 5/5] Documentation update

---
 doc/src/sgml/ref/create_tablespace.sgml | 5 +++--
 doc/src/sgml/ref/pg_basebackup.sgml     | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/ref/create_tablespace.sgml b/doc/src/sgml/ref/create_tablespace.sgml
index c621ec2c6b..e2e9625483 100644
--- a/doc/src/sgml/ref/create_tablespace.sgml
+++ b/doc/src/sgml/ref/create_tablespace.sgml
@@ -94,8 +94,9 @@ CREATE TABLESPACE <replaceable class="parameter">tablespace_name</replaceable>
         The directory that will be used for the tablespace. The directory
         must exist (<command>CREATE TABLESPACE</command> will not create it),
         should be empty, and must be owned by the
-        <productname>PostgreSQL</productname> system user.  The directory must be
-        specified by an absolute path name.
+        <productname>PostgreSQL</productname> system user.  The directory may
+        be specified by a relative path name based on the data directory but
+        must not be inside the data directory.
        </para>
       </listitem>
      </varlistentry>
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index fc9e222f8d..ba9ba13b4c 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -238,8 +238,8 @@ PostgreSQL documentation
         path specification of the tablespace as it is currently defined.  (But
         it is not an error if there is no tablespace
         in <replaceable>olddir</replaceable> contained in the backup.)
-        Both <replaceable>olddir</replaceable>
-        and <replaceable>newdir</replaceable> must be absolute paths.  If a
+        If <replaceable>newdir</replaceable> is a relative path,
+        it is translated based on the output directory. If a
         path happens to contain a <literal>=</literal> sign, escape it with a
         backslash.  This option can be specified multiple times for multiple
         tablespaces.  See examples below.
-- 
2.16.3