Hi, On Fri, Jul 03, 2026 at 10:20:32AM +0530, Dilip Kumar wrote: > On Fri, Jul 3, 2026 at 9:49 AM Bertrand Drouvot > <[email protected]> wrote: > > > > Hi Kuroda-san, > > > > On Fri, Jul 03, 2026 at 03:13:08AM +0000, Hayato Kuroda (Fujitsu) wrote: > > > Dear Bertrand, > > > > > > > Yeah, but I think they would produce "tuple concurrently updated" error > > > > (due to > > > > CatalogTupleUpdate) so that invalid information could not be used. > > > > > > I confirmed with PG14 that tuple concurrently updated ERROR can be raised > > > when > > > ALTER SUBSCRIPTION DISABLE happens concurrently: > > > > > > ``` > > > postgres=# ALTER SUBSCRIPTION sub DISABLE ; > > > ERROR: tuple concurrently updated > > > ``` > > > > Yeah, reproducible by using a breakpoint just before acquiring the lock for > > example. > > > > > It might be harmless but I think the correct ERROR should be reported: > > > the patch > > > should be backpatched. Thought? > > > > I'm not sure about the back patch part as it would only improve error > > messages > > in a rare race condition (and there is no risk of invalid data being used). > > Patch LGTM.
Thanks for looking at it! > IMHO we can backpatch this as it is a small change and > also fixes the bug, without this fix a non-superuser executing ALTER > SUBSCRIPTION could bypass the password_required=false restriction if a > concurrent transaction > updated that flag. I don't think that's right. I just tested it with a breakpoint that way: ALTER SUBSCRIPTION mysub SET (password_required = true); ALTER SUBSCRIPTION mysub OWNER TO nonsuperuser; gdb breakpoint at subscriptioncmds.c:1714 on session 1 (nonsuperuser) session 1 (as nonsuperuser): start ALTER SUBSCRIPTION mysub SET (binary = true); session 1 is paused by the breakpoint session 2 (as superuser): ALTER SUBSCRIPTION mysub SET (password_required = false); continue session 1, gives: postgres=> ALTER SUBSCRIPTION mysub SET (binary = true); ERROR: tuple concurrently updated So it's also "protected" by this error. > but given the patch's simplicity, I recommend > backpatching. That's right but that would only improve error messages. That said, looking closer, they are elog() ones, so "not expected" to occur so yeah backpatch does make sense. That said, what about also fixing DropSubscription() like in the 0002 attached? (that would also produce those elog() messages in case of concurrent DROP or ALTER). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From 483a9b0d6b0344cdb0562c76ef3a460a31125e96 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Fri, 3 Jul 2026 05:17:31 +0000 Subject: [PATCH v2 1/2] 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: Dilip Kumar <[email protected]> Reviewed-by: Hayato Kuroda (Fujitsu) <[email protected]> Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- 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
>From 6e357e922e48547ff9a07ea3e0fe5f69624207f0 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Fri, 3 Jul 2026 05:18:44 +0000 Subject: [PATCH v2 2/2] Re-read subscription state after lock in DropSubscription Similarly to what has been done for AlterSubscription() in XXXX, 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(). Author: Bertrand Drouvot <[email protected]> Reviewed-by: Reviewed-by: Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg --- src/backend/commands/subscriptioncmds.c | 36 ++++++++++++++++++------- 1 file changed, 27 insertions(+), 9 deletions(-) 100.0% src/backend/commands/ diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index be03b3eb7e1..6db92a931b9 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -2582,17 +2582,8 @@ 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())) @@ -2608,6 +2599,33 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel) */ 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. + */ + ReleaseSysCache(tup); + tup = SearchSysCache2(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); + 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); -- 2.34.1
