From 8090a59fecb2ae79a97a790f4eefaa9a9c4125a7 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Fri, 3 Jul 2026 12:28:42 +0000 Subject: [PATCH v6 1/4] Re-read subscription state after lock in AlterSubscription AlterSubscription() reads the subscription's catalog state via GetSubscription() before acquiring AccessExclusiveLock on the subscription object. A concurrent session that commits a DROP or ALTER between the read and the lock acquisition leaves the other session acting with stale information once it unblocks. Fix by moving the GetSubscription() call, the password_required privilege check, and the local variable assignments to after LockSharedObject(), with a re-read of the subscription tuple to ensure we operate on current catalog state. Recheck ownership using the post-lock state as well. Since owner changes do not yet acquire the subscription object lock, the owner may have changed while the ALTER SUBSCRIPTION command was waiting. Add an isolation test that holds the subscription object lock without updating pg_subscription, changes the owner, and verifies that the former owner is rejected when ALTER SUBSCRIPTION resumes. Author: Bertrand Drouvot Reviewed-by: Dilip Kumar Reviewed-by: Hayato Kuroda (Fujitsu) Reviewed-by: Zhijie Hou Reviewed-by: Amit Kapila Reviewed-by: Shlok Kyal Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- src/backend/commands/subscriptioncmds.c | 36 ++++++++++++++---- .../expected/subscription-owner-locking.out | 12 ++++++ src/test/isolation/isolation_schedule | 1 + .../specs/subscription-owner-locking.spec | 37 +++++++++++++++++++ 4 files changed, 78 insertions(+), 8 deletions(-) 24.4% src/backend/commands/ 24.0% src/test/isolation/expected/ 50.2% src/test/isolation/specs/ diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 8e8db08bd93..a8e174869ff 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1730,13 +1730,36 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, if (supported_opts > 0) parse_subscription_options(pstate, stmt->options, supported_opts, &opts); - sub = GetSubscription(subid, false); + heap_freetuple(tup); + + /* Lock the subscription so nobody else can do anything with it. */ + LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock); + + /* + * Re-read the subscription tuple after acquiring the lock. A concurrent + * DROP or ALTER may have committed before we acquired the lock. + */ + tup = SearchSysCacheCopy1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid)); + + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("subscription \"%s\" does not exist", + stmt->subname))); + + form = (Form_pg_subscription) GETSTRUCT(tup); + + /* must still be owner */ + if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, + stmt->subname); /* * Determine in advance whether we need the original conninfo or not, so * that errors are generated consistently in cases where we do need it; * and not generated at all if we don't. */ + sub = GetSubscription(subid, false); /* conninfo needed when refreshing */ switch (stmt->kind) @@ -1788,11 +1811,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, if (orig_conninfo_needed) orig_conninfo = SubscriptionConninfo(sub); - retain_dead_tuples = sub->retaindeadtuples; - origin = sub->origin; - max_retention = sub->maxretention; - retention_active = sub->retentionactive; - /* * Don't allow non-superuser modification of a subscription with * password_required=false. @@ -1803,8 +1821,10 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, errmsg("password_required=false is superuser-only"), errhint("Subscriptions with the password_required option set to false may only be created or modified by the superuser."))); - /* Lock the subscription so nobody else can do anything with it. */ - LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock); + retain_dead_tuples = sub->retaindeadtuples; + origin = sub->origin; + max_retention = sub->maxretention; + retention_active = sub->retentionactive; /* Form a new tuple. */ memset(values, 0, sizeof(values)); diff --git a/src/test/isolation/expected/subscription-owner-locking.out b/src/test/isolation/expected/subscription-owner-locking.out new file mode 100644 index 00000000000..bd15c17cb89 --- /dev/null +++ b/src/test/isolation/expected/subscription-owner-locking.out @@ -0,0 +1,12 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_begin s1_lock s1_alter_owner s2_set_role s2_alter 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_alter: ALTER SUBSCRIPTION regress_sub_owner_lock SET (synchronous_commit = local); +step s1_commit: COMMIT; +step s2_alter: <... completed> +ERROR: must be owner of subscription regress_sub_owner_lock +step s2_reset_role: RESET ROLE; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index df8ce44ede6..eb1b257e56e 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: subscription-owner-locking test: pub-concurrent-drop test: drop-owned-grant diff --git a/src/test/isolation/specs/subscription-owner-locking.spec b/src/test/isolation/specs/subscription-owner-locking.spec new file mode 100644 index 00000000000..c2eec7318ea --- /dev/null +++ b/src/test/isolation/specs/subscription-owner-locking.spec @@ -0,0 +1,37 @@ +# Test that ALTER SUBSCRIPTION rechecks ownership after waiting for the +# subscription object lock. +# +# 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. + +setup +{ + CREATE ROLE regress_sub_owner1; + CREATE ROLE regress_sub_owner2; + CREATE SUBSCRIPTION regress_sub_owner_lock + CONNECTION '' PUBLICATION regress_pub + WITH (connect = false, slot_name = NONE); + ALTER SUBSCRIPTION regress_sub_owner_lock OWNER TO regress_sub_owner1; +} + +teardown +{ + DROP SUBSCRIPTION regress_sub_owner_lock; + DROP ROLE regress_sub_owner1; + DROP ROLE regress_sub_owner2; +} + +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_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; } + +permutation s1_begin s1_lock s1_alter_owner s2_set_role s2_alter s1_commit s2_reset_role -- 2.34.1