Date: Mon, 6 Jul 2026 14:53:39 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
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>
Subject: Re: Re-read subscription state after lock in AlterSubscription
Message-ID: <akvBczTSnfia0fEP@bdtpg>
References: 
 <OS9PR01MB12149E944D7ECABF0C1DDCCB4F5F42@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <akc4NBxbNKeSvumI@bdtpg>
 <CAFiTN-tBC0kvymhKiq9hXdr-Cdsdezyyh-X5FCtzfBsAORGOaw@mail.gmail.com>
 <akdOLGFblqA3Yvd6@bdtpg>
 <TY4PR01MB17718E4AA1A80B59B1B6FBF2D94F42@TY4PR01MB17718.jpnprd01.prod.outlook.com>
 <CAA4eK1+OU57xnjQXC-z6qMT+bdPsHMzQVXLPMdBmhhq++zwu+g@mail.gmail.com>
 <akfXqSGeRpL+2Ypz@bdtpg>
 <CAA4eK1++SdeMS2vqMMt+nSBC0CQ-K+0ESkdDdkH4SYbrxmxWLw@mail.gmail.com>
 <aktAgBZqJLEvoyZD@bdtpg>
 <CAA4eK1KKtkHr4S7uE=VLonXAwO4YOsZzFUQefmvbNkCBAe_chg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAA4eK1KKtkHr4S7uE=VLonXAwO4YOsZzFUQefmvbNkCBAe_chg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/akvBczTSnfia0fEP%40bdtpg>
Precedence: bulk

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