agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: Re-read subscription state after lock in AlterSubscription
Date: Mon, 6 Jul 2026 14:53:39 +0000
Message-ID: <akvBczTSnfia0fEP@bdtpg> (raw)
In-Reply-To: <CAA4eK1KKtkHr4S7uE=VLonXAwO4YOsZzFUQefmvbNkCBAe_chg@mail.gmail.com>
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>
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
view thread (35+ messages) latest in thread
Message-ID: <akvBczTSnfia0fEP@bdtpg>
Permalink: ../akvBczTSnfia0fEP@bdtpg/
Also on: postgresql.org/message-id/akvBczTSnfia0fEP@bdtpg
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bertranddrouvot.pg@gmail.com, amit.kapila16@gmail.com, houzj.fnst@fujitsu.com, dilipbalaut@gmail.com, kuroda.hayato@fujitsu.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Re-read subscription state after lock in AlterSubscription
In-Reply-To: <akvBczTSnfia0fEP@bdtpg>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox