Hi, On Thu, Aug 06, 2026 at 03:50:09PM +0530, Shlok Kyal wrote: > On Tue, 4 Aug 2026 at 11:25, Bertrand Drouvot > <[email protected]> wrote: > > > > Hi, > > > > On Mon, Jul 06, 2026 at 02:53:39PM +0000, Bertrand Drouvot wrote: > > > Hi, > > > > > > On Mon, Jul 06, 2026 at 03:07:24PM +0530, Amit Kapila wrote: > > > > > > DROP SUBSCRIPTION however has its own dedicated code path and does not go > > > through > > > get_object_address(): 0003 adds the retry loop for it. And if DROP > > > already uses > > > the retry loop then ALTER should probably use it too (also done in 0003 > > > and 0004). > > > > Mandatory rebase attached. > > > Hi, I reviewed 0001 and 0002 patches.
Thanks! > 0001 LGTM. > > Some comments for 0002 patch: > > 1. Here if 'tup' is invalid we are not checking the 'stmt->missing_ok' flag. > Should we only throw an error if 'stmt->missing' is false? > Otherwise 'DROP SUBSCRIPTION IF EXISTS' can throw an error like: > > postgres=# DROP SUBSCRIPTION IF EXISTS sub1; > ERROR: subscription "sub1" does not exist > > 2. Should the function 'InvokeObjectDropHook' be called after the > check in 'if (!HeapTupleIsValid(tup))'? > If 'tup' is not valid, an error is thrown, and in this case, calling > the function 'InvokeObjectDropHook' is unnecessary. Yeah, both comments would be addressed by 0003, but 0002 should be correct on its own, so fixed in the attached. Also, while at it, I changed my mind about the post-lock owner recheck. I think reporting "must be owner of subscription" is better than "tuple concurrently updated", so 0001 and 0002 now perform this recheck and add some basic isolation tests for it. Those explicit rechecks become redundant once 0003 is applied (and 0004 does the same for publications), but they make 0001 and 0002 behave better on their own. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From 8090a59fecb2ae79a97a790f4eefaa9a9c4125a7 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> 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 <[email protected]> Reviewed-by: Dilip Kumar <[email protected]> Reviewed-by: Hayato Kuroda (Fujitsu) <[email protected]> Reviewed-by: Zhijie Hou <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Shlok Kyal <[email protected]> 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); <waiting ...> +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
>From ed68fbe78a60cc7abf7354d3b42163e0d2c4430c Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> 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 <[email protected]> Reviewed-by: Zhijie Hou <[email protected]> Reviewed-by: Amit Kapila <[email protected]> Reviewed-by: Shlok Kyal <[email protected]> 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; <waiting ...> +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; <waiting ...> +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; <waiting ...> +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
>From 5e62922b2bec13734222a90d9516d0ea1bea98cf Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Mon, 6 Jul 2026 04:44:13 +0000 Subject: [PATCH v6 3/4] Add invalidation-based retry loop for Alter/Drop Subscription Following the approach of RangeVarGetRelidExtended() for relations, add a retry loop that includes name resolution, ownership check, and lock acquisition in AlterSubscription() and DropSubscription(). The loop records SharedInvalidMessageCounter, resolves the subscription name to an OID, checks ownership, then locks the subscription. If the invalidation counter changed (indicating concurrent DDL), we save the current OID and retry. On the next iteration, if the name still resolves to the same OID, we're done (already holding the correct lock). If it resolves to a different OID, we release the old lock and acquire the new one. This mirrors RangeVarGetRelidExtended()'s behavior: the lock is kept across retries to avoid a window where another session could have committed concurrent DDL modifying the ownership and/or the name resolution. Author: Bertrand Drouvot <[email protected]> Reviewed-by: Dilip Kumar <[email protected]> Reviewed-by: Hayato Kuroda (Fujitsu) <[email protected]> Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- src/backend/commands/subscriptioncmds.c | 202 +++++++++++++++--------- 1 file changed, 129 insertions(+), 73 deletions(-) 100.0% src/backend/commands/ diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 11ef1822701..84bf7d873b6 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -51,6 +51,7 @@ #include "replication/worker_internal.h" #include "storage/lmgr.h" #include "storage/lock.h" +#include "storage/sinval.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/guc.h" @@ -1666,23 +1667,67 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, rel = table_open(SubscriptionRelationId, RowExclusiveLock); - /* Fetch the existing tuple. */ - tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, ObjectIdGetDatum(MyDatabaseId), - CStringGetDatum(stmt->subname)); + /* + * Lock the subscription so nobody else can do anything with it. + * + * Like RangeVarGetRelidExtended() does for relations, we resolve the + * name, check ownership, and lock inside a loop. If invalidation messages + * arrive (indicating concurrent DDL), we retry. We keep the lock held + * across retries and only release it if the name resolves to a different + * OID on the next iteration. + */ + { + Oid oldSubId = InvalidOid; + bool retry = false; - if (!HeapTupleIsValid(tup)) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("subscription \"%s\" does not exist", - stmt->subname))); + for (;;) + { + uint64 inval_count = SharedInvalidMessageCounter; - form = (Form_pg_subscription) GETSTRUCT(tup); - subid = form->oid; + tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, + ObjectIdGetDatum(MyDatabaseId), + CStringGetDatum(stmt->subname)); - /* must be owner */ - if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, - stmt->subname); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("subscription \"%s\" does not exist", + stmt->subname))); + + form = (Form_pg_subscription) GETSTRUCT(tup); + subid = form->oid; + + if (!object_ownercheck(SubscriptionRelationId, subid, + GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, + stmt->subname); + + /* + * If upon retry we get the same OID, the invalidation messages + * did not change the final answer. So we're done. If we got a + * different OID, unlock the old one and lock the new one below. + */ + if (retry) + { + if (subid == oldSubId) + break; + UnlockSharedObject(SubscriptionRelationId, oldSubId, 0, + AccessExclusiveLock); + } + + LockSharedObject(SubscriptionRelationId, subid, 0, + AccessExclusiveLock); + + /* If no invalidation messages, we're done. */ + if (inval_count == SharedInvalidMessageCounter) + break; + + /* Something may have changed, retry. */ + retry = true; + oldSubId = subid; + heap_freetuple(tup); + } + } /* parse and check options */ switch (stmt->kind) @@ -1730,30 +1775,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, if (supported_opts > 0) parse_subscription_options(pstate, stmt->options, supported_opts, &opts); - 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; @@ -2659,56 +2680,91 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel) return; } - form = (Form_pg_subscription) GETSTRUCT(tup); - subid = form->oid; - - /* must be owner */ - if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, - stmt->subname); - ReleaseSysCache(tup); /* * Lock the subscription so nobody else can do anything with it (including * the replication workers). + * + * Like RangeVarGetRelidExtended() does for relations, we resolve the + * name, check ownership, and lock inside a loop. If invalidation messages + * arrive (indicating concurrent DDL), we retry. We keep the lock held + * across retries and only release it if the name resolves to a different + * OID on the next iteration. */ - LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock); + { + Oid oldSubId = InvalidOid; + bool retry = false; - /* - * 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)); + for (;;) + { + uint64 inval_count = SharedInvalidMessageCounter; - if (!HeapTupleIsValid(tup)) - { - UnlockSharedObject(SubscriptionRelationId, subid, 0, - AccessExclusiveLock); - table_close(rel, NoLock); + tup = SearchSysCache2(SUBSCRIPTIONNAME, + ObjectIdGetDatum(MyDatabaseId), + CStringGetDatum(stmt->subname)); - 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))); + if (!HeapTupleIsValid(tup)) + { + if (retry) + UnlockSharedObject(SubscriptionRelationId, oldSubId, 0, + AccessExclusiveLock); + table_close(rel, NoLock); - return; - } + 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))); - /* must still be owner */ - if (!object_ownercheck(SubscriptionRelationId, subid, GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, - stmt->subname); + return; + } + + form = (Form_pg_subscription) GETSTRUCT(tup); + subid = form->oid; + + if (!object_ownercheck(SubscriptionRelationId, subid, + GetUserId())) + { + ReleaseSysCache(tup); + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION, + stmt->subname); + } + + /* + * If upon retry we get the same OID, the invalidation messages + * did not change the final answer. So we're done. If we got a + * different OID, unlock the old one and lock the new one below. + */ + if (retry) + { + if (subid == oldSubId) + break; + UnlockSharedObject(SubscriptionRelationId, oldSubId, 0, + AccessExclusiveLock); + } + + LockSharedObject(SubscriptionRelationId, subid, 0, + AccessExclusiveLock); + + /* If no invalidation messages, we're done. */ + if (inval_count == SharedInvalidMessageCounter) + break; + + /* Something may have changed, retry. */ + retry = true; + oldSubId = subid; + ReleaseSysCache(tup); + } + } /* 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; -- 2.34.1
>From de523a1f5304bec7418eaf2d5c6c2671cf513606 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Fri, 3 Jul 2026 14:46:42 +0000 Subject: [PATCH v6 4/4] Add invalidation-based retry loop for AlterPublication Apply the same RangeVarGetRelidExtended() style retry loop to AlterPublication()'s tables/schemas branch that was added for subscriptions in the preceding XXX commit. Previously, this branch resolved the publication name and checked ownership at the top of AlterPublication(), then locked and re-read by OID. This left a window where concurrent DDL could have modified the ownership and/or the name resolution. Now the tables/schemas branch has its own complete retry loop: name resolution, ownership check, and lock acquisition all inside the loop. Add an isolation test that changes the publication owner while an ALTER PUBLICATION command is waiting for the publication object lock, and verifies that the former owner is rejected when the command resumes. Author: Bertrand Drouvot <[email protected]> Reviewed-by: Dilip Kumar <[email protected]> Reviewed-by: Hayato Kuroda (Fujitsu) <[email protected]> Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- src/backend/commands/publicationcmds.c | 105 ++++++++++++------ .../expected/publication-owner-locking.out | 12 ++ src/test/isolation/isolation_schedule | 1 + .../specs/publication-owner-locking.spec | 37 ++++++ 4 files changed, 122 insertions(+), 33 deletions(-) 60.5% src/backend/commands/ 12.3% src/test/isolation/expected/ 26.5% src/test/isolation/specs/ diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 440adb356ad..dfd707bc7d7 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -39,6 +39,7 @@ #include "parser/parse_relation.h" #include "rewrite/rewriteHandler.h" #include "storage/lmgr.h" +#include "storage/sinval.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/inval.h" @@ -1662,54 +1663,92 @@ AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt) rel = table_open(PublicationRelationId, RowExclusiveLock); - tup = SearchSysCacheCopy1(PUBLICATIONNAME, - CStringGetDatum(stmt->pubname)); + if (stmt->options) + { + tup = SearchSysCacheCopy1(PUBLICATIONNAME, + CStringGetDatum(stmt->pubname)); - if (!HeapTupleIsValid(tup)) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("publication \"%s\" does not exist", - stmt->pubname))); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("publication \"%s\" does not exist", + stmt->pubname))); - pubform = (Form_pg_publication) GETSTRUCT(tup); + pubform = (Form_pg_publication) GETSTRUCT(tup); - /* must be owner */ - if (!object_ownercheck(PublicationRelationId, pubform->oid, GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION, - stmt->pubname); + /* must be owner */ + if (!object_ownercheck(PublicationRelationId, pubform->oid, + GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION, + stmt->pubname); - if (stmt->options) AlterPublicationOptions(pstate, stmt, rel, tup); + } else { List *relations = NIL; List *exceptrelations = NIL; List *schemaidlist = NIL; - Oid pubid = pubform->oid; + Oid pubid; - ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations, - &exceptrelations, &schemaidlist); + /* + * Lock the publication so nobody else can do anything with it. + * + * Like RangeVarGetRelidExtended() does for relations, we resolve the + * name, check ownership, and lock inside a loop. If invalidation + * messages arrive (indicating concurrent DDL), we retry. We keep the + * lock held across retries and only release it if the name resolves + * to a different OID on the next iteration. + */ + { + Oid oldPubId = InvalidOid; + bool retry = false; - CheckAlterPublication(stmt, tup, relations, schemaidlist); + for (;;) + { + uint64 inval_count = SharedInvalidMessageCounter; - heap_freetuple(tup); + tup = SearchSysCacheCopy1(PUBLICATIONNAME, + CStringGetDatum(stmt->pubname)); - /* Lock the publication so nobody else can do anything with it. */ - LockDatabaseObject(PublicationRelationId, pubid, 0, - AccessExclusiveLock); + if (!HeapTupleIsValid(tup)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("publication \"%s\" does not exist", + stmt->pubname))); - /* - * It is possible that by the time we acquire the lock on publication, - * concurrent DDL has removed it. We can test this by checking the - * existence of publication. We get the tuple again to avoid the risk - * of any publication option getting changed. - */ - tup = SearchSysCacheCopy1(PUBLICATIONOID, ObjectIdGetDatum(pubid)); - if (!HeapTupleIsValid(tup)) - ereport(ERROR, - errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("publication \"%s\" does not exist", - stmt->pubname)); + pubform = (Form_pg_publication) GETSTRUCT(tup); + pubid = pubform->oid; + + if (!object_ownercheck(PublicationRelationId, pubid, + GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION, + stmt->pubname); + + if (retry) + { + if (pubid == oldPubId) + break; + UnlockDatabaseObject(PublicationRelationId, oldPubId, 0, + AccessExclusiveLock); + } + + LockDatabaseObject(PublicationRelationId, pubid, 0, + AccessExclusiveLock); + + if (inval_count == SharedInvalidMessageCounter) + break; + + retry = true; + oldPubId = pubid; + heap_freetuple(tup); + } + } + + ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations, + &exceptrelations, &schemaidlist); + + CheckAlterPublication(stmt, tup, relations, schemaidlist); relations = list_concat(relations, exceptrelations); AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext, diff --git a/src/test/isolation/expected/publication-owner-locking.out b/src/test/isolation/expected/publication-owner-locking.out new file mode 100644 index 00000000000..06d1d70d5a9 --- /dev/null +++ b/src/test/isolation/expected/publication-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 PUBLICATION regress_pub_owner_lock IS 'locked'; +step s1_alter_owner: ALTER PUBLICATION regress_pub_owner_lock OWNER TO regress_pub_owner2; +step s2_set_role: SET ROLE regress_pub_owner1; +step s2_alter: ALTER PUBLICATION regress_pub_owner_lock ADD TABLE regress_pub_owner_lock_table; <waiting ...> +step s1_commit: COMMIT; +step s2_alter: <... completed> +ERROR: must be owner of publication regress_pub_owner_lock +step s2_reset_role: RESET ROLE; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index eb1b257e56e..fb60d106f7f 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -129,5 +129,6 @@ test: lock-nowait test: for-portion-of test: ddl-dependency-locking test: subscription-owner-locking +test: publication-owner-locking test: pub-concurrent-drop test: drop-owned-grant diff --git a/src/test/isolation/specs/publication-owner-locking.spec b/src/test/isolation/specs/publication-owner-locking.spec new file mode 100644 index 00000000000..8ef0e8c31b7 --- /dev/null +++ b/src/test/isolation/specs/publication-owner-locking.spec @@ -0,0 +1,37 @@ +# Test post-lock publication ownership checks in ALTER PUBLICATION. +# +# Session s1 holds the publication object lock with COMMENT ON PUBLICATION, +# 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 +# publication state and reject the former owner. + +setup +{ + CREATE ROLE regress_pub_owner1; + CREATE ROLE regress_pub_owner2; + CREATE TABLE regress_pub_owner_lock_table (a int); + ALTER TABLE regress_pub_owner_lock_table OWNER TO regress_pub_owner1; + CREATE PUBLICATION regress_pub_owner_lock; + ALTER PUBLICATION regress_pub_owner_lock OWNER TO regress_pub_owner1; +} + +teardown +{ + DROP PUBLICATION regress_pub_owner_lock; + DROP TABLE regress_pub_owner_lock_table; + DROP ROLE regress_pub_owner1; + DROP ROLE regress_pub_owner2; +} + +session s1 +step s1_begin { BEGIN; } +step s1_lock { COMMENT ON PUBLICATION regress_pub_owner_lock IS 'locked'; } +step s1_alter_owner { ALTER PUBLICATION regress_pub_owner_lock OWNER TO regress_pub_owner2; } +step s1_commit { COMMIT; } + +session s2 +step s2_set_role { SET ROLE regress_pub_owner1; } +step s2_alter { ALTER PUBLICATION regress_pub_owner_lock ADD TABLE regress_pub_owner_lock_table; } +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
