Re: BUG #18988: DROP SUBSCRIPTION locks not-yet-accessed database - Mailing list pgsql-bugs

On Thu, 7 Aug 2025 at 19:18, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Aug 6, 2025 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Aug 6, 2025 at 11:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Wed, Aug 6, 2025 at 10:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > Won't that add a new restriction that one can't drop postgres database
> > > > after this?
> > >
> > > That's correct, so maybe this is not acceptable IMHO.
> > >
> > > > BTW, isn't it better to acquire the share_lock on subscription during
> > > > worker initialization (InitializeLogRepWorker()) where we already
> > > > checking whether the subscription is dropped by that time? I think if
> > > > we don't acquire the lock on subscription during launcher or during
> > > > InitializeLogRepWorker(), there is a risk that DropSubscription would
> > > > have reached the place where it would have tried to stop the workers
> > > > and launcher will launch the worker after that point. Then there is a
> > > > possibility of dangling worker because the GetSubscription() can still
> > > > return valid value as the transaction in which we are dropping a
> > > > subscription could still be in-progress.
> > >
> > > Here is my thought on these 2 approaches, so if we lock in launcher
> > > and check there we avoid launching the worker altogether but for that
> > > we will have to revalidate the subscription using sequence scan on
> > > pg_subcription as we can not open additional indexes as we are not
> > > connected to any database OTOH if we acquire lock in
> > > InitializeLogRepWorker() we don't need to do any additional scan but
> > > we will have to launch the worker and after that if subscription
> > > recheck shows its dropped we will exit the worker.
> > >
> > > I think either way it's fine because this is not a very common
> > > performance path, I prefer what you proposed as we will have to add
> > > less code in this case, because we are already checking the
> > > subscription and just need to acquire the shared object lock in
> > > InitializeLogRepWorker(), lets see what other thinks, meanwhile I will
> > > modify the code with this approach as well.
> >
> > Here is a patch with a different approach.  IMHO with this approach
> > the code change is much simpler.
>
> I have slightly modified the patch, basically removing explicitly
> unlocking the shared object as that will be taken care by transaction
> commit / proc_exit()

Thanks for the updated patch. This change is required for the back
branches too. Currently it is not applying for PG16 and below
branches.
git am v3-0001-Fix-DROP-SUBSCRIPTION-deadlock-with-new-database-.patch
Applying: Fix DROP SUBSCRIPTION deadlock with new database creation
error: patch failed: src/backend/commands/subscriptioncmds.c:1571
error: src/backend/commands/subscriptioncmds.c: patch does not apply
error: patch failed: src/backend/replication/logical/worker.c:4624
error: src/backend/replication/logical/worker.c: patch does not apply
Patch failed at 0001 Fix DROP SUBSCRIPTION deadlock with new database creation

Regards,
Vignesh