Re: Re-read subscription state after lock in AlterSubscription

Hi,

On Thu, Aug 06, 2026 at 03:50:09PM +0530, Shlok Kyal wrote:
> On Tue, 4 Aug 2026 at 11:25, Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 06, 2026 at 02:53:39PM +0000, Bertrand Drouvot wrote:
> > > Hi,
> > >
> > > On Mon, Jul 06, 2026 at 03:07:24PM +0530, Amit Kapila wrote:
> > >
> > > 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).
> >
> > Mandatory rebase attached.
> >
> Hi, I reviewed 0001 and 0002 patches.

Thanks!

> 0001 LGTM.
>
> Some comments for 0002 patch:
>
> 1. Here if 'tup' is invalid we are not checking the 'stmt->missing_ok' flag.
> Should we only throw an error if 'stmt->missing' is false?
> Otherwise 'DROP SUBSCRIPTION IF EXISTS' can throw an error like:
>
> postgres=# DROP SUBSCRIPTION IF EXISTS sub1;
> ERROR: subscription "sub1" does not exist
>
> 2. Should the function 'InvokeObjectDropHook' be called after the
> check in 'if (!HeapTupleIsValid(tup))'?
> If 'tup' is not valid, an error is thrown, and in this case, calling
> the function 'InvokeObjectDropHook' is unnecessary.

Yeah, both comments would be addressed by 0003, but 0002 should be correct on
its own, so fixed in the attached.

Also, while at it, I changed my mind about the post-lock owner recheck. I think
reporting "must be owner of subscription" is better than "tuple concurrently
updated", so 0001 and 0002 now perform this recheck and add some basic isolation
tests for it.

Those explicit rechecks become redundant once 0003 is applied (and 0004 does
the same for publications), but they make 0001 and 0002 behave better on their
own.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com