Hi hackers, while playing with the new ALTER SUBSCRIPTION parameter added in a5918fddf10, I realized that the subscription is not re-read once we acquire the lock in AlterSubscription().
This pre-existing issue is now more visible after a5918fddf10: 1/ two concurrent ALTER SUBSCRIPTION SET (conflict_log_destination = 'table') could result in the second session attempting to create an already-existing conflict log table, producing a confusing "relation already exists" error: ERROR: relation "pg_conflict_log_24614" already exists It's confusing because ALTER SUBSCRIPTION SET (conflict_log_destination = 'table') would not report an error if the conflict table already exists (and no concurrent ALTER is running). 2/ a concurrent DROP followed by the ALTER would emit a NOTICE about creating the conflict log table before failing with "referenced subscription was concurrently dropped". That sounds like a weird messaging: NOTICE: created conflict log table "pg_conflict.pg_conflict_log_24620" for subscription "mysub" ERROR: referenced subscription was concurrently dropped The attached fixes it 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. 3/ the "privileges" checks are still also done before the lock acquisition because we don't want to lock an object we don't have privileges on. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From 72a53d1991e7cd9d4a52f48284ddc974a0a4ae65 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> 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 <[email protected]> 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
