Re: Re-read subscription state after lock in AlterSubscription

On Mon, Jul 6, 2026 at 11:13 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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(dot)pg(at)gmail(dot)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.