From ed68fbe78a60cc7abf7354d3b42163e0d2c4430c Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Fri, 3 Jul 2026 11:54:29 +0000 Subject: [PATCH v6 2/4] Re-read subscription state after lock in DropSubscription As done for AlterSubscription() in the preceding XXX commit, re-read the subscription tuple after LockSharedObject() in DropSubscription(). A concurrent DROP or ALTER may have committed while we were waiting for the lock. Without a re-read, DropSubscription would deal with invalid data, which currently produces a confusing "tuple concurrently updated" elog() from CatalogTupleDelete(). If the subscription no longer exists after taking the lock, honor missing_ok, and invoke the DROP hook only after confirming that the subscription still exists. Also recheck ownership before invoking the hook or performing any cleanup, because the owner may have changed while the command was waiting. Extend the isolation test from the preceding commit to cover DROP SUBSCRIPTION. Author: Bertrand Drouvot Reviewed-by: Zhijie Hou Reviewed-by: Amit Kapila Reviewed-by: Shlok Kyal Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- src/backend/commands/subscriptioncmds.c | 58 +++++++++++++++---- .../expected/subscription-owner-locking.out | 27 +++++++++ .../specs/subscription-owner-locking.spec | 34 +++++++---- 3 files changed, 98 insertions(+), 21 deletions(-) 21.7% src/backend/commands/ 29.1% src/test/isolation/expected/ 49.1% src/test/isolation/specs/ diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index a8e174869ff..11ef1822701 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -2659,25 +2659,15 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel) return; } - datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup, - Anum_pg_subscription_subconninfo, &isnull); - if (!isnull) - subconninfo = TextDatumGetCString(datum); - form = (Form_pg_subscription) GETSTRUCT(tup); subid = form->oid; - subowner = form->subowner; - subserver = form->subserver; - subconflictlogrelid = form->subconflictlogrelid; - must_use_password = !superuser_arg(subowner) && form->subpasswordrequired; /* must be owner */ if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, stmt->subname); - /* DROP hook for the subscription being removed */ - InvokeObjectDropHook(SubscriptionRelationId, subid, 0); + ReleaseSysCache(tup); /* * Lock the subscription so nobody else can do anything with it (including @@ -2685,6 +2675,52 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel) */ LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock); + /* + * Re-read the subscription tuple after acquiring the lock. A concurrent + * ALTER or DROP may have committed before we acquired the lock. + */ + tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid)); + + if (!HeapTupleIsValid(tup)) + { + UnlockSharedObject(SubscriptionRelationId, subid, 0, + AccessExclusiveLock); + table_close(rel, NoLock); + + if (!stmt->missing_ok) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("subscription \"%s\" does not exist", + stmt->subname))); + else + ereport(NOTICE, + (errmsg("subscription \"%s\" does not exist, skipping", + stmt->subname))); + + return; + } + + /* must still be owner */ + if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, + stmt->subname); + + /* DROP hook for the subscription being removed */ + InvokeObjectDropHook(SubscriptionRelationId, subid, 0); + + form = (Form_pg_subscription) GETSTRUCT(tup); + subowner = form->subowner; + subserver = form->subserver; + subconflictlogrelid = form->subconflictlogrelid; + must_use_password = !superuser_arg(subowner) && form->subpasswordrequired; + + datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup, + Anum_pg_subscription_subconninfo, &isnull); + if (!isnull) + subconninfo = TextDatumGetCString(datum); + else + subconninfo = NULL; + /* Get subname */ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup, Anum_pg_subscription_subname); diff --git a/src/test/isolation/expected/subscription-owner-locking.out b/src/test/isolation/expected/subscription-owner-locking.out index bd15c17cb89..3b8d0ea4f87 100644 --- a/src/test/isolation/expected/subscription-owner-locking.out +++ b/src/test/isolation/expected/subscription-owner-locking.out @@ -10,3 +10,30 @@ step s1_commit: COMMIT; step s2_alter: <... completed> ERROR: must be owner of subscription regress_sub_owner_lock step s2_reset_role: RESET ROLE; + +starting permutation: s1_begin s1_lock s1_alter_owner s2_set_role s2_drop s1_commit s2_reset_role +step s1_begin: BEGIN; +step s1_lock: COMMENT ON SUBSCRIPTION regress_sub_owner_lock IS 'locked'; +step s1_alter_owner: ALTER SUBSCRIPTION regress_sub_owner_lock OWNER TO regress_sub_owner2; +step s2_set_role: SET ROLE regress_sub_owner1; +step s2_drop: DROP SUBSCRIPTION regress_sub_owner_lock; +step s1_commit: COMMIT; +step s2_drop: <... completed> +ERROR: must be owner of subscription regress_sub_owner_lock +step s2_reset_role: RESET ROLE; + +starting permutation: s1_begin s1_drop s2_drop s1_commit +step s1_begin: BEGIN; +step s1_drop: DROP SUBSCRIPTION regress_sub_owner_lock; +step s2_drop: DROP SUBSCRIPTION regress_sub_owner_lock; +step s1_commit: COMMIT; +step s2_drop: <... completed> +ERROR: subscription "regress_sub_owner_lock" does not exist + +starting permutation: s1_begin s1_drop s2_drop_if_exists s1_commit +step s1_begin: BEGIN; +step s1_drop: DROP SUBSCRIPTION regress_sub_owner_lock; +step s2_drop_if_exists: DROP SUBSCRIPTION IF EXISTS regress_sub_owner_lock; +step s1_commit: COMMIT; +s2: NOTICE: subscription "regress_sub_owner_lock" does not exist, skipping +step s2_drop_if_exists: <... completed> diff --git a/src/test/isolation/specs/subscription-owner-locking.spec b/src/test/isolation/specs/subscription-owner-locking.spec index c2eec7318ea..e19d4b0fbd6 100644 --- a/src/test/isolation/specs/subscription-owner-locking.spec +++ b/src/test/isolation/specs/subscription-owner-locking.spec @@ -1,10 +1,15 @@ -# Test that ALTER SUBSCRIPTION rechecks ownership after waiting for the -# subscription object lock. +# Test post-lock subscription checks in ALTER and DROP SUBSCRIPTION. # -# Session s1 holds the subscription object lock with COMMENT ON SUBSCRIPTION, -# then changes the owner in the same transaction. Session s2 sees the old -# owner and waits for the object lock. Once s1 commits, s2 must recheck the -# subscription state and reject the former owner. +# In the first two permutations, session s1 holds the subscription object lock +# with COMMENT ON SUBSCRIPTION, then changes the owner in the same transaction. +# Session s2 sees the old owner and waits for the object lock. Once s1 commits, +# s2 must recheck the subscription state and reject the former owner. +# +# The last two permutations cover concurrent DROP separately. Session s1 +# deletes the subscription but leaves the transaction open, so session s2 can +# resolve the old name before waiting for the object lock. After s1 commits, +# s2 must process the invalidation, recheck the subscription state, and report +# either ERROR or NOTICE according to whether IF EXISTS was specified. setup { @@ -18,7 +23,7 @@ setup teardown { - DROP SUBSCRIPTION regress_sub_owner_lock; + DROP SUBSCRIPTION IF EXISTS regress_sub_owner_lock; DROP ROLE regress_sub_owner1; DROP ROLE regress_sub_owner2; } @@ -27,11 +32,20 @@ session s1 step s1_begin { BEGIN; } step s1_lock { COMMENT ON SUBSCRIPTION regress_sub_owner_lock IS 'locked'; } step s1_alter_owner { ALTER SUBSCRIPTION regress_sub_owner_lock OWNER TO regress_sub_owner2; } +step s1_drop { DROP SUBSCRIPTION regress_sub_owner_lock; } step s1_commit { COMMIT; } session s2 -step s2_set_role { SET ROLE regress_sub_owner1; } -step s2_alter { ALTER SUBSCRIPTION regress_sub_owner_lock SET (synchronous_commit = local); } -step s2_reset_role { RESET ROLE; } +step s2_set_role { SET ROLE regress_sub_owner1; } +step s2_alter { ALTER SUBSCRIPTION regress_sub_owner_lock SET (synchronous_commit = local); } +step s2_drop { DROP SUBSCRIPTION regress_sub_owner_lock; } +step s2_drop_if_exists { DROP SUBSCRIPTION IF EXISTS regress_sub_owner_lock; } +step s2_reset_role { RESET ROLE; } permutation s1_begin s1_lock s1_alter_owner s2_set_role s2_alter s1_commit s2_reset_role +permutation s1_begin s1_lock s1_alter_owner s2_set_role s2_drop s1_commit s2_reset_role + +# The second DROP must recheck the subscription after waiting and honor +# IF EXISTS if the first DROP removed it. +permutation s1_begin s1_drop s2_drop s1_commit +permutation s1_begin s1_drop s2_drop_if_exists s1_commit -- 2.34.1