From 72a53d1991e7cd9d4a52f48284ddc974a0a4ae65 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Thu, 2 Jul 2026 11:07:04 +0000 Subject: [PATCH v1] 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: - Re-reading the subscription tuple after LockSharedObject() and refreshing the Subscription struct. - Moving the local variable assignments to after the re-read. - Re-checking the password_required privilege restriction after the re-read. Remarks: 1/ not re-checking password_required after the re-read would still produce a "tuple concurrently updated" error, but re-checking it allows us to display a better error message. 2/ the ownership check is intentionally not re-done after the lock because AlterSubscriptionOwner() does not take AccessExclusiveLock on the subscription object: it only takes RowExclusiveLock on the pg_subscription catalog table. This means ownership can change regardless of our lock, making a re-check after lock acquisition pointless. The existing "tuple concurrently updated" error from CatalogTupleUpdate() already provides a protection if ownership changes concurrently. Author: Bertrand Drouvot Reviewed-by: Discussion: --- src/backend/commands/subscriptioncmds.c | 41 ++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) 100.0% src/backend/commands/ diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 4292e7fb8f4..be03b3eb7e1 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1695,11 +1695,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, */ sub = GetSubscription(subid, false, orig_conninfo_needed, false); - 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. @@ -1713,6 +1708,42 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, /* 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. + */ + heap_freetuple(tup); + tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, ObjectIdGetDatum(MyDatabaseId), + CStringGetDatum(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); + + /* Refresh the subscription. */ + pfree(sub); + sub = GetSubscription(subid, false, orig_conninfo_needed, false); + + /* + * Re-check whether a non-superuser is allowed to alter this subscription. + * A concurrent ALTER may have set password_required=false while we were + * waiting for the lock. + */ + if (!sub->passwordrequired && !superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + 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."))); + + 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)); memset(nulls, false, sizeof(nulls)); -- 2.34.1