Re: Re-read subscription state after lock in AlterSubscription

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(dot)pg(at)gmail(dot)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