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

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 05 Apr 2022 17:18:57 +0900 (JST)

At Tue, 05 Apr 2022 16:38:06 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> At Tue, 05 Apr 2022 11:16:44 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> > So, I have the following points in my mind for now.
> > 
> > - We create the directory "since we know it is just tentative state".
> > 
> > - Then, check that no directory in pg_tblspc when reaching consistency
> >   when allow_in_place_tablespaces is false.
> > 
> > - Leave the log_invalid_page() mechanism alone as it is always result
> >   in a corrpt page if a differential WAL record is applied on a newly
> >   created page that should have been exist.
> > 
> > However, while working on it, I found that I found that recovery faces
> > missing tablespace directories *after* reaching consistency.  I'm
> > examining that further.
> 
> Okay, it was my thinko.
> 
> This is the first cut of the above.

It had an unused variable for Windows.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 1e7f5e5e10ea504e168d01b3db8be12c2f63b6d6 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 5 Apr 2022 15:31:45 +0900
Subject: [PATCH v23] Fix replay of create database records on standby

Crash recovery on standby may encounter missing directories when
replaying create database WAL records.  Prior to this patch, the
standby would fail to recover in such a case.  However, the
directories could be legitimately missing.  Consider a sequence of WAL
records as follows:

    CREATE DATABASE
    DROP DATABASE
    DROP TABLESPACE

If, after replaying the last WAL record and removing the tablespace
directory, the standby crashes and has to replay the create database
record again, the crash recovery must be able to move on.

This patch allows missing tablespaces to be created during recovery
before reaching consistency.  The tablespaces are created as real
directories that should not exists but will be removed until reaching
consistency. CheckRecoveryConsistency is responsible to make sure they
have disappeared.

Similar to log_invalid_page mechanism, the GUC ignore_invalid_pages
turns into PANIC errors detected by this patch into WARNING, which
allows continueing recovery.

Diagnosed-by: Paul Guo <[email-masked]>
Author: Paul Guo <[email-masked]>
Author: Kyotaro Horiguchi <[email-masked]>
Author: Asim R Praveen <[email-masked]>
Discussion: https://postgr.es/m/CAEET0ZGx9AvioViLf7nbR_8tH9-=[email-masked]
---
 doc/src/sgml/config.sgml                    |   5 +-
 src/backend/access/transam/xlogrecovery.c   |  55 +++++++
 src/backend/commands/dbcommands.c           |  71 +++++++++
 src/backend/commands/tablespace.c           |  28 +---
 src/backend/utils/misc/guc.c                |   8 +-
 src/include/access/xlogutils.h              |   2 +
 src/test/recovery/t/029_replay_tsp_drops.pl | 155 ++++++++++++++++++++
 7 files changed, 292 insertions(+), 32 deletions(-)
 create mode 100644 src/test/recovery/t/029_replay_tsp_drops.pl

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 43e4ade83e..c22229468b 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11228,11 +11228,12 @@ LOG:  CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
       <listitem>
        <para>
         If set to <literal>off</literal> (the default), detection of
-        WAL records having references to invalid pages during
+        WAL records having references to invalid pages or
+        WAL records resulting in invalid directory operations during
         recovery causes <productname>PostgreSQL</productname> to
         raise a PANIC-level error, aborting the recovery. Setting
         <varname>ignore_invalid_pages</varname> to <literal>on</literal>
-        causes the system to ignore invalid page references in WAL records
+        causes the system to ignore invalid actions caused by such WAL records
         (but still report a warning), and continue the recovery.
         This behavior may <emphasis>cause crashes, data loss,
         propagate or hide corruption, or other serious problems</emphasis>.
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 8d2395dae2..18dcc452ca 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1986,6 +1986,53 @@ xlogrecovery_redo(XLogReaderState *record, TimeLineID replayTLI)
 	}
 }
 
+/*
+ * Makes sure that ./pg_tblspc directory doesn't contain a real directory.
+ *
+ * This is intended to be called after reaching consistency.
+ * ignore_invalid_pages=on turns into the PANIC error into WARNING so that
+ * recovery can continue.
+ * 
+ * Note that it is the normal behavior when allow_in_place_tablespaces=on, but
+ * we don't bother caring that case since it is a developer-only setting.
+ */
+static void
+CheckTablespaceDirectory(void)
+{
+	char *tblspc_path = "./pg_tblspc";
+	DIR		   *dir;
+	struct dirent *de;
+
+	dir = AllocateDir(tblspc_path);
+	while ((de = ReadDir(dir, tblspc_path)) != NULL)
+	{
+		char	path[MAXPGPATH];
+		char   *p;
+#ifndef WIN32
+		struct stat st;
+#endif
+
+		/* Skip entries of non-oid names */
+		for (p = de->d_name; *p && isdigit(*p); p++);
+		if (*p)
+			continue;
+
+		snprintf(path, MAXPGPATH, "%s/%s", tblspc_path, de->d_name);
+
+#ifndef WIN32
+		if (lstat(path, &st) < 0)
+			ereport(ERROR, errcode_for_file_access(),
+					errmsg("could not stat file \"%s\": %m", path));
+
+		if (!S_ISLNK(st.st_mode))
+#else
+		if (!pgwin32_is_junction(path))
+#endif
+			elog(ignore_invalid_pages ? WARNING : PANIC,
+				 "real directory found in pg_tblspc directory: %s", de->d_name);
+	}
+}
+
 /*
  * Checks if recovery has reached a consistent state. When consistency is
  * reached and we have a valid starting standby snapshot, tell postmaster
@@ -2051,6 +2098,14 @@ CheckRecoveryConsistency(void)
 		ereport(LOG,
 				(errmsg("consistent recovery state reached at %X/%X",
 						LSN_FORMAT_ARGS(lastReplayedEndRecPtr))));
+
+		/*
+		 * Check that pg_tblspc doesn't contain a real
+		 * directory. Database/CREATE_* records may create a tablespace
+		 * directory that should have been removed until consistency is
+		 * reached.
+		 */
+		CheckTablespaceDirectory();
 	}
 
 	/*
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index df16533901..910101da01 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -30,6 +30,7 @@
 #include "access/tableam.h"
 #include "access/xact.h"
 #include "access/xloginsert.h"
+#include "access/xlogrecovery.h"
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/dependency.h"
@@ -47,6 +48,7 @@
 #include "commands/defrem.h"
 #include "commands/seclabel.h"
 #include "commands/tablespace.h"
+#include "common/file_perm.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -62,6 +64,7 @@
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/guc.h"
 #include "utils/pg_locale.h"
 #include "utils/relmapper.h"
 #include "utils/snapmgr.h"
@@ -135,6 +138,7 @@ static void CreateDirAndVersionFile(char *dbpath, Oid dbid, Oid tsid,
 									bool isRedo);
 static void CreateDatabaseUsingFileCopy(Oid src_dboid, Oid dboid, Oid src_tsid,
 										Oid dst_tsid);
+static void maybe_create_directory(char *path);
 
 /*
  * Create a new database using the WAL_LOG strategy.
@@ -3003,6 +3007,43 @@ get_database_name(Oid dbid)
 	return result;
 }
 
+/*
+ * maybe_create_directory()
+ *
+ * During recovery, there's a case where we validly need to recover a missing
+ * tablespace directory so that recovery can continue.  This happens when
+ * recovery wants to create a database but the holding tablespace has been
+ * removed before the server stopped.  Since we expect that the directory will
+ * be gone before reaching recovery consistency, and we have no knowledge about
+ * the tablespace other than its OID here, we create a real directory under
+ * pg_tblspc here instead of restoring the symlink.  ignore_invalid_pages=on
+ * reduces the error level so that recovery can continue.
+ */
+static void
+maybe_create_directory(char *path)
+{
+	struct stat	st;
+
+	Assert(RecoveryInProgress());
+
+	if (stat(path, &st) == 0)
+		return;
+
+	/* XXX: Do we make sure that the path is under pg_tblspc? */
+
+	if (reachedConsistency && !ignore_invalid_pages)
+		ereport(PANIC,
+				errmsg("missing directory \"%s\"", path));
+
+	elog(reachedConsistency ? WARNING : DEBUG1,
+		 "creating missing directory: %s", path);
+
+	if (pg_mkdir_p(path, pg_dir_create_mode) != 0)
+		ereport(PANIC,
+				errmsg("could not create missing directory \"%s\": %m", path));
+}
+
+
 /*
  * DATABASE resource manager's routines
  */
@@ -3039,6 +3080,30 @@ dbase_redo(XLogReaderState *record)
 								dst_path)));
 		}
 
+		if (stat(dst_path, &st) < 0)
+		{
+			char *parent_path;
+
+			if (errno != ENOENT)
+				ereport(FATAL,
+						errmsg("could not stat directory \"%s\": %m",
+							   dst_path));
+
+			/* create the parent directory if needed and valid */
+			parent_path = pstrdup(dst_path);
+			get_parent_directory(parent_path);
+			maybe_create_directory(parent_path);
+		}
+
+		/*
+		 * There's a case where the copy source directory is missing for the
+		 * same reason above.  Create the emtpy source directory so that
+		 * copydir below doesn't fail.  The directory will be dropped soon by
+		 * recovery.
+		 */
+		if (stat(src_path, &st) < 0 && errno == ENOENT)
+			maybe_create_directory(src_path);
+
 		/*
 		 * Force dirty buffers out to disk, to ensure source database is
 		 * up-to-date for the copy.
@@ -3057,9 +3122,15 @@ dbase_redo(XLogReaderState *record)
 		xl_dbase_create_wal_log_rec *xlrec =
 		(xl_dbase_create_wal_log_rec *) XLogRecGetData(record);
 		char	   *dbpath;
+		char	   *parent_path;
 
 		dbpath = GetDatabasePath(xlrec->db_id, xlrec->tablespace_id);
 
+		/* create the parent directory if needed and valid */
+		parent_path = pstrdup(dbpath);
+		get_parent_directory(parent_path);
+		maybe_create_directory(parent_path);
+
 		/* Create the database directory with the version file. */
 		CreateDirAndVersionFile(dbpath, xlrec->db_id, xlrec->tablespace_id,
 								true);
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 40514ab550..675f578dfe 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -155,8 +155,6 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
 				/* Directory creation failed? */
 				if (MakePGDirectory(dir) < 0)
 				{
-					char	   *parentdir;
-
 					/* Failure other than not exists or not in WAL replay? */
 					if (errno != ENOENT || !isRedo)
 						ereport(ERROR,
@@ -169,32 +167,8 @@ TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo)
 					 * continue by creating simple parent directories rather
 					 * than a symlink.
 					 */
-
-					/* create two parents up if not exist */
-					parentdir = pstrdup(dir);
-					get_parent_directory(parentdir);
-					get_parent_directory(parentdir);
-					/* Can't create parent and it doesn't already exist? */
-					if (MakePGDirectory(parentdir) < 0 && errno != EEXIST)
-						ereport(ERROR,
-								(errcode_for_file_access(),
-								 errmsg("could not create directory \"%s\": %m",
-										parentdir)));
-					pfree(parentdir);
-
-					/* create one parent up if not exist */
-					parentdir = pstrdup(dir);
-					get_parent_directory(parentdir);
-					/* Can't create parent and it doesn't already exist? */
-					if (MakePGDirectory(parentdir) < 0 && errno != EEXIST)
-						ereport(ERROR,
-								(errcode_for_file_access(),
-								 errmsg("could not create directory \"%s\": %m",
-										parentdir)));
-					pfree(parentdir);
-
 					/* Create database directory */
-					if (MakePGDirectory(dir) < 0)
+					if (pg_mkdir_p(dir, pg_dir_create_mode) < 0)
 						ereport(ERROR,
 								(errcode_for_file_access(),
 								 errmsg("could not create directory \"%s\": %m",
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9e8ab1420d..9134a73d3d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -42,6 +42,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "access/xlogrecovery.h"
+#include "access/xlogutils.h"
 #include "catalog/namespace.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_authid.h"
@@ -139,7 +140,6 @@ extern int	CommitSiblings;
 extern char *default_tablespace;
 extern char *temp_tablespaces;
 extern bool ignore_checksum_failure;
-extern bool ignore_invalid_pages;
 extern bool synchronize_seqscans;
 
 #ifdef TRACE_SYNCSCAN
@@ -1304,10 +1304,12 @@ static struct config_bool ConfigureNamesBool[] =
 		{"ignore_invalid_pages", PGC_POSTMASTER, DEVELOPER_OPTIONS,
 			gettext_noop("Continues recovery after an invalid pages failure."),
 			gettext_noop("Detection of WAL records having references to "
-						 "invalid pages during recovery causes PostgreSQL to "
+						 "invalid pages or WAL records resulting in invalid "
+						 "directory operations during "
+						 "recovery that cause PostgreSQL"
 						 "raise a PANIC-level error, aborting the recovery. "
 						 "Setting ignore_invalid_pages to true causes "
-						 "the system to ignore invalid page references "
+						 "the system to ignore those inconsistencies "
 						 "in WAL records (but still report a warning), "
 						 "and continue recovery. This behavior may cause "
 						 "crashes, data loss, propagate or hide corruption, "
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index 64708949db..d88661997f 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -54,6 +54,8 @@ typedef enum
 
 extern HotStandbyState standbyState;
 
+extern bool ignore_invalid_pages;
+
 #define InHotStandby (standbyState >= STANDBY_SNAPSHOT_PENDING)

diff --git a/src/test/recovery/t/029_replay_tsp_drops.pl b/src/test/recovery/t/029_replay_tsp_drops.pl
new file mode 100644
index 0000000000..b401ab8072
--- /dev/null
+++ b/src/test/recovery/t/029_replay_tsp_drops.pl
@@ -0,0 +1,155 @@
+
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+#
+# Tests relating to PostgreSQL crash recovery and redo
+#
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+sub test_tablespace
+{
+	my ($strategy) = @_;
+
+	my $node_primary = PostgreSQL::Test::Cluster->new("primary1_$strategy");
+	$node_primary->init(allows_streaming => 1);
+	$node_primary->start;
+	$node_primary->psql('postgres',
+			qq[
+				SET allow_in_place_tablespaces=on;
+				CREATE TABLESPACE dropme_ts1 LOCATION '';
+				CREATE TABLESPACE dropme_ts2 LOCATION '';
+				CREATE TABLESPACE source_ts  LOCATION '';
+				CREATE TABLESPACE target_ts  LOCATION '';
+				CREATE DATABASE template_db IS_TEMPLATE = true;
+			]);
+	my $backup_name = 'my_backup';
+	$node_primary->backup($backup_name);
+
+	my $node_standby = PostgreSQL::Test::Cluster->new("standby2_$strategy");
+	$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+	$node_standby->append_conf('postgresql.conf', "ignore_invalid_pages = on");	
+	$node_standby->start;
+
+	# Make sure connection is made
+	$node_primary->poll_query_until(
+		'postgres', 'SELECT count(*) = 1 FROM pg_stat_replication');
+
+	$node_standby->safe_psql('postgres', 'CHECKPOINT');
+
+	# Do immediate shutdown just after a sequence of CREAT DATABASE / DROP
+	# DATABASE / DROP TABLESPACE. This causes CREATE DATABASE WAL records
+	# to be applied to already-removed directories.
+	my $query = q[
+	CREATE DATABASE dropme_db1 WITH TABLESPACE dropme_ts1 STRATEGY=<STRATEGY>;
+	CREATE TABLE t (a int) TABLESPACE dropme_ts2;
+	CREATE DATABASE dropme_db2 WITH TABLESPACE dropme_ts2 STRATEGY=<STRATEGY>;
+	CREATE DATABASE moveme_db TABLESPACE source_ts STRATEGY=<STRATEGY>;
+	ALTER DATABASE moveme_db SET TABLESPACE target_ts;
+	CREATE DATABASE newdb TEMPLATE template_db STRATEGY=<STRATEGY>;
+	ALTER DATABASE template_db IS_TEMPLATE = false;
+	DROP DATABASE dropme_db1;
+	DROP TABLE t;
+	DROP DATABASE dropme_db2; DROP TABLESPACE dropme_ts2;
+	DROP TABLESPACE source_ts;
+	DROP DATABASE template_db;];
+
+	$query =~ s/<STRATEGY>/$strategy/g;
+	$node_primary->safe_psql('postgres', $query);
+	$node_primary->wait_for_catchup($node_standby, 'replay',
+									$node_primary->lsn('replay'));
+
+	# show "create missing directory" log message
+	$node_standby->safe_psql('postgres',
+							 "ALTER SYSTEM SET log_min_messages TO debug1;");
+	$node_standby->stop('immediate');
+	# Should restart ignoring directory creation error.
+	is($node_standby->start(fail_ok => 1), 1);
+	$node_standby->stop('immediate');
+}	
+
+test_tablespace("FILE_COPY");
+test_tablespace("WAL_LOG");
+
+# Ensure that a missing tablespace directory during create database
+# replay immediately causes panic if the standby has already reached
+# consistent state (archive recovery is in progress).  This is
+# effective only for CREATE DATABASE WITH STRATEGY=FILE_COPY.
+
+my $node_primary = PostgreSQL::Test::Cluster->new('primary2');
+$node_primary->init(allows_streaming => 1);
+$node_primary->start;
+
+# Create tablespace
+$node_primary->safe_psql('postgres', q[
+						 SET allow_in_place_tablespaces=on;
+						 CREATE TABLESPACE ts1 LOCATION '']);
+$node_primary->safe_psql('postgres', "CREATE DATABASE db1 WITH TABLESPACE ts1 STRATEGY=FILE_COPY");
+
+# Take backup
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+my $node_standby = PostgreSQL::Test::Cluster->new('standby3');
+$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+$node_standby->append_conf('postgresql.conf', "ignore_invalid_pages = on");	
+$node_standby->start;
+
+# Make sure standby reached consistency and starts accepting connections
+$node_standby->poll_query_until('postgres', 'SELECT 1', '1');
+
+# Remove standby tablespace directory so it will be missing when
+# replay resumes.
+my $tspoid = $node_standby->safe_psql('postgres',
+									  "SELECT oid FROM pg_tablespace WHERE spcname = 'ts1';");
+my $tspdir = $node_standby->data_dir . "/pg_tblspc/$tspoid";
+File::Path::rmtree($tspdir);
+
+my $logstart = get_log_size($node_standby);
+
+# Create a database in the tablespace and a table in default tablespace
+$node_primary->safe_psql('postgres',
+						q[CREATE TABLE should_not_replay_insertion(a int);
+						  CREATE DATABASE db2 WITH TABLESPACE ts1 STRATEGY=FILE_COPY;
+						  INSERT INTO should_not_replay_insertion VALUES (1);]);
+
+# Standby should fail and should not silently skip replaying the wal
+# In this test, PANIC turns into WARNING by ignore_invalid_pages.
+# Check the log messages instead of confirming standby failure.
+my $max_attempts = $PostgreSQL::Test::Utils::timeout_default;
+while ($max_attempts-- >= 0)
+{
+	last if (find_in_log(
+				 $node_standby,
+				 "WARNING:  creating missing directory: pg_tblspc/",
+				 $logstart));
+	sleep 1;
+}
+ok($max_attempts > 0, "invalid directory creation is detected");
+
+done_testing();
+
+
+# return the size of logfile of $node in bytes
+sub get_log_size
+{
+	my ($node) = @_;
+
+	return (stat $node->logfile)[7];
+}
+
+# find $pat in logfile of $node after $off-th byte
+sub find_in_log
+{
+	my ($node, $pat, $off) = @_;
+
+	$off = 0 unless defined $off;
+	my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile);
+	return 0 if (length($log) <= $off);
+
+	$log = substr($log, $off);
+
+	return $log =~ m/$pat/;
+}
-- 
2.27.0

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 13 Jul 2022 18:43:45 +0200

Not a review, just a preparatory rebase across some trivially
conflicting changes.  I also noticed that
src/test/recovery/t/031_recovery_conflict.pl, which was added two days
after v23 was sent, and which uses allow_in_place_tablespaces, bails out
because of the checks introduced by this patch, so I made the check
routine do nothing in that case.

Anyway, here's v24.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La conclusión que podemos sacar de esos estudios es que
no podemos sacar ninguna conclusión de ellos" (Tanenbaum)

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 14 Jul 2022 23:47:40 +0200

Here's a couple of fixups.  0001 is the same as before.  In 0002 I think
CheckTablespaceDirectory ends up easier to read if we split out the test
for validity of the link.  Looking at that again, I think we don't need
to piggyback on ignore_invalid_pages, which is already a stretch, so
let's not -- instead we can use allow_in_place_tablespaces if users need
a workaround.  So that's 0003 (this bit needs more than zero docs,
however.)

0004 is straightforward: let's check for bad directories before logging
about consistent state.

After all this, I'm not sure what to think of dbase_redo.  At line 3102,
is the directory supposed to exist or not?  I'm confused as to what is
the expected state at that point.  I rewrote this, but now I think my
rewrite continues to be confusing, so I'll have to think more about it.

Another aspect are the tests.  Robert described a scenario where the
previously committed version of this patch created trouble.  Do we have
a test case to cover that problematic case?  I think we should strive to
cover it, if possible.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 15 Jul 2022 16:30:59 +0900 (JST)

At Thu, 14 Jul 2022 23:47:40 +0200, Alvaro Herrera <[email-masked]> wrote in 
> Here's a couple of fixups.  0001 is the same as before.  In 0002 I think

Thanks!

+ #else
+ #endif

A regular file with an oid-name also causes this error. Doesn't
something like "unexpected non-(sym)link entry..." work?

> a workaround.  So that's 0003 (this bit needs more than zero docs,

The result of 0003 looks good.

0002:
+is_path_tslink(const char *path)

What the "ts" of tslink stands for? If it stands for tablespace, the
function is not specific for table spaces. We already have 

I'm not sure we need such correctness, but what is failing there is
lstat.  I found similar codes in two places in backend and one place
in frontend. So couldn't it be moved to /common and have a more
generic name?

-	dir = AllocateDir(tblspc_path);
-	while ((de = ReadDir(dir, tblspc_path)) != NULL)
+	dir = AllocateDir("pg_tblspc");
+	while ((de = ReadDir(dir, "pg_tblspc")) != NULL)

xlog.c uses the macro XLOGDIR. Why don't we define TBLSPCDIR?

-		for (p = de->d_name; *p && isdigit(*p); p++);
-		if (*p)
+		if (strspn(de->d_name, "0123456789") != strlen(de->d_name))
 			continue;

The pattern "strspn != strlen" looks kind of remote, or somewhat
pedantic..

+		char	path[MAXPGPATH + 10];
..
-		snprintf(path, MAXPGPATH, "%s/%s", tblspc_path, de->d_name);
+		snprintf(path, sizeof(path), "pg_tblspc/%s", de->d_name);

I don't think we need the extra 10 bytes. A bit paranoic, but we can
check the return value to confirm the d_name is fully stored in the
buffer.

> 0004 is straightforward: let's check for bad directories before logging

I was about to write a comment to do this when looking 0001.

> rewrite continues to be confusing, so I'll have to think more about it.

I'm not sure l3102 exactly points, but haven't we chosen to create
everything required to keep recovery going, whether it is supposed to
exist or not?

> a test case to cover that problematic case?  I think we should strive to

I counldn't recall that clearly and failed to dig out from the thread,
but doesn't the "creating everything needed" strategy naturally save
that case?  We could add that test, but it seems to me a little
cumbersome to confirm the test correctly detect that case..

regards.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 15 Jul 2022 09:56:10 +0200

> something like "unexpected non-(sym)link entry..." work?

Hmm, good point.  I also wonder if we need to cater for using the term
"junction point" rather than "symlink" when under Windows.

> The result of 0003 looks good.

Great, will merge.

> function is not specific for table spaces.

Oh, of course. 

> We already have 
> lstat.

I'll have a look at what we use for lstat failures in other places.

> I found similar codes in two places in backend and one place

I'll have a look at those.  I had the same instinct initially ...

> xlog.c uses the macro XLOGDIR. Why don't we define TBLSPCDIR?

Oh yes, let's do that.  I'd even backpatch that, to avoid a future
backpatching gotcha.

> I don't think we need the extra 10 bytes.

I forgot to mention this, but I just copied these bits from some other
place that processes pg_tblspc entries.  It seemed to me that the
bodiless for loop was a bit too suspicious-looking.

> A bit paranoic, but we can check the return value to confirm the
> d_name is fully stored in the buffer.

Hmm ... I don't think we need to care about that in this patch.  This
coding pattern is already being used in other places.  If we want to
change that, let's do it everywhere, and not in an unrelated
backpatchable bug fix.

> everything required to keep recovery going, whether it is supposed to

I mean just after the two stat() calls for the target directory.

> cumbersome to confirm the test correctly detect that case..

Well, I *hope* it does ... but hope is no strategy, and I've frequently
been on the wrong side when trusting that untested code does what I
think it does.

Thanks for reviewing,

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
                http://smylers.hates-software.com/2005/09/08/1995c749.html

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 15 Jul 2022 12:58:44 +0200

> in frontend. So couldn't it be moved to /common and have a more

I wondered whether it'd be better to check whether get_dirent_type
returns PGFILETYPE_LNK.  However, that doesn't deal with junction points
at all, which seems pretty odd ... I mean, isn't it rather useful as an
abstraction if it doesn't abstract away the one platform-dependent point
we have in the area?

However, looking closer I noticed that on Windows we use our own
readdir() implementation, which AFAICT includes everything to handle
reparse points as symlinks correctly in get_dirent_type.  Which means
that do_pg_start_backup is wasting its time with the "#ifdef WIN32" bits
to handle junction points separately.  We could just do this

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b809a2152c..4966213fde 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8302,13 +8302,8 @@ do_pg_backup_start(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 			 * we sometimes use allow_in_place_tablespaces to create
 			 * directories directly under pg_tblspc, which would fail below.
 			 */
-#ifdef WIN32
-			if (!pgwin32_is_junction(fullpath))
-				continue;
-#else
 			if (get_dirent_type(fullpath, de, false, ERROR) != PGFILETYPE_LNK)
 				continue;
-#endif
 
 #if defined(HAVE_READLINK) || defined(WIN32)
 			rllen = readlink(fullpath, linkpath, sizeof(linkpath));

And everything should continue to work.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 15 Jul 2022 14:03:11 +0200

> And everything should continue to work.

Hmm, but it does not:
https://cirrus-ci.com/build/4824963784900608

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 20 Jul 2022 12:50:49 +0200

v26 here.  I spent some time fighting the readdir() stuff for
Windows (so that get_dirent_type returns LNK for junction points)
but couldn't make it to work and was unable to figure out why.
So I ended up doing what do_pg_backup_start is already doing:
an #ifdef to call pgwin32_is_junction instead.  I remove the
newly added path_is_symlink function, because I realized that
it would mean an extra syscall everywhere other than Windows.

So if somebody wants to fix get_dirent_type() so that it works properly
on Windows, we can change all these places together.

I also change the use of allow_invalid_pages to
allow_in_place_tablespaces.  We could add a
separate GUC for this, but it seems overengineering.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Most hackers will be perfectly comfortable conceptualizing users as entropy
 sources, so let's move on."                               (Nathaniel Smith)

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 20 Jul 2022 17:25:33 +0200

> separate GUC for this, but it seems overengineering.

Oh, but allow_in_place_tablespaces doesn't exist in versions 14 and
older, so this strategy doesn't really work.

I see the following alternatives:

1. not backpatch this fix to 14 and older
2. use a different GUC; either allow_invalid_pages as previously
   suggested, or create a new one just for this purpose
3. not provide any overriding mechanism in versions 14 and older

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Always assume the user will do much worse than the stupidest thing
you can imagine."                                (Julien PUYDT)

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 20 Jul 2022 18:34:23 +0200

> older, so this strategy doesn't really work.

... and get_dirent_type is new in 14, so that'll be one more hurdle.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Cuando no hay humildad las personas se degradan" (A. Christie)

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 21 Jul 2022 13:01:06 +0200

> 3. not provide any overriding mechanism in versions 14 and older

I've got no opinions on this.  I don't like either 1 or 3, so I'm going
to add and backpatch a new GUC allow_recovery_tablespaces as the
override mechanism.

If others disagree with this choice, please speak up.

---

From: Thomas Munro <[email-masked]>
Date: Thu, 21 Jul 2022 23:11:26 +1200

> but couldn't make it to work and was unable to figure out why.

Was it because of this?

https://www.postgresql.org/message-id/CA%2BhUKGKv%2B736Pc8kSj3%3DDijDGd1eC79-uT3Vi16n7jYkcc_raw%40mail.gmail.com

---

From: Thomas Munro <[email-masked]>
Date: Thu, 21 Jul 2022 23:14:57 +1200

> If others disagree with this choice, please speak up.

Would it help if we back-patched the allow_in_place_tablespaces stuff?
 I'm not sure how hard/destabilising that would be, but I could take a
look tomorrow.

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 21 Jul 2022 13:17:51 +0200

> https://www.postgresql.org/message-id/CA%2BhUKGKv%2B736Pc8kSj3%3DDijDGd1eC79-uT3Vi16n7jYkcc_raw%40mail.gmail.com

Oh, that sounds very likely, yeah.  I didn't think of testing the
FILE_ATTRIBUTE_DIRECTORY bit for junction points.

I +1 pushing both of these patches to 14.  Then this patch becomes a
couple of lines shorter.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Before you were born your parents weren't as boring as they are now. They
got that way paying your bills, cleaning up your room and listening to you
tell them how idealistic you are."  -- Charles J. Sykes' advice to teenagers

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 21 Jul 2022 13:20:26 +0200

>  I'm not sure how hard/destabilising that would be, but I could take a

Yeah, I think that would reduce cruft.  I'm not sure this is more
against backpatching policy or less, compared to adding a separate
GUC just for this bugfix.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The problem with the facetime model is not just that it's demoralizing, but
that the people pretending to work interrupt the ones actually working."
                                                           (Paul Graham)

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 21 Jul 2022 13:25:05 +0200

> GUC just for this bugfix.

cruft:

    {
        {"allow_recovery_tablespaces", PG_POSTMASTER, WAL_RECOVERY,
            gettext_noop("Continues recovery after finding invalid database directories."),
            gettext_noop("It is possible for tablespace drop to interfere with database creation "
                         "so that WAL replay is forced to create fake database directories. "
                         "These should have been dropped by the time recovery ends; "
                         "but in case they aren't, this option lets recovery continue if they "
                         "are present.  Note that these directories must be removed manually afterwards."),
            GUC_NOT_IN_SAMPLE
        },
        &allow_recovery_tablespaces,
        false,
        NULL, NULL, NULL
    },

This is not a very good explanation, but I don't know how to make it
better.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I think my standards have lowered enough that now I think 'good design'
is when the page doesn't irritate the living f*ck out of me." (JWZ)

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 22 Jul 2022 09:20:37 +0900 (JST)

At Thu, 21 Jul 2022 23:14:57 +1200, Thomas Munro <[email-masked]> wrote in 
>  I'm not sure how hard/destabilising that would be, but I could take a

+1. Addiotional reason for me is it is a developer option.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 22 Jul 2022 10:02:57 +0900 (JST)

At Thu, 21 Jul 2022 13:25:05 +0200, Alvaro Herrera <[email-masked]> wrote in 
> This is not a very good explanation, but I don't know how to make it

It looks a bit too detailed. I crafted the following..

Recovery can create tentative in-place tablespace directories under
pg_tblspc/. They are assumed to be removed until reaching recovery
consistency, but otherwise PostgreSQL raises a PANIC-level error,
aborting the recovery. Setting allow_recovery_tablespaces to true
causes the system to allow such directories during normal
operation. In case those directories are left after reaching
consistency, that implies data loss and metadata inconsistency and may
cause failure of future tablespace creation.

Though, after writing this, I became to think that piggy-backing on
allow_in_place_tablespaces might be a bit different..

regards.

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 22 Jul 2022 10:18:58 +0200

> +1. Addiotional reason for me is it is a developer option.

OK, I'll wait for allow_in_place_tablespaces to be backpatched then.

I would like to get this fix pushed before the next set of minors, so if
you won't have time for the backpatches early enough, maybe I can work
on getting it done.

Which commits would we consider?

7170f2159fb2	Allow "in place" tablespaces.
f6f0db4d6240    Fix pg_tablespace_location() with in-place tablespaces

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Most hackers will be perfectly comfortable conceptualizing users as entropy
 sources, so let's move on."                               (Nathaniel Smith)

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 22 Jul 2022 17:49:37 +0900 (JST)

At Fri, 22 Jul 2022 10:18:58 +0200, Alvaro Herrera <[email-masked]> wrote in 
> f6f0db4d6240    Fix pg_tablespace_location() with in-place tablespaces

The second one is just to make the function work with in-place
tablespaces. Without it the function yeilds the following error.

> ERROR:  could not read symbolic link "pg_tblspc/16407": Invalid argument

This looks actually odd but I think no need of back-patching because
there's no actual user of the feature is not seen in our test suite.
If we have a test that needs the feature in future, it would be enough
to back-patch it then.

So I think only the first one is needed for now.

regards.

---

From: Thomas Munro <[email-masked]>
Date: Fri, 22 Jul 2022 20:53:12 +1200

> Which commits would we consider?

I wonder how crazy it would be to back-patch
src/test/recovery/t/027_stream_regress.pl too.

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 27 Jul 2022 08:07:16 +0200

> If we have a test that needs the feature in future, it would be enough

Actually, I found that the new test added by the fix in this thread does
depend on this being fixed, so I included an even larger set, which I
think makes this more complete:

7170f2159fb2 Allow "in place" tablespaces.
c6f2f01611d4 Fix pg_basebackup with in-place tablespaces.
f6f0db4d6240 Fix pg_tablespace_location() with in-place tablespaces
7a7cd84893e0 doc: Remove mention to in-place tablespaces for pg_tablespace_location()
5344723755bd Remove unnecessary Windows-specific basebackup code.

I didn't include any of the test changes for now.  I don't intend to do
so, unless we see another reason for that; I think the new tests that
are going to be added by the recovery bugfix should be sufficient
coverage.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La fuerza no está en los medios físicos
sino que reside en una voluntad indomable" (Gandhi)

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 27 Jul 2022 20:54:49 +0200

Okay, I think I'm done with this.  Here's v27 for the master branch,
where I fixed some comments as well as thinkos in the test script.
The ones on older branches aren't materially different, they just have
tonnes of conflicts resolved.  I'll get this pushed tomorrow morning.

I have run it through CI and it seems ... not completely broken, at
least, but I have no working recipes for Windows on branches 14 and
older, so it doesn't really work fully.  If anybody does, please share.
You can see mine here
https://github.com/alvherre/postgres/commits/REL_11_STABLE [etc]
https://cirrus-ci.com/build/5320904228995072
https://cirrus-ci.com/github/alvherre/postgres

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801

---

From: Matthias van de Meent <[email-masked]>
Date: Thu, 28 Jul 2022 20:04:38 +0200

> https://cirrus-ci.com/github/alvherre/postgres

I'd like to bring to your attention that the test that was introduced
with 9e4f914b seem to be flaky in FreeBSD 13 in the CFBot builds: it
sometimes times out while waiting for the secondary to catch up. Or,
at least I think it does, and I'm not too familiar with TAP failure
outputs: it returns with error code 29 and logs that I'd expect when
the timeout is reached.

See bottom for examples (all 3 builds for different patches).

Kind regards,

Matthias van de Meent.

[1] https://cirrus-ci.com/task/4960990331666432?logs=test_world#L2631-L2662
[2] https://cirrus-ci.com/task/5012678384025600?logs=test_world#L2631-L2662
[3] https://cirrus-ci.com/task/5147001137397760?logs=test_world#L2631-L2662

---

From: Tom Lane <[email-masked]>
Date: Thu, 28 Jul 2022 17:57:12 -0400

> outputs: it returns with error code 29 and logs that I'd expect when

It's also failing in the buildfarm, eg

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=conchuela&dt=2022-07-28%2020%3A57%3A50

Looks like only conchuela so far, reinforcing the idea that we're
only seeing it on FreeBSD.  I'd tentatively bet on a timing problem
that requires some FreeBSD scheduling quirk to manifest; we've seen
such quirks before.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Fri, 29 Jul 2022 11:27:01 +1200

> that requires some FreeBSD scheduling quirk to manifest; we've seen

Maybe it just needs a replication slot?  I see:

ERROR:  requested WAL segment 000000010000000000000003 has already been removed

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 29 Jul 2022 14:20:08 +0900 (JST)

At Fri, 29 Jul 2022 11:27:01 +1200, Thomas Munro <[email-masked]> wrote in 
> ERROR:  requested WAL segment 000000010000000000000003 has already been removed

Agreed, I see the same.  The same failure can be surely reproducible
by inserting wal-switch+checkpoint after taking backup [1].  And it is
fixed by the attached.

regards.

[1]:
--- a/src/test/recovery/t/033_replay_tsp_drops.pl
+++ b/src/test/recovery/t/033_replay_tsp_drops.pl
@@ -30,6 +30,13 @@ sub test_tablespace
 	my $backup_name = 'my_backup';
 	$node_primary->backup($backup_name);
 
+	$node_primary->psql(
+		'postgres',
+		qq[
+		CREATE TABLE t(); DROP TABLE t; SELECT pg_switch_wal();
+		CHECKPOINT;
+		]);
+
 	my $node_standby = PostgreSQL::Test::Cluster->new("standby2_$strategy");
 	$node_standby->init_from_backup($node_primary, $backup_name,
 		has_streaming => 1);
diff --git a/src/test/recovery/t/033_replay_tsp_drops.pl b/src/test/recovery/t/033_replay_tsp_drops.pl
index 9b74cb09ac..0756ca6c87 100644
--- a/src/test/recovery/t/033_replay_tsp_drops.pl
+++ b/src/test/recovery/t/033_replay_tsp_drops.pl
@@ -20,6 +20,7 @@ sub test_tablespace
 	$node_primary->psql(
 		'postgres',
 		qq[
+			SELECT pg_create_physical_replication_slot('slot1', true);
 			SET allow_in_place_tablespaces=on;
 			CREATE TABLESPACE dropme_ts1 LOCATION '';
 			CREATE TABLESPACE dropme_ts2 LOCATION '';

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 29 Jul 2022 12:59:03 +0200

> by inserting wal-switch+checkpoint after taking backup [1].  And it is

WFM, pushed that way.  I added a slot drop after the pg_stat_replication
count check to be a little less intrusive. Thanks Matthias for
reporting.  (Note that the Cirrus page has a download link for the
complete logs as artifacts).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I'm always right, but sometimes I'm more right than other times."
                                                  (Linus Torvalds)

---

From: Tom Lane <[email-masked]>
Date: Sat, 30 Jul 2022 10:37:55 -0400

Alvaro Herrera <[email-masked]> writes:
> WFM, pushed that way.

Looks like conchuela is still intermittently unhappy.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=conchuela&dt=2022-07-30%2004%3A57%3A51

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Sat, 30 Jul 2022 12:51:35 -0400

> Looks like conchuela is still intermittently unhappy.

BTW, quite aside from stability, is it really necessary for this test to
be so freakin' slow?  florican for instance reports

[12:43:38] t/025_stuck_on_old_timeline.pl ....... ok    49010 ms ( 0.00 usr  0.00 sys +  3.64 cusr  2.49 csys =  6.13 CPU)
[12:44:12] t/026_overwrite_contrecord.pl ........ ok    34751 ms ( 0.01 usr  0.00 sys +  3.14 cusr  1.76 csys =  4.91 CPU)
[12:49:00] t/027_stream_regress.pl .............. ok   287278 ms ( 0.00 usr  0.00 sys +  9.66 cusr  6.95 csys = 16.60 CPU)
[12:50:04] t/028_pitr_timelines.pl .............. ok    64543 ms ( 0.00 usr  0.00 sys +  3.59 cusr  3.20 csys =  6.78 CPU)
[12:50:17] t/029_stats_restart.pl ............... ok    12505 ms ( 0.02 usr  0.00 sys +  3.16 cusr  1.40 csys =  4.57 CPU)
[12:50:51] t/030_stats_cleanup_replica.pl ....... ok    33933 ms ( 0.01 usr  0.01 sys +  3.55 cusr  2.46 csys =  6.03 CPU)
[12:51:25] t/031_recovery_conflict.pl ........... ok    34249 ms ( 0.00 usr  0.00 sys +  3.37 cusr  2.20 csys =  5.57 CPU)
[12:52:09] t/032_relfilenode_reuse.pl ........... ok    44274 ms ( 0.01 usr  0.00 sys +  3.21 cusr  2.05 csys =  5.27 CPU)
[12:54:07] t/033_replay_tsp_drops.pl ............ ok   117840 ms ( 0.01 usr  0.00 sys +  8.72 cusr  5.41 csys = 14.14 CPU)

027 is so bloated because it runs the core regression tests YA time,
which I'm not very happy about either; but that's no excuse for
every new test to contribute an additional couple of minutes.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Sun, 31 Jul 2022 11:08:01 +1200

> every new test to contribute an additional couple of minutes.

Complaints about 027 noted, I'm thinking about what we could do about that.

As for 033, I worried that it might be the new ProcSignalBarrier stuff
around tablespaces, but thankfully the DEBUG logging I added there
recently shows those all completing in single digit milliseconds.  I
also confirmed there are no unexpected fsync'd being produced here.

That is quite a lot of CPU, but it's a huge amount of total runtime.
It runs in 5-8 seconds on various modern systems, 19 seconds on my
Linux RPi4, and 50 seconds on my Celeron-powered NAS box with spinning
disks.

I noticed this is a 32 bit FBSD system.  Is it running on UFS, perhaps
on slow storage?  Are soft updates enabled (visible as options in
output of "mount")?  Without soft updates, a lot more file system ops
perform synchronous I/O, which really slows down our tests.  In
general, UFS isn't as good as modern file systems at avoiding I/O for
short-lived files, and we set up and tear down a lot of them in our
testing.  Another thing that makes a difference is to use a filesystem
with 8KB block size.  This has been a subject of investigation for
speeding up CI (see src/tools/ci/gcp_freebsd_repartition.sh), but
several mysteries remain unsolved...

---

From: Tom Lane <[email-masked]>
Date: Sat, 30 Jul 2022 19:17:12 -0400

Thomas Munro <[email-masked]> writes:
> output of "mount")?

It's an ancient (2006) mac mini with 5400RPM spinning rust.
"mount" says

/dev/ada0s2a on / (ufs, local, soft-updates, journaled soft-updates)
devfs on /dev (devfs)

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Sun, 31 Jul 2022 15:46:33 +1200

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=conchuela&dt=2022-07-30%2004%3A57%3A51

And here's one from CI that failed on Linux (this was a cfbot run with
an unrelated patch, parent commit b998196 so a few commits after "Fix
test instability"):

https://cirrus-ci.com/task/5282155000496128

https://api.cirrus-ci.com/v1/artifact/task/5282155000496128/log/src/test/recovery/tmp_check/log/033_replay_tsp_drops_primary1_WAL_LOG.log

It looks like this sequence is racy and we need to wait for more than
just "connection is made" before dropping the slot?

    $node_standby->start;

    # Make sure connection is made
    $node_primary->poll_query_until('postgres',
        'SELECT count(*) = 1 FROM pg_stat_replication');
    $node_primary->safe_psql('postgres', "SELECT
pg_drop_replication_slot('slot')");

Why not set the replication slot name so that the standby uses it
"properly", like in other tests?