agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feed[PATCH v1] Re-read subscription state after lock in AlterSubscription
35+ messages / 6 participants
[nested] [flat]
* [PATCH v1] Re-read subscription state after lock in AlterSubscription
@ 2026-07-02 11:07 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-02 11:07 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
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
--MHKy70OBPdw7Q14M--
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re-read subscription state after lock in AlterSubscription
@ 2026-07-02 12:08 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-02 12:08 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
Attachments:
[text/x-diff] v1-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch (3.9K, ../../akZUpiDa1UfmzYxL@bdtpg/2-v1-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch)
download | inline diff:
From 72a53d1991e7cd9d4a52f48284ddc974a0a4ae65 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
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 <bertranddrouvot.pg@gmail.com>
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
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-02 12:27 Dilip Kumar <dilipbalaut@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Dilip Kumar @ 2026-07-02 12:27 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Thu, Jul 2, 2026 at 5:38 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> 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.
>
Thanks Bertrand, yeah this seems like a valid issue, and I agree we
need to reread the subscription after acquiring the object lock.
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 35+ messages in thread
* RE: Re-read subscription state after lock in AlterSubscription
@ 2026-07-02 12:48 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-07-02 12:48 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
Dear Bertrand,
Good catch. Current code allows that old `sub` value is retained, so it sounds
reasonable fix even for me.
BTW, the issue that GetSubscription() is called before the LockSharedObject() looks
the existing issues even on REL_13_STABLE. So does it mean that there were no
cases that concurrent altering can be the unexpected state? At least,
"retain_dead_tuples" can avoid the issue because the launcher manages the
conflict slot.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-02 13:20 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-02 13:20 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
Hi Kuroda-san,
On Thu, Jul 02, 2026 at 12:48:53PM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Bertrand,
>
> Good catch. Current code allows that old `sub` value is retained, so it sounds
> reasonable fix even for me.
>
> BTW, the issue that GetSubscription() is called before the LockSharedObject() looks
> the existing issues even on REL_13_STABLE. So does it mean that there were no
> cases that concurrent altering can be the unexpected state?
Yeah, but I think they would produce "tuple concurrently updated" error (due to
CatalogTupleUpdate) so that invalid information could not be used.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 35+ messages in thread
* RE: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 03:13 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-07-03 03:13 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
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
```
It might be harmless but I think the correct ERROR should be reported: the patch
should be backpatched. Thought?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 04:19 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 04:19 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; Dilip Kumar <dilipbalaut@gmail.com>
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).
Since a5918fddf10, that's a different story because a table creation is now
involved.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 04:50 Dilip Kumar <dilipbalaut@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Dilip Kumar @ 2026-07-03 04:50 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Fri, Jul 3, 2026 at 9:49 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> 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. 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. However, we could argue that this is a corner case
and can be skipped but given the patch's simplicity, I recommend
backpatching.
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v2 1/2] Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 05:17 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 05:17 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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
--IX1d9TnmxjGKMYcM
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v2 2/2] Re-read subscription state after lock in DropSubscription
@ 2026-07-03 05:18 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 05:18 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
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
--IX1d9TnmxjGKMYcM--
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 05:52 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 05:52 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
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
> <bertranddrouvot.pg@gmail.com> 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
Attachments:
[text/x-diff] v2-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch (4.0K, ../../akdOLGFblqA3Yvd6@bdtpg/2-v2-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch)
download | inline diff:
From 483a9b0d6b0344cdb0562c76ef3a460a31125e96 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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
[text/x-diff] v2-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch (2.9K, ../../akdOLGFblqA3Yvd6@bdtpg/3-v2-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch)
download | inline diff:
From 6e357e922e48547ff9a07ea3e0fe5f69624207f0 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
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 <bertranddrouvot.pg@gmail.com>
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
^ permalink raw reply [nested|flat] 35+ messages in thread
* RE: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 08:08 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 2 replies; 35+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-07-03 08:08 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Friday, July 3, 2026 1:53 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>
> > 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.
+1 for backpatching, even if it's rare, the "ERROR: tuple concurrently updated"
message seems confusing to me.
>
> 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).
For the patch, I'm not sure if we must repeat the checks twice. Could we
simply move the original checks to after we take the lock? At least, the
GetSubscription() call and the password check can be moved there and old codes
can be deleted.
BTW, this may not be strictly related, but I think it's not safe to do the
ownership check before locking the subscription as well. If the subscription is
concurrently dropped, a "tuple concurrently updated" error can still occur.
(Thanks to Kuroda-San for discussing this with me off-list.)
Best Regards,
Hou zj
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 09:03 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 09:03 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hi,
On Fri, Jul 03, 2026 at 08:08:13AM +0000, Zhijie Hou (Fujitsu) wrote:
> On Friday, July 3, 2026 1:53 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> >
> > 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).
>
> For the patch, I'm not sure if we must repeat the checks twice.
Thanks for looking at it!
> Could we
> simply move the original checks to after we take the lock? At least, the
> GetSubscription() call and the password check can be moved there and old codes
> can be deleted.
I'm not sure which checks you refer to. The ones that are keep before the lock
acquisition are because we don't want to lock an object we don't have privileges
on (see remark 3 in [1]).
> BTW, this may not be strictly related, but I think it's not safe to do the
> ownership check before locking the subscription as well. If the subscription is
> concurrently dropped, a "tuple concurrently updated" error can still occur.
That's right, I explained why in remark number 2 in [1]:
"
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.
"
Does that make sense?
[1]: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 35+ messages in thread
* RE: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 09:56 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-07-03 09:56 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Friday, July 3, 2026 5:03 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> On Fri, Jul 03, 2026 at 08:08:13AM +0000, Zhijie Hou (Fujitsu) wrote:
> > On Friday, July 3, 2026 1:53 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > 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).
> >
> > For the patch, I'm not sure if we must repeat the checks twice.
>
> Thanks for looking at it!
>
> > Could we
> > simply move the original checks to after we take the lock? At least,
> > the
> > GetSubscription() call and the password check can be moved there and
> > old codes can be deleted.
>
> I'm not sure which checks you refer to. The ones that are keep before the lock
> acquisition are because we don't want to lock an object we don't have
> privileges on (see remark 3 in [1]).
I was referring to the password_required check and the GetSubscription() call.
I think failing the password_required check does not necessarily mean we do not
have the permission to lock the subscription, It seems to me we only need to
disallow changing the subscription data in this case. In
DropSubscription, we take a lock on the subscription regardless of
password_required.
Best Regards,
Hou zj
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 10:15 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 1 reply; 35+ messages in thread
From: Amit Kapila @ 2026-07-03 10:15 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Fri, Jul 3, 2026 at 1:38 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, July 3, 2026 1:53 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> >
> > > 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.
>
> +1 for backpatching, even if it's rare, the "ERROR: tuple concurrently updated"
> message seems confusing to me.
>
I also think backpatching makes sense. BTW, I have a comment:
+ heap_freetuple(tup);
+ tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, ObjectIdGetDatum(MyDatabaseId),
+ CStringGetDatum(stmt->subname));
heap_freetuple() could be done before acquiring the lock, is there a
reason to keep it after lock?
> >
> > 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).
>
> For the patch, I'm not sure if we must repeat the checks twice. Could we
> simply move the original checks to after we take the lock? At least, the
> GetSubscription() call and the password check can be moved there and old codes
> can be deleted.
>
Isn't the same true for the AlterSubscription() case as well? Also, I
noticed that AlterPublication() does the same trick but it uses
PUBLICATIONOID cacheid, so shouldn't we use SUBSCRIPTIONOID cacheid
here as well? I think this is to prevent the case where the same name
pub/sub is recreated after lock.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v3 2/4] Re-read subscription state after lock in DropSubscription
@ 2026-07-03 11:54 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 11:54 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
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 517d46f47f9..e23b366a87d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2567,17 +2567,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()))
@@ -2587,12 +2578,39 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
/* 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
* the replication workers).
*/
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))
+ 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
--3eAVwo4vDNlHc8RC
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v3-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v4 2/4] Re-read subscription state after lock in DropSubscription
@ 2026-07-03 11:54 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 11:54 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 40 ++++++++++++++++++-------
1 file changed, 29 insertions(+), 11 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 517d46f47f9..c9e7fbdb47b 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2567,25 +2567,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
@@ -2593,6 +2583,34 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);
+ /* DROP hook for the subscription being removed */
+ InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+
+ /*
+ * 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))
+ 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
--b+bwjVhifbzgesmw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v5 2/4] Re-read subscription state after lock in DropSubscription
@ 2026-07-03 11:54 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 11:54 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 40 ++++++++++++++++++-------
1 file changed, 29 insertions(+), 11 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a2973aa229e..9bb9fa90503 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2601,25 +2601,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
@@ -2627,6 +2617,34 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);
+ /* DROP hook for the subscription being removed */
+ InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+
+ /*
+ * 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))
+ 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
--1HOA+h/avFjxXHxm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v6 2/4] Re-read subscription state after lock in DropSubscription
@ 2026-07-03 11:54 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 11:54 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
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
--YgvQoaaPJLlueFuQ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v3 1/4] Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 12:28 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 12:28 UTC (permalink / raw)
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.
Remark:
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 30 +++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4292e7fb8f4..517d46f47f9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1686,6 +1686,25 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -1695,11 +1714,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.
@@ -1710,8 +1724,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));
--
2.34.1
--3eAVwo4vDNlHc8RC
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v3-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v4 1/4] Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 12:28 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 12:28 UTC (permalink / raw)
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.
Remark:
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 30 +++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4292e7fb8f4..517d46f47f9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1686,6 +1686,25 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -1695,11 +1714,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.
@@ -1710,8 +1724,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));
--
2.34.1
--b+bwjVhifbzgesmw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v5 1/4] Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 12:28 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 12:28 UTC (permalink / raw)
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.
Remark:
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 30 +++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d3eef2a3efa..a2973aa229e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1728,6 +1728,25 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -1737,11 +1756,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.
@@ -1752,8 +1766,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));
--
2.34.1
--1HOA+h/avFjxXHxm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* [PATCH v6 1/4] Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 12:28 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 12:28 UTC (permalink / raw)
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com>
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
--YgvQoaaPJLlueFuQ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch"
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-03 15:39 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 3 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-03 15:39 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hi,
On Fri, Jul 03, 2026 at 03:45:34PM +0530, Amit Kapila wrote:
> On Fri, Jul 3, 2026 at 1:38 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Friday, July 3, 2026 1:53 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > > 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.
> >
> > +1 for backpatching, even if it's rare, the "ERROR: tuple concurrently updated"
> > message seems confusing to me.
> >
>
> I also think backpatching makes sense. BTW, I have a comment:
Thanks for looking at it!
> + heap_freetuple(tup);
> + tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, ObjectIdGetDatum(MyDatabaseId),
> + CStringGetDatum(stmt->subname));
>
> heap_freetuple() could be done before acquiring the lock, is there a
> reason to keep it after lock?
No particular reason, could be done before. Done in 0001 attached.
>
> > >
> > > 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).
> >
> > For the patch, I'm not sure if we must repeat the checks twice. Could we
> > simply move the original checks to after we take the lock? At least, the
> > GetSubscription() call and the password check can be moved there and old codes
> > can be deleted.
> >
>
> Isn't the same true for the AlterSubscription() case as well?
I think there is no need to lock if we are later going to disallow changing the
subscription data due to the password_required/superuser check.
That said moving it as suggested by Hou-san, does simplify the code and the lock
is not held for long, so done that way in 0001.
> Also, I
> noticed that AlterPublication() does the same trick but it uses
> PUBLICATIONOID cacheid, so shouldn't we use SUBSCRIPTIONOID cacheid
> here as well? I think this is to prevent the case where the same name
> pub/sub is recreated after lock.
Oh right and I did it that way in 0001 and 0002.
But while doing this and looking closely, I'm not sure AlterPublication() does
it right. Indeed, in theory, the OID could have been re-used too (between the
time we did the name resolution and the time we lock the publication). I think
what is needed is something similar to RangeVarGetRelidExtended(), means do the
name resolution, acl check (ownership) and lock acquisition, all in unison.
That's what 0003 is trying to achieve for the subscription and 0004 for the
publication.
What do you think?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v3-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch (3.9K, ../../akfXqSGeRpL+2Ypz@bdtpg/2-v3-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch)
download | inline diff:
From 37c9434730537f124223aed18e874103ad37969e Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 12:28:42 +0000
Subject: [PATCH v3 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.
Remark:
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 30 +++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4292e7fb8f4..517d46f47f9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1686,6 +1686,25 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -1695,11 +1714,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.
@@ -1710,8 +1724,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));
--
2.34.1
[text/x-diff] v3-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch (3.1K, ../../akfXqSGeRpL+2Ypz@bdtpg/3-v3-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch)
download | inline diff:
From 58b9cfaa5f806d527ad36904f8b3b67c05b70478 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 11:54:29 +0000
Subject: [PATCH v3 2/4] 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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
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 517d46f47f9..e23b366a87d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2567,17 +2567,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()))
@@ -2587,12 +2578,39 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
/* 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
* the replication workers).
*/
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))
+ 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
[text/x-diff] v3-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch (8.1K, ../../akfXqSGeRpL+2Ypz@bdtpg/4-v3-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch)
download | inline diff:
From 0915ae18376961d702695bb6617b8c04b0e50bf1 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 14:03:13 +0000
Subject: [PATCH v3 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 <bertranddrouvot.pg@gmail.com>
Reviewed-by:
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 167 +++++++++++++++++-------
1 file changed, 118 insertions(+), 49 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e23b366a87d..866341c2cfb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -50,6 +50,7 @@
#include "replication/walsender.h"
#include "replication/worker_internal.h"
#include "storage/lmgr.h"
+#include "storage/sinval.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/guc.h"
@@ -1592,23 +1593,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)
@@ -1686,25 +1731,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
-
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -2570,11 +2596,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
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);
-
/* DROP hook for the subscription being removed */
InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
@@ -2583,20 +2604,68 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
/*
* 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))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("subscription \"%s\" does not exist",
- stmt->subname)));
+ 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);
+ 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);
+ }
+ }
form = (Form_pg_subscription) GETSTRUCT(tup);
subowner = form->subowner;
--
2.34.1
[text/x-diff] v3-0004-Add-invalidation-based-retry-loop-for-AlterPublic.patch (5.5K, ../../akfXqSGeRpL+2Ypz@bdtpg/5-v3-0004-Add-invalidation-based-retry-loop-for-AlterPublic.patch)
download | inline diff:
From f8f68c999dbd52ed566a9953b35292a1218015fe Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 14:46:42 +0000
Subject: [PATCH v3 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 commit XXXX.
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.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by:
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/publicationcmds.c | 105 +++++++++++++++++--------
1 file changed, 72 insertions(+), 33 deletions(-)
100.0% src/backend/commands/
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,
--
2.34.1
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-04 08:00 Dilip Kumar <dilipbalaut@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2 siblings, 1 reply; 35+ messages in thread
From: Dilip Kumar @ 2026-07-04 08:00 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Fri, Jul 3, 2026 at 9:09 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Fri, Jul 03, 2026 at 03:45:34PM +0530, Amit Kapila wrote:
> > On Fri, Jul 3, 2026 at 1:38 PM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Friday, July 3, 2026 1:53 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> > > >
> > > > > 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.
> > >
> > > +1 for backpatching, even if it's rare, the "ERROR: tuple concurrently updated"
> > > message seems confusing to me.
> > >
> >
> > I also think backpatching makes sense. BTW, I have a comment:
>
> Thanks for looking at it!
>
> > + heap_freetuple(tup);
> > + tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, ObjectIdGetDatum(MyDatabaseId),
> > + CStringGetDatum(stmt->subname));
> >
> > heap_freetuple() could be done before acquiring the lock, is there a
> > reason to keep it after lock?
>
> No particular reason, could be done before. Done in 0001 attached.
>
> >
> > > >
> > > > 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).
> > >
> > > For the patch, I'm not sure if we must repeat the checks twice. Could we
> > > simply move the original checks to after we take the lock? At least, the
> > > GetSubscription() call and the password check can be moved there and old codes
> > > can be deleted.
> > >
> >
> > Isn't the same true for the AlterSubscription() case as well?
>
> I think there is no need to lock if we are later going to disallow changing the
> subscription data due to the password_required/superuser check.
>
> That said moving it as suggested by Hou-san, does simplify the code and the lock
> is not held for long, so done that way in 0001.
>
> > Also, I
> > noticed that AlterPublication() does the same trick but it uses
> > PUBLICATIONOID cacheid, so shouldn't we use SUBSCRIPTIONOID cacheid
> > here as well? I think this is to prevent the case where the same name
> > pub/sub is recreated after lock.
>
> Oh right and I did it that way in 0001 and 0002.
>
> But while doing this and looking closely, I'm not sure AlterPublication() does
> it right. Indeed, in theory, the OID could have been re-used too (between the
> time we did the name resolution and the time we lock the publication). I think
> what is needed is something similar to RangeVarGetRelidExtended(), means do the
> name resolution, acl check (ownership) and lock acquisition, all in unison.
>
> That's what 0003 is trying to achieve for the subscription and 0004 for the
> publication.
>
> What do you think?
>
0003:
It looks like the implementation of DROP SUBSCRIPTION IF EXISTS has a
concurrent drop race condition in DropSubscription(). Currently, if
stmt->missing_ok is true, the initial lookup safely handles a missing
subscription. However, once a subscription is found and the code
enters the drop loop, a second internal lookup/refetch happens. If a
concurrent transaction drops the subscription after our initial check
but before this internal refetch, the code throws an error.
Essentially, the loop completely ignores the missing_ok flag during
the refetch phase. Am I missing something?
--
Regards,
Dilip Kumar
Google
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-04 08:19 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-04 08:19 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hi,
On Sat, Jul 04, 2026 at 01:30:08PM +0530, Dilip Kumar wrote:
> On Fri, Jul 3, 2026 at 9:09 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > But while doing this and looking closely, I'm not sure AlterPublication() does
> > it right. Indeed, in theory, the OID could have been re-used too (between the
> > time we did the name resolution and the time we lock the publication). I think
> > what is needed is something similar to RangeVarGetRelidExtended(), means do the
> > name resolution, acl check (ownership) and lock acquisition, all in unison.
> >
> > That's what 0003 is trying to achieve for the subscription and 0004 for the
> > publication.
> >
> > What do you think?
> >
> 0003:
>
> It looks like the implementation of DROP SUBSCRIPTION IF EXISTS has a
> concurrent drop race condition in DropSubscription(). Currently, if
> stmt->missing_ok is true, the initial lookup safely handles a missing
> subscription. However, once a subscription is found and the code
> enters the drop loop, a second internal lookup/refetch happens. If a
> concurrent transaction drops the subscription after our initial check
> but before this internal refetch, the code throws an error.
> Essentially, the loop completely ignores the missing_ok flag during
> the refetch phase.
Good catch, will fix, thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 35+ messages in thread
* RE: Re-read subscription state after lock in AlterSubscription
@ 2026-07-06 02:43 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2 siblings, 1 reply; 35+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-07-06 02:43 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Dear Bertrand,
Thanks for updating the patch. I found one issue:
```
/* DROP hook for the subscription being removed */
InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
```
I think the reporting should be after the loop, otherwise the wrong subid can be
reported. Am I missing something?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-06 04:54 Amit Kapila <amit.kapila16@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2 siblings, 1 reply; 35+ messages in thread
From: Amit Kapila @ 2026-07-06 04:54 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Fri, Jul 3, 2026 at 9:09 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Fri, Jul 03, 2026 at 03:45:34PM +0530, Amit Kapila wrote:
>
> But while doing this and looking closely, I'm not sure AlterPublication() does
> it right. Indeed, in theory, the OID could have been re-used too (between the
> time we did the name resolution and the time we lock the publication). I think
> what is needed is something similar to RangeVarGetRelidExtended(), means do the
> name resolution, acl check (ownership) and lock acquisition, all in unison.
>
It seems RangeVarGetRelidExtended() also doesn't do the additional
invalidation handling if the caller already has an appropriate lock,
see comments [1]. Apart from that also, I am not sure it is a good
ideal to add this additional handling in Pub/Sub DDLs as in worst case
scenario even if the OID is re-used the user will face "tuple
concurrently updated" or similar ERRORs, it won't do anything wrong.
So for such rare cases, it doesn't seem worth adding this additional
re-checking machinery. Based on the same theory, I am thinking again
whether it is worth backpatching these patches? I mean these fall into
the category of improving user facing messages during Pub/Sub DDLs, so
isn't it okay to just push this work in HEAD?
[1]:
/*
* If no lock requested, we assume the caller knows what they're
* doing. They should have already acquired a heavyweight lock on
* this relation earlier in the processing of this same statement, so
* it wouldn't be appropriate to AcceptInvalidationMessages() here, as
* that might pull the rug out from under them.
*/
if (lockmode == NoLock)
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-06 05:01 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 0 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-06 05:01 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hi Kuroda-san,
On Mon, Jul 06, 2026 at 02:43:20AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Bertrand,
>
> Thanks for updating the patch. I found one issue:
>
> ```
> /* DROP hook for the subscription being removed */
> InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
>
> ```
>
> I think the reporting should be after the loop, otherwise the wrong subid can be
> reported.
Yeah, and I think this is an existing behavior not related to the patch. Currently,
InvokeObjectDropHook() is called before we lock the subscription. I think that
makes more sense to do it after the lock is acquired, so this is now changed in
0002.
Also addressing Dilip's comment in the attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch (3.9K, ../../aks2jHJeKW8Ay2N5@bdtpg/2-v4-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch)
download | inline diff:
From 78fa91b3ae24c16e4b6255f4c3550e5ca0d556d8 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 12:28:42 +0000
Subject: [PATCH v4 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.
Remark:
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 30 +++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4292e7fb8f4..517d46f47f9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1686,6 +1686,25 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -1695,11 +1714,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.
@@ -1710,8 +1724,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));
--
2.34.1
[text/x-diff] v4-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch (3.3K, ../../aks2jHJeKW8Ay2N5@bdtpg/3-v4-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch)
download | inline diff:
From f02a9fc9202f493b824cefb46a167f44a7b2e786 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 11:54:29 +0000
Subject: [PATCH v4 2/4] 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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 40 ++++++++++++++++++-------
1 file changed, 29 insertions(+), 11 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 517d46f47f9..c9e7fbdb47b 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2567,25 +2567,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
@@ -2593,6 +2583,34 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);
+ /* DROP hook for the subscription being removed */
+ InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+
+ /*
+ * 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))
+ 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
[text/x-diff] v4-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch (8.7K, ../../aks2jHJeKW8Ay2N5@bdtpg/4-v4-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch)
download | inline diff:
From 501cd86bcf3f66d85770b27133820cb4964ed35e Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 6 Jul 2026 04:44:13 +0000
Subject: [PATCH v4 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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 189 +++++++++++++++++-------
1 file changed, 134 insertions(+), 55 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index c9e7fbdb47b..615c3921bc3 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -50,6 +50,7 @@
#include "replication/walsender.h"
#include "replication/worker_internal.h"
#include "storage/lmgr.h"
+#include "storage/sinval.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/guc.h"
@@ -1592,23 +1593,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)
@@ -1686,25 +1731,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
-
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -2567,38 +2593,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;
- /* DROP hook for the subscription being removed */
- InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+ for (;;)
+ {
+ uint64 inval_count = SharedInvalidMessageCounter;
- /*
- * 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));
+ 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)));
+ if (!HeapTupleIsValid(tup))
+ {
+ if (retry)
+ UnlockSharedObject(SubscriptionRelationId, oldSubId, 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;
+ }
+
+ 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
[text/x-diff] v4-0004-Add-invalidation-based-retry-loop-for-AlterPublic.patch (5.6K, ../../aks2jHJeKW8Ay2N5@bdtpg/5-v4-0004-Add-invalidation-based-retry-loop-for-AlterPublic.patch)
download | inline diff:
From e1bf848f56a93bd7f79a300eebea52a2429f21a6 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 14:46:42 +0000
Subject: [PATCH v4 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 commit XXXX.
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.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/publicationcmds.c | 105 +++++++++++++++++--------
1 file changed, 72 insertions(+), 33 deletions(-)
100.0% src/backend/commands/
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,
--
2.34.1
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-06 05:43 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-06 05:43 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hi,
On Mon, Jul 06, 2026 at 10:24:26AM +0530, Amit Kapila wrote:
> On Fri, Jul 3, 2026 at 9:09 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > On Fri, Jul 03, 2026 at 03:45:34PM +0530, Amit Kapila wrote:
> >
> > But while doing this and looking closely, I'm not sure AlterPublication() does
> > it right. Indeed, in theory, the OID could have been re-used too (between the
> > time we did the name resolution and the time we lock the publication). I think
> > what is needed is something similar to RangeVarGetRelidExtended(), means do the
> > name resolution, acl check (ownership) and lock acquisition, all in unison.
> >
>
> It seems RangeVarGetRelidExtended() also doesn't do the additional
> invalidation handling if the caller already has an appropriate lock,
> see comments [1].
From what I can see, the NoLock callers of RangeVarGetRelidExtended(), are for
callers that don't modify objects (they are "read only" callers). The only
exception is nextval() but there is an XXX that mentions it.
Here we modify the subscription or publication, so I don't think we are in the
NoLock spirit of RangeVarGetRelidExtended().
> Apart from that also, I am not sure it is a good
> ideal to add this additional handling in Pub/Sub DDLs as in worst case
> scenario even if the OID is re-used the user will face "tuple
> concurrently updated" or similar ERRORs, it won't do anything wrong.
That's probably right before a5918fddf10, but with conflict_log_destination='table'
we now perform creating/dropping a table based on the stale data before it ever
reaches CatalogTupleUpdate().
Also, even prior a5918fddf10 I believe there might be situations that could not
produce the tuple concurrently updated" but corrupt the tuple (say if vacuum had
the time to clean up the tuple and the same ctid is reused). Probably extremely
rare scenario, though.
> So for such rare cases, it doesn't seem worth adding this additional
> re-checking machinery. Based on the same theory, I am thinking again
> whether it is worth backpatching these patches? I mean these fall into
> the category of improving user facing messages during Pub/Sub DDLs, so
> isn't it okay to just push this work in HEAD?
Those are not ereport() but elog() messages so not "expected" to happen.
Based on the above, I'm thinking that backpatching 0001 and 0002 and keep
0003 and 0004 only for HEAD could make sense.
What do you think?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-06 09:37 Amit Kapila <amit.kapila16@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Amit Kapila @ 2026-07-06 09:37 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Mon, Jul 6, 2026 at 11:13 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Mon, Jul 06, 2026 at 10:24:26AM +0530, Amit Kapila wrote:
> > On Fri, Jul 3, 2026 at 9:09 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > On Fri, Jul 03, 2026 at 03:45:34PM +0530, Amit Kapila wrote:
> > >
> > > But while doing this and looking closely, I'm not sure AlterPublication() does
> > > it right. Indeed, in theory, the OID could have been re-used too (between the
> > > time we did the name resolution and the time we lock the publication). I think
> > > what is needed is something similar to RangeVarGetRelidExtended(), means do the
> > > name resolution, acl check (ownership) and lock acquisition, all in unison.
> > >
> >
> > It seems RangeVarGetRelidExtended() also doesn't do the additional
> > invalidation handling if the caller already has an appropriate lock,
> > see comments [1].
>
> From what I can see, the NoLock callers of RangeVarGetRelidExtended(), are for
> callers that don't modify objects (they are "read only" callers). The only
> exception is nextval() but there is an XXX that mentions it.
>
> Here we modify the subscription or publication, so I don't think we are in the
> NoLock spirit of RangeVarGetRelidExtended().
>
IIUC, here the risk is that during the first read and before we take
the Lock, if the same OID is reused for a different subscription then
we may end up modifying an unintended subscription. I think that is a
theoretical risk rather than a practical one. We already note similar
risk at other places, like see comments atop GetNewOidWithIndex(Since
the OID is not immediately inserted into the table, there is a race
condition here; but a problem could occur only if someone else managed
to cycle through 2^32 OIDs and generate the same OID before we finish
inserting our row. This seems unlikely to be a problem.). Similarly
comments atop GetNewRelFileNumber( As with GetNewOidWithIndex(), there
is some theoretical risk of a race) made a note of similar risk. I
feel we should note this in comments rather than trying to add
additional code to handle it.
As per my understanding the loop exists in RangeVarGetRelidExtended()
because relation lookup follows the name, and no lock can pin a
name->OID binding, so the binding can be rebound by concurrent DDL
between lookup and lock. Concretely, our lock protects relation X's
OID, but it can't stop someone renaming X away and handing X's old
name to a different relation Y. The lockable thing (the OID) and the
thing that changes (the name binding) are different objects. The loop
detects exactly this: acquiring the lock runs
AcceptInvalidationMessages(), and if any invalidations arrived while
we waited (inval_count == SharedInvalidMessageCounter), it re-resolves
the name. If the name now maps to a different OID than the one we
locked, it releases the old lock and locks the new OID. It repeats
until the name resolves to the same OID across a lock acquisition —
i.e. until the binding is stable while locked. OTOH, the subscription
path follows the locked OID instead, so it needs only a single
re-read.
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-07-06 14:53 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 35+ messages in thread
From: Bertrand Drouvot @ 2026-07-06 14:53 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Hi,
On Mon, Jul 06, 2026 at 03:07:24PM +0530, Amit Kapila wrote:
> On Mon, Jul 6, 2026 at 11:13 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > > It seems RangeVarGetRelidExtended() also doesn't do the additional
> > > invalidation handling if the caller already has an appropriate lock,
> > > see comments [1].
> >
> > From what I can see, the NoLock callers of RangeVarGetRelidExtended(), are for
> > callers that don't modify objects (they are "read only" callers). The only
> > exception is nextval() but there is an XXX that mentions it.
> >
> > Here we modify the subscription or publication, so I don't think we are in the
> > NoLock spirit of RangeVarGetRelidExtended().
> >
>
> IIUC, here the risk is that during the first read and before we take
> the Lock, if the same OID is reused for a different subscription then
> we may end up modifying an unintended subscription. I think that is a
> theoretical risk rather than a practical one.
I agree OID reuse itself is theoretical.
> As per my understanding the loop exists in RangeVarGetRelidExtended()
> because relation lookup follows the name, and no lock can pin a
> name->OID binding, so the binding can be rebound by concurrent DDL
> between lookup and lock. Concretely, our lock protects relation X's
> OID, but it can't stop someone renaming X away and handing X's old
> name to a different relation Y.
Not sure it's only about renaming. The commit message of 4240e429d0c mentions
"This was particularly problematic in the case where a table had been dropped
and recreated". b3ad5d02c9c also used the same logic and reasoning "avoids
needlessly failing when the object of interest is concurrently dropped and
recreated".
Also in 4240e429d0c: "there's nothing at all here to guard against similar race
conditions for non-relations": I think that subscriptions and publications are
among those non-relations cases.
> The lockable thing (the OID) and the
> thing that changes (the name binding) are different objects. The loop
> detects exactly this: acquiring the lock runs
> AcceptInvalidationMessages(), and if any invalidations arrived while
> we waited (inval_count == SharedInvalidMessageCounter), it re-resolves
> the name. If the name now maps to a different OID than the one we
> locked, it releases the old lock and locks the new OID. It repeats
> until the name resolves to the same OID across a lock acquisition —
> i.e. until the binding is stable while locked. OTOH, the subscription
> path follows the locked OID instead, so it needs only a single
> re-read.
I think that's for example what RemoveRelations() was doing before 4240e429d0c
and what get_object_address() was doing before b3ad5d02c9c:
1/ Resolve name to OID
2/ Lock by OID
3/ Check if it still exists
4/ If gone then elog(ERROR..
but has been changed in b3ad5d02c9c with a retry loop.
Also looking at get_object_address(), I can see that it handles publications and
subscriptions:
case OBJECT_PUBLICATION:
case OBJECT_SUBSCRIPTION:
address = get_object_address_unqualified(objtype,
castNode(String, object), missing_ok);
and that DROP PUBLICATION goes through it, so that it already benefits from the
retry loop in get_object_address().
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).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-08-04 05:55 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 2 replies; 35+ messages in thread
From: Bertrand Drouvot @ 2026-08-04 05:55 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
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.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v5-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch (3.9K, ../../anF+wLFJdW9sNcvh@bdtpg/2-v5-0001-Re-read-subscription-state-after-lock-in-AlterSub.patch)
download | inline diff:
From 4ff620ab3401573443fec13488386f84c896533b Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 12:28:42 +0000
Subject: [PATCH v5 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.
Remark:
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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 30 +++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d3eef2a3efa..a2973aa229e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1728,6 +1728,25 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -1737,11 +1756,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.
@@ -1752,8 +1766,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));
--
2.34.1
[text/x-diff] v5-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch (3.3K, ../../anF+wLFJdW9sNcvh@bdtpg/3-v5-0002-Re-read-subscription-state-after-lock-in-DropSubs.patch)
download | inline diff:
From 483d4cc028dab24cdfb46fdec76a1e12d78bf1e7 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 11:54:29 +0000
Subject: [PATCH v5 2/4] 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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 40 ++++++++++++++++++-------
1 file changed, 29 insertions(+), 11 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index a2973aa229e..9bb9fa90503 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2601,25 +2601,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
@@ -2627,6 +2617,34 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
*/
LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);
+ /* DROP hook for the subscription being removed */
+ InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+
+ /*
+ * 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))
+ 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
[text/x-diff] v5-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch (8.7K, ../../anF+wLFJdW9sNcvh@bdtpg/4-v5-0003-Add-invalidation-based-retry-loop-for-Alter-Drop-.patch)
download | inline diff:
From cc7334ab73d69e9b289f69b905a416b8caf30c75 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 6 Jul 2026 04:44:13 +0000
Subject: [PATCH v5 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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/subscriptioncmds.c | 189 +++++++++++++++++-------
1 file changed, 134 insertions(+), 55 deletions(-)
100.0% src/backend/commands/
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9bb9fa90503..4acefbdcf7a 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"
@@ -1634,23 +1635,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)
@@ -1728,25 +1773,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = 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);
-
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
@@ -2601,38 +2627,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;
- /* DROP hook for the subscription being removed */
- InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+ for (;;)
+ {
+ uint64 inval_count = SharedInvalidMessageCounter;
- /*
- * 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));
+ 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)));
+ if (!HeapTupleIsValid(tup))
+ {
+ if (retry)
+ UnlockSharedObject(SubscriptionRelationId, oldSubId, 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;
+ }
+
+ 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
[text/x-diff] v5-0004-Add-invalidation-based-retry-loop-for-AlterPublic.patch (5.6K, ../../anF+wLFJdW9sNcvh@bdtpg/5-v5-0004-Add-invalidation-based-retry-loop-for-AlterPublic.patch)
download | inline diff:
From c84584a8d74f00b3acaf249b5f8e666d2c24075f Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 3 Jul 2026 14:46:42 +0000
Subject: [PATCH v5 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 commit XXXX.
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.
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
---
src/backend/commands/publicationcmds.c | 105 +++++++++++++++++--------
1 file changed, 72 insertions(+), 33 deletions(-)
100.0% src/backend/commands/
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,
--
2.34.1
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-08-05 11:28 Shlok Kyal <shlok.kyal.oss@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
1 sibling, 0 replies; 35+ messages in thread
From: Shlok Kyal @ 2026-08-05 11:28 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Tue, 4 Aug 2026 at 11:25, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> 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 Bertrand,
While reviewing another patch, I found a bug on HEAD. It occurs when
ALTER SUBSCRIPTION ... REFRESH PUBLICATION and DROP SUBSCRIPTION runs
concurrently.
Suppose we have a logical replication setup between subscriber 'sub1'
and publisher 'pub1'.
Initially, we have:
postgres=# select oid, subname from pg_subscription;
oid | subname
-------+---------
16389 | sub1
(1 row)
postgres=# select * from pg_subscription_rel;
srsubid | srrelid | srsubstate | srsublsn
---------+---------+------------+------------
16389 | 16384 | r | 0/01750B90
(1 row)
Suppose we have two sessions S1 and S2 (attached GDB in S1).
S1: ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION. (stops at a
breakpoint at 'LockSharedObject(SubscriptionRelationId, subid, 0,
AccessExclusiveLock)')
S2: DROP SUBSCRIPTION sub1;
S1: continue the execution
Both ALTER and DROP executes successfully.
After execution:
postgres=# select oid, subname from pg_subscription;
oid | subname
-----+---------
(0 rows)
postgres=# select * from pg_subscription_rel;
srsubid | srrelid | srsubstate | srsublsn
---------+---------+------------+----------
16389 | 16384 | i |
(1 row)
The subscription has been removed from pg_subscription, but entries
for that subscription remain in pg_subscription_rel. In other words,
DROP SUBSCRIPTION succeeds, yet orphaned rows are recreated in
pg_subscription_rel. I believe this is a bug.
The issue occurs because ALTER SUBSCRIPTION fetches the subscription
info before acquiring the subscription lock. If the subscription is
dropped in the meantime, ALTER SUBSCRIPTION continues using the stale
info. Although DROP SUBSCRIPTION removes the existing rows from
pg_subscription_rel, the still-running ALTER SUBSCRIPTION fetches the
publication table list from the publisher and recreates the
corresponding pg_subscription_rel entries in the 'init' state.
I believe the root cause is the same as the issue discussed in this
thread, so I'm reporting it here.
I tested this with the v5 patch series, and it resolves the issue.
Thanks,
Shlok Kyal
^ permalink raw reply [nested|flat] 35+ messages in thread
* Re: Re-read subscription state after lock in AlterSubscription
@ 2026-08-06 10:20 Shlok Kyal <shlok.kyal.oss@gmail.com>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
1 sibling, 0 replies; 35+ messages in thread
From: Shlok Kyal @ 2026-08-06 10:20 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Dilip Kumar <dilipbalaut@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Tue, 4 Aug 2026 at 11:25, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> 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.
0001 LGTM.
Some comments for 0002 patch:
+ /* DROP hook for the subscription being removed */
+ InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+
+ /*
+ * 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))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("subscription \"%s\" does not exist",
+ stmt->subname)));
+
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.
Thanks,
Shlok Kyal
^ permalink raw reply [nested|flat] 35+ messages in thread
end of thread, other threads:[~2026-08-06 10:20 UTC | newest]
Thread overview: 35+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-02 11:07 [PATCH v1] Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-02 12:08 Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-02 12:27 ` Dilip Kumar <dilipbalaut@gmail.com>
2026-07-02 12:48 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-07-02 13:20 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 03:13 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-07-03 04:19 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 04:50 ` Dilip Kumar <dilipbalaut@gmail.com>
2026-07-03 05:52 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 08:08 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-07-03 09:03 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 09:56 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-07-03 10:15 ` Amit Kapila <amit.kapila16@gmail.com>
2026-07-03 15:39 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-04 08:00 ` Dilip Kumar <dilipbalaut@gmail.com>
2026-07-04 08:19 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-06 02:43 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-07-06 05:01 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-06 04:54 ` Amit Kapila <amit.kapila16@gmail.com>
2026-07-06 05:43 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-06 09:37 ` Amit Kapila <amit.kapila16@gmail.com>
2026-07-06 14:53 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-08-04 05:55 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-08-05 11:28 ` Shlok Kyal <shlok.kyal.oss@gmail.com>
2026-08-06 10:20 ` Shlok Kyal <shlok.kyal.oss@gmail.com>
2026-07-03 05:17 [PATCH v2 1/2] Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 05:18 [PATCH v2 2/2] Re-read subscription state after lock in DropSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 11:54 [PATCH v5 2/4] Re-read subscription state after lock in DropSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 11:54 [PATCH v6 2/4] Re-read subscription state after lock in DropSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 11:54 [PATCH v3 2/4] Re-read subscription state after lock in DropSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 11:54 [PATCH v4 2/4] Re-read subscription state after lock in DropSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 12:28 [PATCH v6 1/4] Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 12:28 [PATCH v4 1/4] Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 12:28 [PATCH v5 1/4] Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2026-07-03 12:28 [PATCH v3 1/4] Re-read subscription state after lock in AlterSubscription Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox