Date: Mon, 6 Jul 2026 05:43:28 +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: <aktAgBZqJLEvoyZD@bdtpg>
References: 
 <TYRPR01MB121568CC10A0920C7A5ADCCC2F5F52@TYRPR01MB12156.jpnprd01.prod.outlook.com>
 <akZlreb754FkCFrv@bdtpg>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAA4eK1++SdeMS2vqMMt+nSBC0CQ-K+0ESkdDdkH4SYbrxmxWLw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aktAgBZqJLEvoyZD%40bdtpg>
Precedence: bulk

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