Subject: standby recovery fails (tablespace related) (tentative patch and discussion)

From: Paul Guo <[email-masked]>
Date: Wed, 17 Apr 2019 15:56:30 +0800

Hello postgres hackers,

Recently my colleagues and I encountered an issue: a standby can
not recover after an unclean shutdown and it's related to tablespace.
The issue is that the standby re-replay some xlog that needs tablespace
directories (e.g. create a database with tablespace),
but the tablespace directories has already been removed in the
previous replay.

In details, the standby normally finishes replaying for the below
operations, but due to unclean shutdown, the redo lsn
is not updated in pg_control and is still kept a value before the 'create
db with tabspace' xlog, however since the tablespace
directories were removed so it reports error when repay the database create
wal.

create db with tablespace
drop database
drop tablespace.

Here is the log on the standby.
2019-04-17 14:52:14.926 CST [23029] LOG:  starting PostgreSQL 12devel on
x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 (Red Hat
4.8.5-4), 64-bit
2019-04-17 14:52:14.927 CST [23029] LOG:  listening on IPv4 address
"192.168.35.130", port 5432
2019-04-17 14:52:14.929 CST [23029] LOG:  listening on Unix socket
"/tmp/.s.PGSQL.5432"
2019-04-17 14:52:14.943 CST [23030] LOG:  database system was interrupted
while in recovery at log time 2019-04-17 14:48:27 CST
2019-04-17 14:52:14.943 CST [23030] HINT:  If this has occurred more than
once some data might be corrupted and you might need to choose an earlier
recovery target.
2019-04-17 14:52:14.949 CST [23030] LOG:  entering standby mode

2019-04-17 14:52:14.950 CST [23030] LOG:  redo starts at 0/30105B8

2019-04-17 14:52:14.951 CST [23030] FATAL:  could not create directory
"pg_tblspc/65546/PG_12_201904072/65547": No such file or directory
2019-04-17 14:52:14.951 CST [23030] CONTEXT:  WAL redo at 0/3011650 for
Database/CREATE: copy dir 1663/1 to 65546/65547
2019-04-17 14:52:14.951 CST [23029] LOG:  startup process (PID 23030)
exited with exit code 1
2019-04-17 14:52:14.951 CST [23029] LOG:  terminating any other active
server processes
2019-04-17 14:52:14.953 CST [23029] LOG:  database system is shut down

Steps to reprodce:

1. setup a master and standby.
2. On both side, run: mkdir /tmp/some_isolation2_pg_basebackup_tablespace

3. Run SQLs:
drop tablespace if exists some_isolation2_pg_basebackup_tablespace;
create tablespace some_isolation2_pg_basebackup_tablespace location
'/tmp/some_isolation2_pg_basebackup_tablespace';

3. Clean shutdown and restart both postgres instances.

4. Run the following SQLs:

drop database if exists some_database_with_tablespace;
create database some_database_with_tablespace tablespace
some_isolation2_pg_basebackup_tablespace;
drop database some_database_with_tablespace;
drop tablespace some_isolation2_pg_basebackup_tablespace;
\! pkill -9 postgres; ssh host70 pkill -9 postgres

Note immediate shutdown via pg_ctl should also be able to reproduce and the
above steps probably does not 100% reproduce.

I created an initial patch for this issue (see the attachment). The idea is
re-creating those directories recursively. The above issue exists
in dbase_redo(),
but TablespaceCreateDbspace (for relation file create redo) is probably
buggy also so I modified that function also. Even there is no bug
in that function, it seems that using simple pg_mkdir_p() is cleaner. Note
reading TablespaceCreateDbspace(), I found it seems that this issue
has already be thought though insufficient but frankly this solution
(directory recreation) seems to be not perfect given actually this should
have been the responsibility of tablespace creation (also tablespace
creation does more like symlink creation, etc). Also, I'm not sure whether
we need to use invalid page mechanism (see xlogutils.c).

Another solution is that, actually, we create a checkpoint when
createdb/movedb/dropdb/droptablespace, maybe we should enforce to create
restartpoint on standby for such special kind of checkpoint wal - that
means we need to set a flag in checkpoing wal and let checkpoint redo
code to create restartpoint if that flag is set. This solution seems to be
safer.

Thanks,
Paul

---

From: Asim R P <[email-masked]>
Date: Fri, 19 Apr 2019 10:08:04 +0530

> create db with tablespace

Essentially, that sequence of operations causes crash recovery to fail
if the "drop tablespace" transaction was committed before crashing.
This is a bug in crash recovery in general and should be reproducible
without configuring a standby.  Is that right?

Your patch creates missing directories in the destination.  Don't we
need to create the tablespace symlink under pg_tblspc/?  I would
prefer extending the invalid page mechanism to deal with this, as
suggested by Ashwin off-list.  It will allow us to avoid creating
directories and files only to remove them shortly afterwards when the
drop database and drop tablespace records are replayed.

Asim

---

From: Paul Guo <[email-masked]>
Date: Mon, 22 Apr 2019 12:36:43 +0800

Please see my replies inline. Thanks.

> without configuring a standby.  Is that right?

No. In general, checkpoint is done for drop_db/create_db/drop_tablespace on
master.
That makes the file/directory update-to-date if I understand the related
code correctly.
For standby, checkpoint redo does not ensure that.

> need to create the tablespace symlink under pg_tblspc/?  I would

 'create db with tablespace' redo log does not include the tablespace real
directory information.
Yes, we could add in it into the xlog, but that seems to be an overdesign.

> drop database and drop tablespace records are replayed.
'invalid page' mechanism seems to be more proper for missing pages of a
file. For
missing directories, we could, of course, hack to use that (e.g. reading
any page of
a relfile in that database) to make sure the tablespace create code
(without symlink)
safer (It assumes those directories will be deleted soon).

More feedback about all of the previous discussed solutions is welcome.

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Mon, 22 Apr 2019 16:15:13 +0900 (Tokyo Standard Time)

Hello.

At Mon, 22 Apr 2019 12:36:43 +0800, Paul Guo <[email-masked]> wrote in <CAEET0ZGpUrMGUzfyzVF9FuSq+zb=[email-masked]>
> For standby, checkpoint redo does not ensure that.

That's right partly. As you must have seen, fast shutdown forces
restartpoint for the last checkpoint and it prevents the problem
from happening. Anyway it seems to be a problem.

> Yes, we could add in it into the xlog, but that seems to be an overdesign.

But I don't think creating directory that is to be removed just
after is a wanted solution. The directory most likely to be be
removed just after.

> More feedback about all of the previous discussed solutions is welcome.

It doesn't seem to me that the invalid page mechanism is
applicable in straightforward way, because it doesn't consider
simple file copy.

Drop failure is ignored any time. I suppose we can ignore the
error to continue recovering as far as recovery have not reached
consistency. The attached would work *at least* your case, but I
haven't checked this covers all places where need the same
treatment.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 10a663bae6..0bc63f48da 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -522,6 +522,44 @@ XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
 	return buffer;
 }
 
+/*
+ * XLogMakePGDirectory
+ *
+ * There is a possibility that WAL replay causes a creation of the same
+ * directory left by 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;
+
+		/*
+		 * We might get error trying to create existing directory that is to
+		 * be removed just after.  Don't issue ERROR in the case. Recovery
+		 * will stop if we again failed after 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/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);

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Mon, 22 Apr 2019 16:40:27 +0900 (Tokyo Standard Time)

Oops! The comment in the previous patch is wrong.

At Mon, 22 Apr 2019 16:15:13 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <[email-masked]> wrote in <[email-masked]>
> haven't checked this covers all places where need the same

The comment for the new function XLogMakePGDirectory is wrong:

The correct one is:

+ * 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.

It is fixed in the attached.

regards.

index 10a663bae6..01331f0da9 100644
 
+/*
+ * 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.
+ *

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Mon, 22 Apr 2019 21:19:33 +0900 (Tokyo Standard Time)

At Mon, 22 Apr 2019 16:40:27 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <[email-masked]> wrote in <[email-masked]>
> > > For standby, checkpoint redo does not ensure that.

The attached exercises this sequence, needing some changes in
PostgresNode.pm and RecursiveCopy.pm to allow tablespaces.

regards.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 23 Apr 2019 11:34:38 +0900

> PostgresNode.pm and RecursiveCopy.pm to allow tablespaces.

+    # 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;
+    }

The same stuff is proposed here:
https://www.postgresql.org/message-id/[email-masked]

So there is a lot of demand for making the recursive copy more skilled
at handling symlinks for tablespace tests, and I'd like to propose to
do something among those lines for the tests on HEAD, presumably for
v12 and not v13 as we are talking about a bug fix here?  I am not sure
yet which one of the proposals is better than the other though.
--
Michael

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Tue, 23 Apr 2019 14:02:34 +0900 (Tokyo Standard Time)

At Tue, 23 Apr 2019 11:34:38 +0900, Michael Paquier <[email-masked]> wrote in <[email-masked]>
> yet which one of the proposals is better than the other though.

TBH I like that (my one cieted above) not so much. However, I
prefer to have v12 if this is a bug and to be fixed in
v12. Otherwise we won't add a test for this later:p

Anyway I'll visit there. Thanks. 

regards.

---

From: Paul Guo <[email-masked]>
Date: Tue, 23 Apr 2019 13:31:58 +0800

Hi Kyotaro, ignoring the MakePGDirectory() failure will fix this database
create redo error, but I suspect some other kind of redo, which depends on
the files under the directory (they are not copied since the directory is
not created) and also cannot be covered by the invalid page mechanism,
could fail. Thanks.

On Mon, Apr 22, 2019 at 3:40 PM Kyotaro HORIGUCHI <
[email-masked]> wrote:

> At Mon, 22 Apr 2019 16:15:13 +0900 (Tokyo Standard Time), Kyotaro
> HORIGUCHI <[email-masked]> wrote in <
> [email-masked]>
> > At Mon, 22 Apr 2019 12:36:43 +0800, Paul Guo <[email-masked]> wrote in
> <CAEET0ZGpUrMGUzfyzVF9FuSq+zb=[email-masked]>

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Tue, 23 Apr 2019 16:39:49 +0900 (Tokyo Standard Time)

Hello.

At Tue, 23 Apr 2019 13:31:58 +0800, Paul Guo <[email-masked]> wrote in <[email-masked]>
> not created) and also cannot be covered by the invalid page mechanism,

If recovery starts from just after tablespace creation, that's
simple. The Symlink to the removed tablespace is already removed
in the case. Hence server innocently create files directly under
pg_tblspc, not in the tablespace. Finally all files that were
supposed to be created in the removed tablespace are removed
later in recovery.

If recovery starts from recalling page in a file that have been
in the tablespace, XLogReadBufferExtended creates one (perhaps
directly in pg_tblspc as described above) and the files are
removed later in recoery the same way to above. This case doen't
cause FATAL/PANIC during recovery even in master.

[email-masked]
| * Create the target file if it doesn't already exist.  This lets us cope
| * if the replay sequence contains writes to a relation that is later
| * deleted.  (The original coding of this routine would instead suppress
| * the writes, but that seems like it risks losing valuable data if the
| * filesystem loses an inode during a crash.  Better to write the data
| * until we are actually told to delete the file.)

So buffered access cannot be a problem for the reason above. The
remaining possible issue is non-buffered access to files in
removed tablespaces. This is what I mentioned upthread:

me> but I haven't checked this covers all places where need the same
me> treatment.

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Wed, 24 Apr 2019 17:13:54 +0900 (Tokyo Standard Time)

Mmm. I posted to wrong thread. Sorry.

At Tue, 23 Apr 2019 16:39:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <[email-masked]> wrote in <[email-masked]>
> me> but I haven't checked this covers all places where need the same

RM_DBASE_ID is fixed by the patch.

XLOG/XACT/CLOG/MULTIXACT/RELMAP/STANDBY/COMMIT_TS/REPLORIGIN/LOGICALMSG:
  - are not relevant.

HEAP/HEAP2/BTREE/HASH/GIN/GIST/SEQ/SPGIST/BRIN/GENERIC:
  - Resources works on buffer is not affected.

SMGR:
  - Both CREATE and TRUNCATE seems fine.

TBLSPC:
  - We don't nest tablespace directories. No Problem.

I don't find a similar case.

regards.

---

From: Paul Guo <[email-masked]>
Date: Sun, 28 Apr 2019 15:33:13 +0800

On Wed, Apr 24, 2019 at 4:14 PM Kyotaro HORIGUCHI <
[email-masked]> wrote:

> At Tue, 23 Apr 2019 16:39:49 +0900 (Tokyo Standard Time), Kyotaro
> [email-masked]>
> > At Tue, 23 Apr 2019 13:31:58 +0800, Paul Guo <[email-masked]> wrote in
> <[email-masked]>

I took some time in digging into the related code. It seems that ignoring
if the dst directory cannot be created directly
should be fine since smgr redo code creates tablespace path finally by
calling TablespaceCreateDbspace().
What's more, I found some more issues.

1) The below error message is actually misleading.

That should be due to dbase_desc(). It could be simply fixed following the
code logic in GetDatabasePath().

2) It seems that src directory could be missing then
dbase_redo()->copydir() could error out. For example,

\!rm -rf /tmp/tbspace1
\!mkdir /tmp/tbspace1
\!rm -rf /tmp/tbspace2
\!mkdir /tmp/tbspace2
create tablespace tbs1 location '/tmp/tbspace1';
create tablespace tbs2 location '/tmp/tbspace2';
create database db1 tablespace tbs1;
alter database db1 set tablespace tbs2;
drop tablespace tbs1;

Let's say, the standby finishes all replay but redo lsn on pg_control is
still the point at 'alter database', and then
kill postgres, then in theory when startup, dbase_redo()->copydir() will
ERROR since 'drop tablespace tbs1'
has removed the directories (and symlink) of tbs1. Below simple code change
could fix that.

diff --git a/src/backend/commands/dbcommands.c
b/src/backend/commands/dbcommands.c
index 9707afabd9..7d755c759e 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -2114,6 +2114,15 @@ dbase_redo(XLogReaderState *record)
         */
        FlushDatabaseBuffers(xlrec->src_db_id);

+       /*
+        * It is possible that the source directory is missing if
+        * we are re-replaying the xlog while subsequent xlogs
+        * drop the tablespace in previous replaying. For this
+        * we just skip.
+        */
+       if (!(stat(src_path, &st) == 0 && S_ISDIR(st.st_mode)))
+           return;
+
        /*
         * Copy this subdirectory to the new location
         *

If we want to fix the issue by ignoring the dst path create failure, I do
not think we should do
that in copydir() since copydir() seems to be a common function. I'm not
sure whether it is
used by some extensions or not. If no maybe we should move the dst patch
create logic
out of copydir().

Also I'd suggest we should use pg_mkdir_p() in TablespaceCreateDbspace() to
replace
the code block includes a lot of get_parent_directory(), MakePGDirectory(),
etc even it
is not fixing a bug since pg_mkdir_p() code change seems to be more
graceful and simpler.

Whatever ignore mkdir failure or mkdir_p, I found that these steps seem to
be error-prone
along with postgre evolving since they are hard to test and also we are not
easy to think out
various potential bad cases. Is it possible that we should do real
checkpoint (flush & update
redo lsn) when seeing checkpoint xlogs for these operations? This will slow
down standby
but master also does this and also these operations are not usual,
espeically it seems that it
does not slow down wal receiving usually?

---

From: Paul Guo <[email-masked]>
Date: Tue, 30 Apr 2019 14:33:47 +0800

I updated the original patch to

1) skip copydir() if either src path or dst parent path is missing in
dbase_redo(). Both missing cases seem to be possible. For the src path
missing case, mkdir_p() is meaningless. It seems that moving the directory
existence check step to dbase_redo() has less impact on other code.

2) Fixed dbase_desc(). Now the xlog output looks correct.

rmgr: Database    len (rec/tot):     42/    42, tx:        486, lsn:
0/016386A8, prev 0/01638630, desc: CREATE copy dir base/1 to
pg_tblspc/16384/PG_12_201904281/16386

rmgr: Database    len (rec/tot):     34/    34, tx:        487, lsn:
0/01638EB8, prev 0/01638E40, desc: DROP dir
pg_tblspc/16384/PG_12_201904281/16386

I'm not familiar with the TAP test details previously. I learned a lot
about how to test such case from Kyotaro's patch series.👍

On Sun, Apr 28, 2019 at 3:33 PM Paul Guo <[email-masked]> wrote:

>> > At Tue, 23 Apr 2019 13:31:58 +0800, Paul Guo <[email-masked]> wrote
>> in <[email-masked]>
> has removed the directories (and symlink) of tbs1. Below simple code
> change could fix that.
>
> Also I'd suggest we should use pg_mkdir_p() in TablespaceCreateDbspace()
> to replace
> the code block includes a lot of
> get_parent_directory(), MakePGDirectory(), etc even it
> along with postgre evolving since they are hard to test and also we are
> not easy to think out
> redo lsn) when seeing checkpoint xlogs for these operations? This will
> slow down standby

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Tue, 07 May 2019 15:47:11 +0900 (Tokyo Standard Time)

Hi.

At Tue, 30 Apr 2019 14:33:47 +0800, Paul Guo <[email-masked]> wrote in <[email-masked]>
> I updated the original patch to

It's reasonable not to touch copydir.

> existence check step to dbase_redo() has less impact on other code.

Nice catch.

+      if (!(stat(parent_path, &st) == 0 && S_ISDIR(st.st_mode)))
+      {

This patch is allowing missing source and destination directory
even in consistent state. I don't think it is safe.

+        ereport(WARNING,
+            (errmsg("directory \"%s\" for copydir() does not exists."
+                "It is possibly expected. Skip copydir().",
+                parent_path)));

This message seems unfriendly to users, or it seems like an elog
message. How about something like this. The same can be said for
the source directory.

| WARNING:  skipped creating database directory: "%s"
| DETAIL:  The tabelspace %u may have been removed just before crash.

# I'm not confident in this at all:(

> pg_tblspc/16384/PG_12_201904281/16386

WAL records don't convey such information. The previous
description seems right to me.

> about how to test such case from Kyotaro's patch series.👍

Yeah, good to hear.

> > used by some extensions or not. If no maybe we should move the dst patch

Agreed to this.

> > is not fixing a bug since pg_mkdir_p() code change seems to be more

But I don't agree to this. pg_mkdir_p goes above two-parents up,
which would be unwanted here.

> > does not slow down wal receiving usually?

That dramatically slows recovery (not replication) if databases
are created and deleted frequently. That wouldn't be acceptable.

regards.

---

From: Paul Guo <[email-masked]>
Date: Mon, 13 May 2019 17:37:50 +0800

Thanks for the reply.

> even in consistent state. I don't think it is safe.

I do not understand this. Can you elaborate?

> | DETAIL:  The tabelspace %u may have been removed just before crash.

Yeah. Looks better.

2019-04-17 14:52:14.951 CST [23030] CONTEXT:  WAL redo at 0/3011650 for
Database/CREATE: copy dir 1663/1 to 65546/65547
The directories are definitely wrong and misleading.

> I do not understand this also. pg_mkdir_p() is similar to 'mkdir -p'.
This change just makes the code concise. Though in theory the change is not
needed.

> are created and deleted frequently. That wouldn't be acceptable.

This behavior is rare and seems to have the same impact on master & standby
from checkpoint/restartpoint.
We do not worry about master so we should not worry about standby also.

---

From: Kyotaro HORIGUCHI <[email-masked]>
Date: Tue, 14 May 2019 12:06:13 +0900 (Tokyo Standard Time)

Hello.

At Mon, 13 May 2019 17:37:50 +0800, Paul Guo <[email-masked]> wrote in <[email-masked]>
> I do not understand this. Can you elaborate?

Suppose we were recoverying based on a backup at LSN1 targeting
to LSN3 then it crashed at LSN2, where LSN1 < LSN2 <= LSN3. LSN2
is called as "consistency point", before where the database is
not consistent. It's because we are applying WAL recored older
than those that were already applied in the second trial. The
same can be said for crash recovery, where LSN1 is the latest
checkpoint ('s redo LSN) and LSN2=LSN3 is the crashed LSN.

Creation of an existing directory or dropping of a non-existent
directory are apparently inconsistent or "broken" so we should
stop recovery when seeing such WAL records while database is in
consistent state.

> The directories are definitely wrong and misleading.

The original description is right in the light of how the server
recognizes. The record exactly says that "copy dir 1663/1 to
65546/65547" and the latter path is converted in filesystem layer
via a symlink.

> This change just makes the code concise. Though in theory the change is not

We don't want to create tablespace direcotory after concurrent
DROPing, as the comment just above is saying:

|  * Acquire TablespaceCreateLock to ensure that no DROP TABLESPACE
|  * or TablespaceCreateDbspace is running concurrently.

If the concurrent DROP TABLESPACE destroyed the grand parent
directory, we mustn't create it again.

> We do not worry about master so we should not worry about standby also.

I didn't mention replication. I said that that slows recovery,
which is not governed by master's speed.

regards.

---

From: Paul Guo <[email-masked]>
Date: Mon, 27 May 2019 21:39:03 +0800

On Tue, May 14, 2019 at 11:06 AM Kyotaro HORIGUCHI <
[email-masked]> wrote:

> Hello.
>
> At Mon, 13 May 2019 17:37:50 +0800, Paul Guo <[email-masked]> wrote in <
> [email-masked]>
> > Thanks for the reply.
> >

This seems to be hard to detect. I thought using invalid_page mechanism
long ago,
but it seems to be hard to fully detect a dropped tablespace.

> 65546/65547" and the latter path is converted in filesystem layer

In either $PG_DATA/pg_tblspc or symlinked real tablespace directory,
there is an additional directory like PG_12_201905221 between
tablespace oid and database oid. See the directory layout as below,
so the directory info in xlog dump output was not correct.

$ ls -lh data/pg_tblspc/

total 0

lrwxrwxrwx. 1 gpadmin gpadmin 6 May 27 17:23 16384 -> /tmp/2

$ ls -lh /tmp/2

total 0

drwx------. 3 gpadmin gpadmin 18 May 27 17:24 PG_12_201905221

> directory, we mustn't create it again.

Yes, this is a good reason to keep the original code. Thanks.

By the way, based on your previous test patch I added another test which
could easily detect
the missing src directory case.

---

From: Paul Guo <[email-masked]>
Date: Wed, 19 Jun 2019 15:21:54 +0800

> the missing src directory case.

I updated the patch to v3. In this version, we skip the error if copydir
fails due to missing src/dst directory,
but to make sure the ignoring is legal, I add a simple log/forget mechanism
(Using List) similar to the xlog invalid page
checking mechanism. Two tap tests are included. One is actually from a
previous patch by Kyotaro in this
email thread and another is added by me. In addition, dbase_desc() is fixed
to make the message accurate.

Thanks.

---

From: Thomas Munro <[email-masked]>
Date: Mon, 8 Jul 2019 15:15:35 +1200

On Wed, Jun 19, 2019 at 7:22 PM Paul Guo <[email-masked]> wrote:
> I updated the patch to v3. In this version, we skip the error if copydir fails due to missing src/dst directory,
> but to make sure the ignoring is legal, I add a simple log/forget mechanism (Using List) similar to the xlog invalid page
> checking mechanism. Two tap tests are included. One is actually from a previous patch by Kyotaro in this
> email thread and another is added by me. In addition, dbase_desc() is fixed to make the message accurate.

Hello Paul,

FYI t/011_crash_recovery.pl is failing consistently on Travis CI with
this patch applied:

https://travis-ci.org/postgresql-cfbot/postgresql/builds/555368907

-- 
Thomas Munro
https://enterprisedb.com

---

From: Paul Guo <[email-masked]>
Date: Mon, 15 Jul 2019 18:52:01 +0800

On Mon, Jul 8, 2019 at 11:16 AM Thomas Munro <[email-masked]> wrote:

> > but to make sure the ignoring is legal, I add a simple log/forget
> mechanism (Using List) similar to the xlog invalid page
> https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.org_postgresql-2Dcfbot_postgresql_builds_555368907&d=DwIBaQ&c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&r=Usi0ex6Ch92MsB5QQDgYFw&m=ABylo8AVfubiiYVbCBSgmNnHEMJhMqGXx5c0hkug7Vw&s=5h4m_JhrZwZqsRsu1CHCD3W2eBl14mT8jWLFsj2-bJ4&e=
>
>
>
This failure is because the previous v3 patch does not align with a recent
patch

commit 660a2b19038b2f6b9f6bcb2c3297a47d5e3557a8

Author: Noah Misch <[email-masked]>

Date:   Fri Jun 21 20:34:23 2019 -0700

    Consolidate methods for translating a Perl path to a Windows path.

My patch uses TestLib::real_dir which is now replaced
with TestLib::perl2host in the above commit.

I've updated the patch to v4 to make my code align. Now the test passes in
my local environment.

Please see the attached v4 patch.

Thanks.

---

From: Thomas Munro <[email-masked]>
Date: Fri, 2 Aug 2019 10:37:46 +1200

> Please see the attached v4 patch.

While moving this to the next CF, I noticed that this needs updating
for the new pg_list.h API.

-- 
Thomas Munro
https://enterprisedb.com

---

From: Paul Guo <[email-masked]>
Date: Thu, 22 Aug 2019 21:13:20 +0800

Thanks. I updated the patch to v5. It passes install-check testing and
recovery testing.

On Fri, Aug 2, 2019 at 6:38 AM Thomas Munro <[email-masked]> wrote:

> https://urldefense.proofpoint.com/v2/url?u=https-3A__enterprisedb.com&d=DwIBaQ&c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&r=Usi0ex6Ch92MsB5QQDgYFw&m=1zhC6VaaS7Ximav7vaUXMUt6EGjrVZpNZut32ug7LDI&s=jSDXnTPIW4WNZCCZ_HIbu7gZ3apEBx36DCeNeNuhLpY&e=
>

---

From: Anastasia Lubennikova <[email-masked]>
Date: Thu, 22 Aug 2019 19:13:05 +0300

> Thanks. I updated the patch to v5. It passes install-check testing and 
Hi,
Thank you for working on this fix.
The overall design of the latest version looks good to me.
But during the review, I found a bug in the current implementation.
New behavior must apply to crash-recovery only, now it applies to 
archiveRecovery too.
That can cause a silent loss of a tablespace during regular standby 
operation
since it never calls CheckRecoveryConsistency().

Steps to reproduce:
1) run master and replica
2) create dir for tablespace:
mkdir  /tmp/tblspc1

3) create tablespace and database on the master:
create tablespace tblspc1 location '/tmp/tblspc1';
create database db1 tablespace tblspc1 ;

4) wait for replica to receive this changes and pause replication:
select pg_wal_replay_pause();

5) move replica's tablespace symlink to some empty directory, i.e. 
/tmp/tblspc2
mkdir  /tmp/tblspc2
ln -sfn /tmp/tblspc2 postgresql_data_replica/pg_tblspc/16384

6) create another database in tblspc1 on master:
create database db2 tablespace tblspc1 ;

7) resume replication on standby:
select pg_wal_replay_resume();

8) try to connect to db2 on standby

It's expected that dbase_redo() will fail because the directory on 
standby is not found.
While with the patch it suppresses the error until we attempt to connect 
db2 on the standby:

2019-08-22 18:34:39.178 MSK [21066] HINT:  Execute 
pg_wal_replay_resume() to continue.
2019-08-22 18:42:41.656 MSK [21066] WARNING:  Skip creating database 
directory "pg_tblspc/16384/PG_13_201908012". The dest tablespace may 
have been removed before abnormal shutdown. If the removal is illegal 
after later checking we will panic.
2019-08-22 18:42:41.656 MSK [21066] CONTEXT:  WAL redo at 0/3027738 for 
Database/CREATE: copy dir base/1 to pg_tblspc/16384/PG_13_201908012/16390
2019-08-22 18:42:46.096 MSK [21688] FATAL: 
"pg_tblspc/16384/PG_13_201908012/16390" is not a valid data directory
2019-08-22 18:42:46.096 MSK [21688] DETAIL:  File 
"pg_tblspc/16384/PG_13_201908012/16390/PG_VERSION" is missing.

Also some nitpicking about code style:
1) Please, add comment to forget_missing_directory().

2) +               elog(LOG, "Directory \"%s\" was missing during 
directory copying "
I think we'd better update this message elevel to WARNING.

3) Shouldn't we also move FlushDatabaseBuffers(xlrec->src_db_id); call under
     if (do_copydir) clause?
I don't see a reason to flush pages that we won't use later.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 3 Sep 2019 11:58:55 -0400

> New behavior must apply to crash-recovery only, now it applies to

Hello

Paul, Kyotaro, are you working on updating this bugfix?  FWIW the latest
patch submitted by Paul is still current and CFbot says it passes its
own test, but from Anastasia's email it still needs a bit of work.

Also: it would be good to have this new bogus scenario described by
Anastasia covered by a new TAP test.  Anastasia, can we enlist you to
write that?  Maybe Kyotaro?

Thanks

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Paul Guo <[email-masked]>
Date: Thu, 5 Sep 2019 15:12:23 +0800

> write that?  Maybe Kyotaro?
Thanks Anastasia and Alvaro for comment and suggestion. Sorry I've been busy
working on some non-PG stuffs recently. I've never worked on archive
recovery,
so I expect a bit more time after I'm free (hopefully several days later)
to take a look.
Of course Kyotaro, Anastasia or anyone feel free to address the concern
before that.

---

From: Asim R P <[email-masked]>
Date: Tue, 10 Sep 2019 17:12:55 +0530

Hi Anastasia

archiveRecovery too.
operation
/tmp/tblspc2
> ln -sfn /tmp/tblspc2 postgresql_data_replica/pg_tblspc/16384

By changing the tablespace symlink target, we are silently nullifying
effects of a committed transaction from the standby data directory - the
directory structure created by the standby for create tablespace
transaction.  This step, therefore, does not look like a valid test case to
me.  Can you share a sequence of steps that does not involve changing data
directory manually?

directory copying "
> I don't see a reason to flush pages that we won't use later.

Thank you for the review feedback.  I agree with all the points.  Let me
incorporate them (I plan to pick this work up and drive it to completion as
Paul got busy with other things).

But before that I'm revisiting another solution upthread, that of creating
restart points when replaying create/drop database commands before making
filesystem changes such as removing a directory.  The restart points should
align with checkpoints on master.  The concern against this solution was
creation of restart points will slow down recovery.  I don't think crash
recovery is affected by this solution because of the already existing
enforcement of checkpoints.  WAL records prior to a create/drop database
will not be seen by crash recovery due to the checkpoint enforced during
the command's normal execution.

Asim

---

From: Anastasia Lubennikova <[email-masked]>
Date: Wed, 11 Sep 2019 17:26:44 +0300

10.09.2019 14:42, Asim R P wrote:
> Hi Anastasia
>
> On Thu, Aug 22, 2019 at 9:43 PM Anastasia Lubennikova 
> <[email-masked] <mailto:[email-masked]>> 
> wrote:
> >
> effects of a committed transaction from the standby data directory - 
> the directory structure created by the standby for create tablespace 
> transaction.  This step, therefore, does not look like a valid test 
> case to me.  Can you share a sequence of steps that does not involve 
> changing data directory manually?
>
Hi, the whole idea of the test is to reproduce a data loss. For example, 
if the disk containing this tablespace failed.
Probably, simply deleting the directory 
'postgresql_data_replica/pg_tblspc/16384'
would work as well, though I was afraid that it can be caught by some 
earlier checks and my example won't be so illustrative.
>
> Thank you for the review feedback.  I agree with all the points.  Let 
> me incorporate them (I plan to pick this work up and drive it to 
> completion as Paul got busy with other things).
>
> But before that I'm revisiting another solution upthread, that of 
> creating restart points when replaying create/drop database commands 
> before making filesystem changes such as removing a directory.  The 
> restart points should align with checkpoints on master.  The concern 
> against this solution was creation of restart points will slow down 
> recovery.  I don't think crash recovery is affected by this solution 
> because of the already existing enforcement of checkpoints.  WAL 
> records prior to a create/drop database will not be seen by crash 
> recovery due to the checkpoint enforced during the command's normal 
> execution.
>

I haven't measured the impact of generating extra restart points in 
previous solution, so I cannot tell whether concerns upthread are 
justified.  Still, I enjoy latest design more, since it is clear and 
similar with the code of checking unexpected uninitialized pages. In 
principle it works. And the issue, I described in previous review can be 
easily fixed by several additional checks of InHotStandby macro.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 12 Sep 2019 17:35:36 +0900 (Tokyo Standard Time)

Hello.

At Wed, 11 Sep 2019 17:26:44 +0300, Anastasia Lubennikova <[email-masked]> wrote in <[email-masked]>
> > > since it never calls CheckRecoveryConsistency().

Yeah. We should take the same steps with redo operations on
missing pages. Just record failure during inconsistent state then
forget it if underlying tablespace is gone. If we had a record
when we reached concsistency, we're in a serious situation and
should stop recovery.  log_invalid_page forget_invalid_pages and
CheckRecoveryConsistency are the entry points of the feature to
understand.

> > > 5) move replica's tablespace symlink to some empty directory,
> > > i.e. /tmp/tblspc2
> > > mkdir  /tmp/tblspc2

I see it as the same. WAL is inconsistent with what happend on
storage with the steps. Database is just broken.

> Hi, the whole idea of the test is to reproduce a data loss. For
> example, if the disk containing this tablespace failed.

So, apparently we must start recovery from a backup before that
failure happened in that case, and that should ends in success.

# I remember that the start point of this patch is a crash after
# table space drop subsequent to several operations within the
# table space. Then, crash recovery fails at an operation in the
# finally-removed tablespace. Is it right?

> principle it works. And the issue, I described in previous review can
> be easily fixed by several additional checks of InHotStandby macro.

Generally we shouldn't trigger useless restart point for
uncertain reasons. If standby crashes, it starts the next
recovery from the latest *restart point*.  Even in that case what
we should do is the same.

Of course, for testing, we *should* establish a restartpoint
manually in order to establish the prerequisite state.

regards.

---

From: Asim R P <[email-masked]>
Date: Thu, 12 Sep 2019 17:32:41 +0530

On Thu, Sep 12, 2019 at 2:05 PM Kyotaro Horiguchi <[email-masked]>
wrote:
>
> Hello.
>
> At Wed, 11 Sep 2019 17:26:44 +0300, Anastasia Lubennikova <
[email-masked]> wrote in <
[email-masked]>
> CheckRecoveryConsistency are the entry points of the feature to

Yes, I get it now.  I will adjust the patch written by Paul accordingly.

> # finally-removed tablespace. Is it right?

That's correct.  Once the directories are removed from filesystem, any
attempt to replay WAL records that depend on their existence fails.

> we should do is the same.

The reason is quite clear to me - removing directories from filesystem
break the ability to replay WAL records second time.  And we already create
checkpoints during normal operation in such a case, so crash recovery on a
master node does not suffer from this bug.  I've attached a patch that
performs restart points during drop database replay, just for reference.
It passes both the TAP tests written by Kyotaro and Paul.  I had to modify
drop database WAL record a bit.

Asim

---

From: Asim R P <[email-masked]>
Date: Thu, 19 Sep 2019 17:29:59 +0530

recovery testing.

This patch contains one more bug, in addition to what Anastasia has found.
If the test case in the patch is tweaked slightly, as follows, the standby
crashes due to PANIC.

--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -147,8 +147,6 @@ $node_standby->start;
 $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'));
@@ -156,7 +154,8 @@ $node_standby->safe_psql('postgres', 'CHECKPOINT');

 # Do immediate shutdown ...
 $node_master->safe_psql('postgres',
-                                               q[ALTER DATABASE db1 SET
TABLESPACE ts2;
+                                               q[CREATE DATABASE db1
TABLESPACE ts1;
+                                                 ALTER DATABASE db1 SET
TABLESPACE ts2;
                                                  DROP TABLESPACE ts1;]);
 $node_master->wait_for_catchup($node_standby, 'replay',

 $node_master->lsn('replay'));

Notice the create additional create database in the above change.  That
causes the same tablespace directory (ts1) logged twice in the list of
missing directories.  At the end of crash recovery, there is one unmatched
entry in the missing dirs list and the standby PANICs.

Please find attached a couple of tests that are built on top of what was
already written by Paul, Kyotaro.  The patch includes a test to demonstrate
the above mentioned failure and a test case that my friend Alexandra wrote
to implement the archive recovery scenario noted by Anastasia.

In order to fix the test failures, we need to distinguish between a missing
database directory and a missing tablespace directory.  And also add logic
to forget missing directories during tablespace drop.  I am working on it.

Asim

---

From: Asim R P <[email-masked]>
Date: Fri, 20 Sep 2019 17:53:56 +0530

On Thu, Sep 19, 2019 at 5:29 PM Asim R P <[email-masked]> wrote:
>
> In order to fix the test failures, we need to distinguish between a
missing database directory and a missing tablespace directory.  And also
add logic to forget missing directories during tablespace drop.  I am
working on it.

Please find attached a solution that builds on what Paul has propose.  A
hash table, similar to the invalid page hash table is used to track missing
directory references.  A missing directory may be a tablespace or a
database, based on whether the tablespace is found missing or the source
database is found missing.  The crash recovery succeeds if the hash table
is empty at the end.

Asim

---

From: Anastasia Lubennikova <[email-masked]>
Date: Thu, 26 Dec 2019 17:45:19 +0300

20.09.2019 15:23, Asim R P wrote:
> On Thu, Sep 19, 2019 at 5:29 PM Asim R P <[email-masked] 
> <mailto:[email-masked]>> wrote:
> >
> missing database directory and a missing tablespace directory.  And 
> also add logic to forget missing directories during tablespace drop.  
> I am working on it.
>
> Please find attached a solution that builds on what Paul has propose.  
> A hash table, similar to the invalid page hash table is used to track 
> missing directory references.  A missing directory may be a tablespace 
> or a database, based on whether the tablespace is found missing or the 
> source database is found missing.  The crash recovery succeeds if the 
> hash table is empty at the end.
>
The v6-0003 patch had merge conflicts due to the recent 
'xl_dbase_drop_rec' change, so I rebased it.
See v7-0003 in attachment. Changes are pretty straightforward, though It 
would be great, if you could check them once more.

Newly introduced test 4 in t/011_crash_recovery.pl fails without the 
patch and passes with it.
It seems to me that everything is fine, so I mark it "Ready For Committer"

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 9 Jan 2020 21:22:45 -0300

I looked at this a little while and was bothered by the perl changes; it
seems out of place to have RecursiveCopy be thinking about tablespaces,
which is way out of its league.  So I rewrote that to use a callback:
the PostgresNode code passes a callback that's in charge to handle the
case of a symlink.  Things look much more in place with that.  I didn't
verify that all places that should use this are filled.

In 0002 I found adding a new function unnecessary: we can keep backwards
compat by checking 'ref' of the third argument.  With that we don't have
to add a new function.  (POD changes pending.)

I haven't reviewed 0003.

v8 of all these patches attached.

"git am" told me your 0001 was in unrecognized format.  It applied fine
with "patch".  I suggest that if you're going to submit a series with
commit messages and all, please use "git format-patch" with the same
"-v" argument (9 in this case) for all patches.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 10 Jan 2020 10:43:35 -0300

> to add a new function.  (POD changes pending.)

I forgot to add that something in these changes is broken (probably the
symlink handling callback) so the tests fail, but I couldn't stay away
from my daughter's birthday long enough to figure out what or how.  I'm
on something else today, so if one of you can research and submit fixed
versions, that'd be great.

Thanks,

---

From: Paul Guo <[email-masked]>
Date: Mon, 13 Jan 2020 18:27:16 +0800

> versions, that'd be great.

I spent some time on this before getting off work today.

With below fix, the 4th test is now ok but the 5th (last one) hangs due to
panic.

(gdb) bt
#0  0x0000003397e32625 in raise () from /lib64/libc.so.6
#1  0x0000003397e33e05 in abort () from /lib64/libc.so.6
#2  0x0000000000a90506 in errfinish (dummy=0) at elog.c:590
#3  0x0000000000a92b4b in elog_finish (elevel=22, fmt=0xb2d580 "cannot find
directory %s tablespace %d database %d") at elog.c:1465
#4  0x000000000057aa0a in XLogLogMissingDir (spcNode=16384, dbNode=0,
path=0x1885100 "pg_tblspc/16384/PG_13_202001091/16389") at xlogutils.c:104
#5  0x000000000065e92e in dbase_redo (record=0x1841568) at dbcommands.c:2225
#6  0x000000000056ac94 in StartupXLOG () at xlog.c:7200

diff --git a/src/include/commands/dbcommands.h
b/src/include/commands/dbcommands.h
index b71b400e700..f8f6d5ffd03 100644
--- a/src/include/commands/dbcommands.h
+++ b/src/include/commands/dbcommands.h
@@ -19,8 +19,6 @@
 #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, bool force);
 extern void DropDatabase(ParseState *pstate, DropdbStmt *stmt);
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index e6e7ea505d9..4eef8bb1985 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -615,11 +615,11 @@ sub _srcsymlink
    my $srcrealdir = readlink($srcpath);

    opendir(my $dh, $srcrealdir);
-   while (readdir $dh)
+   while (my $entry = (readdir $dh))
    {
-       next if (/^\.\.?$/);
-       my $spath = "$srcrealdir/$_";
-       my $dpath = "$dstrealdir/$_";
+       next if ($entry eq '.' or $entry eq '..');
+       my $spath = "$srcrealdir/$entry";
+       my $dpath = "$dstrealdir/$entry";
        RecursiveCopy::copypath($spath, $dpath);
    }
    closedir $dh;

---

From: Paul Guo <[email-masked]>
Date: Wed, 15 Jan 2020 18:18:15 +0800

I further fixed the last test failure (due to a small bug in the test, not
in code). Attached are the new patch series. Let's see the CI pipeline
result.

---

From: Fujii Masao <[email-masked]>
Date: Tue, 28 Jan 2020 00:24:37 +0900

On 2020/01/15 19:18, Paul Guo wrote:
> I further fixed the last test failure (due to a small bug in the test, not in code). Attached are the new patch series. Let's see the CI pipeline result.

Thanks for updating the patches!

I started reading the 0003 patch.

The approach that the 0003 patch uses is not the perfect solution.
If the standby crashes after tblspc_redo() removes the directory and before
its subsequent COMMIT record is replayed, PANIC error would occur since
there can be some unresolved missing directory entries when we reach the
consistent state. The problem would very rarely happen, though...
Just idea; calling XLogFlush() to update the minimum recovery point just
before tblspc_redo() performs destroy_tablespace_directories() may be
safe and helpful for the problem?

-		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);

If the patch is for the bug fix and would be back-ported, the above change
would lead to change pg_waldump's output for CREATE/DROP DATABASE between
minor versions. IMO it's better to avoid such change and separate the above
as a separate patch only for master.

-			appendStringInfo(buf, " %u/%u",
-							 xlrec->tablespace_ids[i], xlrec->db_id);
+		{
+			dbpath1 = GetDatabasePath(xlrec->db_id, xlrec->tablespace_ids[i]);
+			appendStringInfo(buf,  "%s", dbpath1);
+			pfree(dbpath1);
+		}

Same as above.

BTW, the above "%s" should be " %s", i.e., a space character needs to be
appended to the head of "%s".

+			get_parent_directory(parent_path);
+			if (!(stat(parent_path, &st) == 0 && S_ISDIR(st.st_mode)))
+			{
+				XLogLogMissingDir(xlrec->tablespace_id, InvalidOid, dst_path);

The third argument of XLogLogMissingDir() should be parent_path instead of
dst_path?

+	if (hash_search(missing_dir_tab, &key, HASH_REMOVE, NULL) == NULL)
+		elog(DEBUG2, "dir %s tablespace %d database %d is not missing",
+			 path, spcNode, dbNode);

I think that this elog() is useless and rather confusing.

+		XLogForgetMissingDir(xlrec->ts_id, InvalidOid, "");

The third argument should be set to the actual path instead of an empty
string. Otherwise XLogForgetMissingDir() may emit a confusing DEBUG2
message. Or the third argument of XLogForgetMissingDir() should be removed
and the path in the DEBUG2 message should be calculated from the spcNode
and dbNode in the hash entry in XLogForgetMissingDir().

+#include "common/file_perm.h"

This seems not necessary.

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

---

From: Fujii Masao <[email-masked]>
Date: Wed, 25 Mar 2020 14:52:10 +0900

> I started reading the 0003 patch.

I marked this patch as Waiting on Author in CF because there is no update
since my last review comments. Could you mark it as Needs Review again
if you post the updated version of the patch.

Regards,

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 7 Jul 2020 23:12:30 +0200

> On 25 Mar 2020, at 06:52, Fujii Masao <[email-masked]> wrote:
> 

This thread has been stalled since effectively January, so I'm marking this
patch Returned with Feedback.  Feel free to open a new entry once the review
comments have been addressed.

cheers ./daniel

---

From: Paul Guo <[email-masked]>
Date: Wed, 8 Jul 2020 12:56:44 +0000

Looks like my previous reply was held for moderation (maybe due to my new email address).
I configured my pg account today using the new email address. I guess this email would be
held for moderation.

I’m now replying my previous reply email and attaching the new patch series.

On Jul 6, 2020, at 10:18 AM, Paul Guo <[email-masked]<mailto:[email-masked]>> wrote:

Thanks for the review. I’m now re-picking up the work. I modified the code following the comments.
Besides, I tweaked the test code a bit. There are several things I’m not 100% sure. Please see
my replies below.

On Jan 27, 2020, at 11:24 PM, Fujii Masao <[email-masked]<mailto:[email-masked]>> wrote:

Yes looks like an issue. My understanding is the below scenario.

XLogLogMissingDir()

XLogFlush() in redo (e.g. in a commit redo).  <- create a minimum recovery point (we call it LSN_A).

tblspc_redo()->XLogForgetMissingDir()
       <- If we panic immediately after we remove the directory in tblspc_redo()
       <- when we do replay during crash-recovery, we will check consistency at LSN_A and thus PANIC inXLogCheckMissingDirs()

commit

We should add a XLogFlush() in tblspc_redo(). This brings several other questions to my minds also.

1. Should we call XLogFlush() in dbase_redo()  for XLOG_DBASE_DROP also?
   It calls both XLogDropDatabase() and XLogForgetMissingDir, which seem to have this issue also?

2. xact_redo_abort() calls DropRelationFiles() also. Why do not we call XLogFlush() there?

I know we do not want wal format between minor releases, but does wal description string change
between minor releases affect users? Anyone I’ll extract this part into a separate patch in the series
since this change is actually independent of the other changes..

Same as above.

BTW, the above "%s" should be " %s", i.e., a space character needs to be
appended to the head of "%s”.

OK

The argument is for debug message printing so both should be fine, but admittedly we are
logging for the tablespace directory so parent_path might be better.

OK. Modified.

I’m now removing the third argument. Use GetDatabasePath() to get the path if database did I snot InvalidOid.

Right.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 05 Jan 2021 10:07:24 +0900 (JST)

At Wed, 8 Jul 2020 12:56:44 +0000, Paul Guo <[email-masked]> wrote in 
> safe and helpful for the problem?

It seems to me that what the current patch does is too complex.  What
we need to do here is to remember every invalid operation then forget
it when the prerequisite object is dropped.

When a table space is dropped before consistency is established, we
don't need to care what has been performed inside the tablespace.  In
this perspective, it is enough to remember tablespace ids when failed
to do something inside it due to the absence of the tablespace and
then forget it when we remove it.  We could remember individual
database id to show them in error messages, but I'm not sure it's
useful.  The reason log_invalid_page records block numbers is to allow
the machinery handle partial table truncations, but this is not the
case since dropping tablespace cannot leave some of containing
databases.

As the result, we won't see an unresolved invalid operations in a
dropped tablespace.

Am I missing something?

dbase_redo:
+      if (!(stat(parent_path, &st) == 0 && S_ISDIR(st.st_mode)))
+      {
+        XLogRecordMissingDir(xlrec->tablespace_id, InvalidOid, parent_path);

This means "record the belonging table space directory if it is not
found OR it is not a directory". The former can be valid but the
latter is unconditionally can not (I don't think we bother considering
symlinks there).

+    /*
+     * Source directory may be missing.  E.g. the template database used
+     * for creating this database may have been dropped, due to reasons
+     * noted above.  Moving a database from one tablespace may also be a
+     * partner in the crime.
+     */
+    if (!(stat(src_path, &st) == 0 && S_ISDIR(st.st_mode)))
+    {
+      XLogLogMissingDir(xlrec->src_tablespace_id, xlrec->src_db_id, src_path);

This is a part of *creation* of the target directory. Lack of the
source directory cannot be valid even if the source directory is
dropped afterwards in the WAL stream and we can allow that if the
*target* tablespace is dropped afterwards. As the result, as I
mentioned above, we don't need to record about the database directory.

By the way the name XLogLogMiss.. is somewhat confusing. How about
XLogReportMissingDir (named after report_invalid_page).

regards.

---

From: Paul Guo <[email-masked]>
Date: Wed, 27 Jan 2021 08:36:22 +0000

Thanks for the review, please see the replies below.

> On Jan 5, 2021, at 9:07 AM, Kyotaro Horiguchi <[email-masked]> wrote:
> 

Yes, removing the database id from the hash key in the log/forget code should
be usually fine, but the previous code does stricter/safer checking.

Consider the scenario:

CREATE DATABASE newdb1 TEMPLATE template_db1;
CREATE DATABASE newdb2 TEMPLATE template_db2; <- in case the template_db2 database directory is missing abnormally somehow.
DROP DATABASE template_db1;

The previous code could detect this but if we remove the database id in the code,
this bad scenario is skipped.

> latter is unconditionally can not (I don't think we bother considering

Again this is a safer check, in the case the parent_path is a file for example somehow,
we should panic finally for the case and let the user checks and then does recovery again.

> XLogReportMissingDir (named after report_invalid_page).

Agree with you.

Also your words remind me that we should skip the checking if the consistency point
is reached.

Here is a git diff against the previous patch. I’ll send out the new rebased patches after
the consensus is reached.

diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 7ade385965..c8fe3fe228 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -90,7 +90,7 @@ typedef struct xl_missing_dir
 static HTAB *missing_dir_tab = NULL;

 void
-XLogLogMissingDir(Oid spcNode, Oid dbNode, char *path)
+XLogReportMissingDir(Oid spcNode, Oid dbNode, char *path)
 {
 	xl_missing_dir_key key;
 	bool found;
@@ -103,16 +103,6 @@ XLogLogMissingDir(Oid spcNode, Oid dbNode, char *path)
 	 */
 	Assert(OidIsValid(spcNode));

-	if (reachedConsistency)
-	{
-		if (dbNode == InvalidOid)
-			elog(PANIC, "cannot find directory %s (tablespace %d)",
-				 path, spcNode);
-		else
-			elog(PANIC, "cannot find directory %s (tablespace %d database %d)",
-				 path, spcNode, dbNode);
-	}
-
 	if (missing_dir_tab == NULL)
 	{
 		/* create hash table when first needed */
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index fbff422c3b..7bd6d4efd9 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -2205,7 +2205,7 @@ dbase_redo(XLogReaderState *record)
 						(errmsg("some useless files may be left behind in old database directory \"%s\"",
 								dst_path)));
 		}
-		else
+		else if (!reachedConsistency)
 		{
 			/*
 			 * It is possible that drop tablespace record appearing later in
@@ -2221,7 +2221,7 @@ dbase_redo(XLogReaderState *record)
 			get_parent_directory(parent_path);
 			if (!(stat(parent_path, &st) == 0 && S_ISDIR(st.st_mode)))
 			{
-				XLogLogMissingDir(xlrec->tablespace_id, InvalidOid, parent_path);
+				XLogReportMissingDir(xlrec->tablespace_id, InvalidOid, parent_path);
 				skip = true;
 				ereport(WARNING,
 						(errmsg("skipping create database WAL record"),
@@ -2239,9 +2239,10 @@ dbase_redo(XLogReaderState *record)
 		 * noted above.  Moving a database from one tablespace may also be a
 		 * partner in the crime.
 		 */
-		if (!(stat(src_path, &st) == 0 && S_ISDIR(st.st_mode)))
+		if (!(stat(src_path, &st) == 0 && S_ISDIR(st.st_mode)) &&
+			!reachedConsistency)
 		{
-			XLogLogMissingDir(xlrec->src_tablespace_id, xlrec->src_db_id, src_path);
+			XLogReportMissingDir(xlrec->src_tablespace_id, xlrec->src_db_id, src_path);
 			skip = true;
 			ereport(WARNING,
 					(errmsg("skipping create database WAL record"),
@@ -2311,7 +2312,8 @@ dbase_redo(XLogReaderState *record)
 						(errmsg("some useless files may be left behind in old database directory \"%s\"",
 								dst_path)));

-			XLogForgetMissingDir(xlrec->tablespace_ids[i], xlrec->db_id);
+			if (!reachedConsistency)
+				XLogForgetMissingDir(xlrec->tablespace_ids[i], xlrec->db_id);

 			pfree(dst_path);
 		}
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 294c9676b4..15eaa757cc 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -1534,7 +1534,8 @@ tblspc_redo(XLogReaderState *record)
 	{
 		xl_tblspc_drop_rec *xlrec = (xl_tblspc_drop_rec *) XLogRecGetData(record);

-		XLogForgetMissingDir(xlrec->ts_id, InvalidOid);
+		if (!reachedConsistency)
+			XLogForgetMissingDir(xlrec->ts_id, InvalidOid);

 		XLogFlush(record->EndRecPtr);

diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index da561af5ab..6561d9cebe 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -23,7 +23,7 @@ extern void XLogDropDatabase(Oid dbid);
 extern void XLogTruncateRelation(RelFileNode rnode, ForkNumber forkNum,
 								 BlockNumber nblocks);

-extern void XLogLogMissingDir(Oid spcNode, Oid dbNode, char *path);
+extern void XLogReportMissingDir(Oid spcNode, Oid dbNode, char *path);
 extern void XLogForgetMissingDir(Oid spcNode, Oid dbNode);
 extern void XLogCheckMissingDirs(void);

diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.pl
index 748200ebb5..95eb6d26cc 100644
--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -141,7 +141,7 @@ $node_master->wait_for_catchup($node_standby, 'replay',
 $node_standby->safe_psql('postgres', 'CHECKPOINT');

 # Do immediate shutdown just after a sequence of CREAT DATABASE / DROP
-# DATABASE / DROP TABLESPACE. This causes CREATE DATBASE WAL records
+# DATABASE / DROP TABLESPACE. This causes CREATE DATABASE WAL records

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 27 Mar 2021 11:23:16 -0300

On 2021-Jan-27, Paul Guo wrote:

> Here is a git diff against the previous patch. I’ll send out the new
> rebased patches after the consensus is reached.

Hmm, can you post a rebased set, where the points under discussion
are marked in XXX comments explaining what the issue is?  This thread is
long and old ago that it's pretty hard to navigate the whole thing in
order to find out exactly what is being questioned.

I think 0004 can be pushed without further ado, since it's a clear and
simple fix.  0001 needs a comment about the new parameter in
RecursiveCopy's POD documentation.

As I understand, this is a backpatchable bug-fix.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

---

From: Paul Guo <[email-masked]>
Date: Tue, 30 Mar 2021 07:12:19 +0000

>    order to find out exactly what is being questioned.

OK. Attached are the rebased version that includes the change I discussed
in my previous reply. Also added POD documentation change for RecursiveCopy,
and modified the patch to use the backup_options introduced in
081876d75ea15c3bd2ee5ba64a794fd8ea46d794 for tablespace mapping.

>    RecursiveCopy's POD documentation.

Yeah, 0004 is no any risky. One concern seemed to be the compatibility of some
WAL dump/analysis tools(?). I have no idea about this. But if we do not backport
0004 we do not seem to need to worry about this.

>    As I understand, this is a backpatchable bug-fix.

Yes.

Thanks.

---

From: Ibrar Ahmed <[email-masked]>
Date: Sat, 10 Jul 2021 00:37:57 +0500

On Tue, Mar 30, 2021 at 12:12 PM Paul Guo <[email-masked]> wrote:

> Patch does not apply successfully,
http://cfbot.cputube.org/patch_33_2161.log

Can you please rebase the patch.

-- 
Ibrar Ahmed

---

From: Paul Guo <[email-masked]>
Date: Thu, 5 Aug 2021 10:20:44 +0000

Rebased.

---

From: Robert Haas <[email-masked]>
Date: Tue, 10 Aug 2021 16:56:37 -0400

> Rebased.

The commit message for 0001 is not clear enough for me to understand
what problem it's supposed to be fixing. The code comments aren't
really either. They make it sound like there's some problem with
copying symlinks but mostly they just talk about callbacks, which
doesn't really help me understand what problem we'd have if we just
didn't commit this (or reverted it later).

I am not really convinced by Álvaro's claim that 0004 is a "fix"; I
think I'd call it an improvement. But either way I agree that could
just be committed.

I haven't analyzed 0002 and 0003 yet.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Paul Guo <[email-masked]>
Date: Wed, 11 Aug 2021 15:58:51 +0800

> didn't commit this (or reverted it later).

Thanks for reviewing. Let me explain a bit. The patch series includes
four patches.

0001 and 0002 are test changes for the fix (0003).
   - 0001 is the test framework change that's needed by 0002.
   - 0002 is the test for the code fix (0003).
0003 is the code change and the commit message explains the issue in detail.
0004 as said is a small enhancement which is a bit independent of the
previous patches.

Basically the issue is that without the fix crash recovery might fail
relevant to tablespace.
Here is the log after I run the tests in 0001/0002 without the 0003 fix.

2021-08-04 10:00:42.231 CST [875] FATAL:  could not create directory
"pg_tblspc/16385/PG_15_202107261/16390": No such file or directory
2021-08-04 10:00:42.231 CST [875] CONTEXT:  WAL redo at 0/3001320 for
Database/CREATE: copy dir base/1 to
pg_tblspc/16385/PG_15_202107261/16390

-- 
Paul Guo (Vmware)

---

From: Robert Haas <[email-masked]>
Date: Wed, 11 Aug 2021 08:59:20 -0400

> Here is the log after I run the tests in 0001/0002 without the 0003 fix.

I do understand all of this, but I (or whoever might commit this)
needs to also be able to understand specifically what each patch is
doing.

---

From: Tom Lane <[email-masked]>
Date: Fri, 24 Sep 2021 14:14:52 -0400

> I haven't analyzed 0002 and 0003 yet.

I took a quick look through this:

* I don't like 0001 either, though it seems like the issue is mostly
documentation.  sub _srcsymlink should have a comment explaining
what it's doing and why.  The documentation of copypath's new parameter
seems like gobbledegook too --- I suppose it should read more like
"By default, copypath fails if a source item is a symlink.  But if
B<srcsymlinkfn> is provided, that subroutine is called to process any
symlink."

* I'm allergic to 0002's completely undocumented changes to
poll_query_until, especially since I don't see anything in the
patch that actually uses them.  Can't we just drop these diffs
in PostgresNode.pm?  BTW, the last error message in the patch,
talking about a 5-second timeout, seems wrong.  With or without
these changes, poll_query_until's default timeout is 180 sec.
The actual test case might be okay other than that nit and a
comment typo or two.

* 0003 might actually be okay.  I've not read it line-by-line,
but it seems like it's implementing a sane solution and it's
adequately commented.

* I'm inclined to reject 0004 out of hand, because I don't
agree with what it's doing.  The purpose of the rmgrdesc
functions is to show you what is in the WAL records, and
everywhere else we interpret that as "show the verbatim,
numeric field contents".  heapdesc.c, for example, doesn't
attempt to look up the name of the table being operated on.
0004 isn't adhering to that style, and aside from being
inconsistent I'm afraid that it's adding failure modes
we don't want.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 4 Nov 2021 13:34:33 +0100

> On 24 Sep 2021, at 20:14, Tom Lane <[email-masked]> wrote:
> 

This patch again fails to apply (seemingly from the Perl namespace work on the
testcode), and needs a few updates as per the above review.

--
Daniel Gustafsson		https://vmware.com/

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 08 Nov 2021 17:55:16 +0900 (JST)

At Thu, 4 Nov 2021 13:34:33 +0100, Daniel Gustafsson <[email-masked]> wrote in 
> testcode), and needs a few updates as per the above review.

Rebased the latest patch removing some of the chages.

0001: (I don't remember about this, though) I don't see how to make it
work on Windows.  Anyway the next step would be to write comments.

0002: I didin't see it in details and didn't check if it finds the
issue but it actually scceeds with the fix.  The change to
poll_query_until is removed since it doesn't seem actually used.

0003: The fix. I didn't touch this.

0004: Removed at all. I agree to Tom. (And I faintly remember that I
said something like that.)

regards.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 9 Nov 2021 12:51:15 +0900

On Mon, Nov 08, 2021 at 05:55:16PM +0900, Kyotaro Horiguchi wrote:

I have quickly looked at the patch set.

> work on Windows.  Anyway the next step would be to write comments.

Look at Utils.pm where we have dir_symlink, then.  symlink() does not
work on WIN32, so we have a wrapper that uses junction points.  FWIW,
I don't like much the behavior you are enforcing in init_from_backup
when coldly copying a source path, but I have not looked enough at the
patch set to have a strong opinion about this part, either.

> 0002: I didn't see it in details and didn't check if it finds the

+# Create tablespace
+my $dropme_ts_master1 = PostgreSQL::Test::Utils::tempdir();
+$dropme_ts_master1 =
PostgreSQL::Test::Utils::perl2host($dropme_ts_master1);
+my $dropme_ts_master2 = PostgreSQL::Test::Utils::tempdir();
+$dropme_ts_master2 =
PostgreSQL::Test::Utils::perl2host($dropme_ts_master2);
+my $source_ts_master = PostgreSQL::Test::Utils::tempdir();
+$source_ts_master =
PostgreSQL::Test::Utils::perl2host($source_ts_master);
+my $target_ts_master = PostgreSQL::Test::Utils::tempdir();
+$target_ts_master =
PostgreSQL::Test::Utils::perl2host($target_ts_master);

Rather than creating N temporary directories, it would be simpler to
create only one, and have subdirs in it for the rest?  It seems to me
that it would make debugging much easier.  The uses of perl2host()
seem sufficient.
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 09 Nov 2021 17:05:49 +0900 (JST)

At Tue, 9 Nov 2021 12:51:15 +0900, Michael Paquier <[email-masked]> wrote in 
> patch set to have a strong opinion about this part, either.

Thanks for the info. If we can handle symlink on Windows, we don't
need to have a cold copy.

> that it would make debugging much easier.  The uses of perl2host()

Thanks for the suggestion.  My eyeballs got hopping around looking
that part so I gave up looking there in more detail:p I agree to that.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 10 Nov 2021 17:14:11 +0900 (JST)

At Tue, 09 Nov 2021 17:05:49 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> need to have a cold copy.

I bumped into the good-old 100-byte limit of the (v7?) tar format on
which pg_basebackup is depending. It is unlikely in the real world but
I think it is quite common in developping environment.  The tablespace
directory path in my dev environment was 110 chacters-long.  As small
as 10 bytes but it's quite annoying to chip off that number of bytes
from the path..

regards.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 10 Nov 2021 09:14:30 -0300

> as 10 bytes but it's quite annoying to chip off that number of bytes

Can you use PostgreSQL::Test::Utils::tempdir_short() for those
tablespaces?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 11 Nov 2021 11:13:52 +0900 (JST)

At Wed, 10 Nov 2021 09:14:30 -0300, Alvaro Herrera <[email-masked]> wrote in 
> Can you use PostgreSQL::Test::Utils::tempdir_short() for those

Thanks for the suggestion!

It works for a live cluster. But doesn't work for backups, since I
find no way to relate a tablespace directory with a backup directory
not using a symlink.  One way would be taking a backup with tentative
tablespace directory in the short-named temporary directory then move
it into the backup direcotry. I'm going that way for now.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 12 Nov 2021 16:43:27 +0900 (JST)

At Thu, 11 Nov 2021 11:13:52 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> it into the backup direcotry. I'm going that way for now.

This is that.

0001 adds several routines to handle tablespace directories, and adds
tablespace support to backup/_backup_fs.

We don't know an oid corresponding to a tablespace directory before
actually assigning the oid to the tablespace.  So we cannot name a
tablespace directory after the oid.  On the other hand, after defining
the tablespace, cold data files don't tell the real directory name of
the tablespace directory for an oid or a tablespace name, unless we
have readlink.

The function dir_readlink added to Utils.pm is that. Honestly I don't
like the way function works. It uses "cmd /c "dir /A:L $dir"" to
collect information of junctions. I'm not sure that the type label
"<JUNCTION>" is immutable among locales but at least it is shown as
"<JUNCTION>" on Japanese (CP-932) environment. I didn't actually
tested it on Windows and msys environment ...yet.

Premising the availability of the function, we can name tablespace
directories from meaningful words.

The directory to store tablespace directories can be a temporary
directory, but with that way it is needed to create a symlink to find
those directories from a backup.  I chose to place tablespace
directories directly under backup directory.

The attached first file is a revised (or remade) version of tablespace
support for TAP test.

The second is the version adapted to the revised framework. (I
confirmed that the test actually detects the error.)

The third is not changed at all.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 24 Dec 2021 19:21:59 +0900 (JST)

Just a complaint..

At Fri, 12 Nov 2021 16:43:27 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> tested it on Windows and msys environment ...yet.

Active perl cannot be installed because of (perhaps) a powershell
version issue... Annoying..

https://community.activestate.com/t/please-update-your-powershell-install-scripts/7897

regards.