Re: BUG #18988: DROP SUBSCRIPTION locks not-yet-accessed database

On Wed, Aug 13, 2025 at 11:16 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Thu, 7 Aug 2025 at 19:18, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Aug 6, 2025 at 11:38 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Aug 6, 2025 at 11:07 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, Aug 6, 2025 at 10:28 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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

Right, I was waiting for consensus for the fix, but anyway I will send
the patches for back branches, thanks.

--
Regards,
Dilip Kumar
Google