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

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()

--
Regards,
Dilip Kumar
Google