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