Fix concurrency issues with DROP TABLESPACE

DROP TABLESPACE checked pg_shdepend for dependent objects without
first locking the tablespace.  A concurrent command that recorded a
shared dependency on the tablespace right after that check could still
commit, leaving an object whose pg_shdepend entry (or, for a relation,
pg_class.reltablespace) pointed to a tablespace that no longer existed.

Close the race by having DropTableSpace() take an AccessExclusiveLock
on the tablespace before calling checkSharedDependencies().  That
conflicts with the AccessShareLock shdepLockAndCheckObject() takes when
recording a new dependency, so the loser of the race blocks and
rechecks once the winner commits.

That AccessExclusiveLock creates a new deadlock: ALTER TABLESPACE
RENAME/SET and the internal ACL/owner updates in DROP OWNED and
REASSIGN OWNED touched the catalog tuple without locking the
tablespace, risking a lock-order cycle with DROP.  Fix by taking an
AccessShareLock first in all four paths, rechecking pg_shdepend after
any wait in the DROP OWNED and REASSIGN OWNED cases.  GRANT, REVOKE,
and ALTER TABLESPACE ... OWNER TO already lock the object.

Add isolation tests covering both orderings of the original race
(dependency-first and drop-first) and all four previously-unlocked
update paths.

Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/CAJTYsWXjAQFnGKzXsht3XK8KHhyhontwFJw4JAHsUSfs_ptR4g@mail.gmail.com
Backpatch-through: 14

5 files changed, 239 insertions, 7 deletions

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index e9b37383acf..14072653ef9 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1160,15 +1160,14 @@ classIdGetDbId(Oid classId)
 /*
  * shdepLockAndCheckObject
  *
- * Lock the object that we are about to record a dependency on.
- * After it's locked, verify that it hasn't been dropped while we
- * weren't looking.  If the object has been dropped, this function
- * does not return!
+ * Acquire an AccessShareLock on a shared object and verify that it still
+ * exists.  This is used when recording a dependency or performing another
+ * operation that must protect the object against a concurrent DROP.
  */
 void
 shdepLockAndCheckObject(Oid classId, Oid objectId)
 {
-	/* AccessShareLock should be OK, since we are not modifying the object */
+	/* AccessShareLock is sufficient to prevent a concurrent DROP. */
 	LockSharedObject(classId, objectId, 0, AccessShareLock);
 
 	switch (classId)
@@ -1417,6 +1416,23 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
 					elog(ERROR, "unexpected dependency type");
 					break;
 				case SHARED_DEPENDENCY_ACL:
+
+					/*
+					 * Lock tablespaces before updating their catalog tuple.
+					 */
+					if (sdepForm->classid == TableSpaceRelationId)
+					{
+						LockSharedObject(sdepForm->classid,
+										 sdepForm->objid, 0,
+										 AccessShareLock);
+						if (!systable_recheck_tuple(scan, tuple))
+						{
+							UnlockSharedObject(sdepForm->classid,
+											   sdepForm->objid, 0,
+											   AccessShareLock);
+							break;
+						}
+					}
 					RemoveRoleFromObjectACL(roleid,
 											sdepForm->classid,
 											sdepForm->objid);
@@ -1653,6 +1669,26 @@ shdepReassignOwned(List *roleids, Oid newrole)
 						Oid			classId = sdepForm->classid;
 						Relation	catalog;
 
+						/*
+						 * Lock tablespaces before updating their catalog
+						 * tuple.
+						 */
+						if (classId == TableSpaceRelationId)
+						{
+							LockSharedObject(classId, sdepForm->objid, 0,
+											 AccessShareLock);
+							if (!systable_recheck_tuple(scan, tuple))
+							{
+								UnlockSharedObject(classId, sdepForm->objid, 0,
+												   AccessShareLock);
+								break;
+							}
+						}
+
+						/*
+						 * For large objects, the catalog to modify is
+						 * pg_largeobject_metadata
+						 */
 						if (classId == LargeObjectRelationId)
 							classId = LargeObjectMetadataRelationId;
 
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 9d895960aaf..4d0ef70693e 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -74,6 +74,8 @@
 #include "postmaster/bgwriter.h"
 #include "storage/fd.h"
 #include "storage/lmgr.h"
+#include "storage/lwlock.h"
+#include "storage/procsignal.h"
 #include "storage/standby.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
@@ -453,6 +455,10 @@ DropTableSpace(DropTableSpaceStmt *stmt)
 		aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLESPACE,
 					   tablespacename);
 
+	/* Prevent new shared dependencies while we drop the tablespace. */
+	LockSharedObject(TableSpaceRelationId, tablespaceoid, 0,
+					 AccessExclusiveLock);
+
 	/* Check for pg_shdepend entries depending on this tablespace */
 	if (checkSharedDependencies(TableSpaceRelationId, tablespaceoid,
 								&detail, &detail_log))
@@ -960,6 +966,9 @@ RenameTableSpace(const char *oldname, const char *newname)
 
 	table_endscan(scan);
 
+	/* Lock the tablespace before updating its catalog tuple. */
+	shdepLockAndCheckObject(TableSpaceRelationId, tspId);
+
 	/* Must be owner */
 	if (!pg_tablespace_ownercheck(tspId, GetUserId()))
 		aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLESPACE, oldname);
@@ -1043,7 +1052,12 @@ AlterTableSpaceOptions(AlterTableSpaceOptionsStmt *stmt)
 				 errmsg("tablespace \"%s\" does not exist",
 						stmt->tablespacename)));
 
+	tup = heap_copytuple(tup);
 	tablespaceoid = ((Form_pg_tablespace) GETSTRUCT(tup))->oid;
+	table_endscan(scandesc);
+
+	/* Lock the tablespace before updating its catalog tuple. */
+	shdepLockAndCheckObject(TableSpaceRelationId, tablespaceoid);
 
 	/* Must be owner of the existing object */
 	if (!pg_tablespace_ownercheck(tablespaceoid, GetUserId()))
@@ -1075,9 +1089,8 @@ AlterTableSpaceOptions(AlterTableSpaceOptionsStmt *stmt)
 	InvokeObjectPostAlterHook(TableSpaceRelationId, tablespaceoid, 0);
 
 	heap_freetuple(newtuple);
+	heap_freetuple(tup);
 
-	/* Conclude heap scan. */
-	table_endscan(scandesc);
 	table_close(rel, NoLock);
 
 	return tablespaceoid;
diff --git a/src/test/isolation/expected/tablespace-dependency-locking.out b/src/test/isolation/expected/tablespace-dependency-locking.out
new file mode 100644
index 00000000000..2a6a0530584
--- /dev/null
+++ b/src/test/isolation/expected/tablespace-dependency-locking.out
@@ -0,0 +1,84 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_begin s1_create_table_in_tablespace s2_drop_tablespace s1_commit s1_drop_table s1_drop_tablespace
+step s1_begin: BEGIN;
+step s1_create_table_in_tablespace: 
+	CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+		TABLESPACE regress_dependency_tablespace;
+
+step s2_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_tablespace: <... completed>
+ERROR:  tablespace "regress_dependency_tablespace" cannot be dropped because some objects depend on it
+step s1_drop_table: DROP TABLE tbl_tablespace;
+step s1_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace;
+
+starting permutation: s1_begin s1_alter_tablespace s2_drop_tablespace s3_create_table_in_dropped_tablespace s1_rollback
+step s1_begin: BEGIN;
+step s1_alter_tablespace: 
+	ALTER TABLESPACE regress_dependency_tablespace
+		SET (random_page_cost = 1.1);
+
+step s2_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace; <waiting ...>
+step s3_create_table_in_dropped_tablespace: 
+	DO $$
+	BEGIN
+		EXECUTE 'CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+			TABLESPACE regress_dependency_tablespace';
+	EXCEPTION WHEN undefined_object THEN
+		RAISE NOTICE 'referenced tablespace was concurrently dropped';
+	END
+	$$;
+ <waiting ...>
+step s1_rollback: ROLLBACK;
+step s2_drop_tablespace: <... completed>
+s3: NOTICE:  referenced tablespace was concurrently dropped
+step s3_create_table_in_dropped_tablespace: <... completed>
+
+starting permutation: s1_begin s1_alter_tablespace s2_drop_tablespace s1_create_table_in_tablespace s1_commit
+step s1_begin: BEGIN;
+step s1_alter_tablespace: 
+	ALTER TABLESPACE regress_dependency_tablespace
+		SET (random_page_cost = 1.1);
+
+step s2_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace; <waiting ...>
+step s1_create_table_in_tablespace: 
+	CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+		TABLESPACE regress_dependency_tablespace;
+
+step s1_commit: COMMIT;
+step s2_drop_tablespace: <... completed>
+ERROR:  tablespace "regress_dependency_tablespace" cannot be dropped because some objects depend on it
+
+starting permutation: s1_begin s1_rename_tablespace s2_drop_tablespace s1_create_table_in_renamed_tablespace s1_commit
+step s1_begin: BEGIN;
+step s1_rename_tablespace: ALTER TABLESPACE regress_dependency_tablespace RENAME TO regress_dependency_tablespace_renamed;
+step s2_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace; <waiting ...>
+step s1_create_table_in_renamed_tablespace: CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a) TABLESPACE regress_dependency_tablespace_renamed;
+step s1_commit: COMMIT;
+step s2_drop_tablespace: <... completed>
+ERROR:  tablespace "regress_dependency_tablespace" cannot be dropped because some objects depend on it
+
+starting permutation: s1_begin s1_reassign_owned s2_drop_tablespace s1_create_table_in_tablespace s1_commit
+step s1_begin: BEGIN;
+step s1_reassign_owned: REASSIGN OWNED BY regress_ts_owner TO CURRENT_USER;
+step s2_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace; <waiting ...>
+step s1_create_table_in_tablespace: 
+	CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+		TABLESPACE regress_dependency_tablespace;
+
+step s1_commit: COMMIT;
+step s2_drop_tablespace: <... completed>
+ERROR:  tablespace "regress_dependency_tablespace" cannot be dropped because some objects depend on it
+
+starting permutation: s1_begin s1_drop_owned s2_drop_tablespace s1_create_table_in_tablespace s1_commit
+step s1_begin: BEGIN;
+step s1_drop_owned: DROP OWNED BY regress_ts_grantee;
+step s2_drop_tablespace: DROP TABLESPACE regress_dependency_tablespace; <waiting ...>
+step s1_create_table_in_tablespace: 
+	CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+		TABLESPACE regress_dependency_tablespace;
+
+step s1_commit: COMMIT;
+step s2_drop_tablespace: <... completed>
+ERROR:  tablespace "regress_dependency_tablespace" cannot be dropped because some objects depend on it
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 6ad0e33f3f5..bc108f3296a 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -105,3 +105,4 @@ test: serializable-parallel
 test: serializable-parallel-2
 test: serializable-parallel-3
 test: ddl-dependency-locking
+test: tablespace-dependency-locking
diff --git a/src/test/isolation/specs/tablespace-dependency-locking.spec b/src/test/isolation/specs/tablespace-dependency-locking.spec
new file mode 100644
index 00000000000..23de346c5f7
--- /dev/null
+++ b/src/test/isolation/specs/tablespace-dependency-locking.spec
@@ -0,0 +1,98 @@
+# Test that concurrent DROP TABLESPACE and CREATE TABLE do not leave behind
+# references to a non-existent tablespace.
+
+setup
+{
+	SET allow_in_place_tablespaces = true;
+	CREATE ROLE regress_ts_owner;
+	CREATE ROLE regress_ts_grantee;
+}
+
+setup
+{
+	CREATE TABLESPACE regress_dependency_tablespace
+		OWNER regress_ts_owner LOCATION '';
+}
+
+setup
+{
+	GRANT CREATE ON TABLESPACE regress_dependency_tablespace
+		TO regress_ts_grantee;
+}
+
+teardown
+{
+	DROP TABLESPACE IF EXISTS regress_dependency_tablespace;
+}
+
+session "s1"
+
+step "s1_begin" { BEGIN; }
+step "s1_create_table_in_tablespace"
+{
+	CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+		TABLESPACE regress_dependency_tablespace;
+}
+step "s1_alter_tablespace"
+{
+	ALTER TABLESPACE regress_dependency_tablespace
+		SET (random_page_cost = 1.1);
+}
+step "s1_rename_tablespace" { ALTER TABLESPACE regress_dependency_tablespace RENAME TO regress_dependency_tablespace_renamed; }
+step "s1_reassign_owned" { REASSIGN OWNED BY regress_ts_owner TO CURRENT_USER; }
+step "s1_drop_owned" { DROP OWNED BY regress_ts_grantee; }
+step "s1_create_table_in_renamed_tablespace" { CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a) TABLESPACE regress_dependency_tablespace_renamed; }
+step "s1_commit" { COMMIT; }
+step "s1_rollback" { ROLLBACK; }
+step "s1_drop_table" { DROP TABLE tbl_tablespace; }
+step "s1_drop_tablespace" { DROP TABLESPACE regress_dependency_tablespace; }
+
+teardown
+{
+	SET client_min_messages = warning;
+	DROP TABLE IF EXISTS tbl_tablespace;
+	DROP OWNED BY regress_ts_grantee;
+	REASSIGN OWNED BY regress_ts_owner TO CURRENT_USER;
+	DO $$
+	BEGIN
+		IF EXISTS (SELECT FROM pg_tablespace
+				   WHERE spcname = 'regress_dependency_tablespace_renamed') THEN
+			ALTER TABLESPACE regress_dependency_tablespace_renamed
+				RENAME TO regress_dependency_tablespace;
+		END IF;
+	END
+	$$;
+	DROP ROLE IF EXISTS regress_ts_owner;
+	DROP ROLE IF EXISTS regress_ts_grantee;
+}
+
+session "s2"
+
+step "s2_drop_tablespace" { DROP TABLESPACE regress_dependency_tablespace; }
+
+session "s3"
+
+step "s3_create_table_in_dropped_tablespace"
+{
+	DO $$
+	BEGIN
+		EXECUTE 'CREATE TABLE tbl_tablespace (a int) PARTITION BY RANGE (a)
+			TABLESPACE regress_dependency_tablespace';
+	EXCEPTION WHEN undefined_object THEN
+		RAISE NOTICE 'referenced tablespace was concurrently dropped';
+	END
+	$$;
+}
+
+# create table - drop tablespace
+permutation "s1_begin" "s1_create_table_in_tablespace" "s2_drop_tablespace" "s1_commit" "s1_drop_table" "s1_drop_tablespace"
+
+# drop tablespace - create table; ALTER makes DROP wait while deleting the
+# catalog tuple, after DROP has checked for dependencies
+permutation "s1_begin" "s1_alter_tablespace" "s2_drop_tablespace" "s3_create_table_in_dropped_tablespace" "s1_rollback"
+
+# pg_tablespace updates must lock the tablespace before the catalog tuple
+permutation "s1_begin" "s1_alter_tablespace" "s2_drop_tablespace" "s1_create_table_in_tablespace" "s1_commit"
+permutation "s1_begin" "s1_rename_tablespace" "s2_drop_tablespace" "s1_create_table_in_renamed_tablespace" "s1_commit"
+permutation "s1_begin" "s1_reassign_owned" "s2_drop_tablespace" "s1_create_table_in_tablespace" "s1_commit"
+permutation "s1_begin" "s1_drop_owned" "s2_drop_tablespace" "s1_create_table_in_tablespace" "s1_commit"