From 13863e1505ba0696d1f3eaa51e82cc516d3b447d Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Tue, 8 Sep 2026 13:06:00 +0000 Subject: [PATCH v2] Prevent orphaned tablespace dependencies DROP TABLESPACE checked pg_shdepend without first locking the tablespace object. A concurrent command could therefore add a shared dependency after that check, allowing both commands to commit and leaving an object that referenced a deleted tablespace. Take an AccessExclusiveLock on the tablespace before checking its shared dependencies. This conflicts with the AccessShareLock taken when recording a dependency, so either DROP observes the dependency or the dependency creator rechecks the tablespace after waiting. The new DROP-side lock also requires a consistent lock order for paths that update pg_tablespace. Otherwise, a transaction can hold the catalog tuple while DROP holds the object lock, then deadlock when it subsequently records a tablespace dependency. Acquire an AccessShareLock before changing a tablespace's name or options and before the internal ACL and owner updates performed by DROP OWNED and REASSIGN OWNED. Recheck the pg_shdepend tuple after any lock wait. Direct GRANT, REVOKE, and ALTER OWNER paths already acquire an object lock. Add isolation coverage for both orderings of the original race and for all four previously unlocked catalog-update paths. --- src/backend/catalog/pg_shdepend.c | 37 ++++++- src/backend/commands/tablespace.c | 16 ++- .../tablespace-dependency-locking.out | 84 ++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/tablespace-dependency-locking.spec | 98 +++++++++++++++++++ 5 files changed, 229 insertions(+), 7 deletions(-) create mode 100644 src/test/isolation/expected/tablespace-dependency-locking.out create mode 100644 src/test/isolation/specs/tablespace-dependency-locking.spec diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c index f70bffd527f..96ef6abf9dd 100644 --- a/src/backend/catalog/pg_shdepend.c +++ b/src/backend/catalog/pg_shdepend.c @@ -1199,15 +1199,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) @@ -1450,6 +1449,20 @@ shdepDropOwned(List *roleids, DropBehavior behavior) */ if (sdepForm->classid != AuthMemRelationId) { + /* 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); @@ -1605,6 +1618,20 @@ shdepReassignOwned(List *roleids, Oid newrole) switch (sdepForm->deptype) { case SHARED_DEPENDENCY_OWNER: + /* 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; + } + } shdepReassignOwned_Owner(sdepForm, newrole); break; case SHARED_DEPENDENCY_INITACL: diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index e3c4a7fac87..e01fb2db913 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -70,6 +70,7 @@ #include "miscadmin.h" #include "postmaster/bgwriter.h" #include "storage/fd.h" +#include "storage/lmgr.h" #include "storage/lwlock.h" #include "storage/procsignal.h" #include "storage/standby.h" @@ -457,6 +458,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)) @@ -967,6 +972,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 (!object_ownercheck(TableSpaceRelationId, tspId, GetUserId())) aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLESPACE, oldname); @@ -1056,7 +1064,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 (!object_ownercheck(TableSpaceRelationId, tablespaceoid, GetUserId())) @@ -1088,9 +1101,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; +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; +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 + $$; + +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; +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; +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; +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; +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 1fcf4e63238..fc45d504d2b 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -128,5 +128,6 @@ test: matview-write-skew test: lock-nowait test: for-portion-of test: ddl-dependency-locking +test: tablespace-dependency-locking test: pub-concurrent-drop test: drop-owned-grant 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" -- 2.43.0