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

On Tue, Aug 5, 2025 at 11:52 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Aug 4, 2025 at 7:08 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Aug 4, 2025 at 6:20 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > >
> > > On 2025-Aug-04, Dilip Kumar wrote:
> > >
> > > > I have worked on this and produced a first version of patch, let's see
> > > > what others think about this idea.  It would have been better if we
> > > > could use SysCache for rechecking the subscription, but since we are
> > > > not connected to the database in the launcher we can not use the
> > > > SysCache, at least that's what I think.
> > >
> > > I think it's reasonable to recheck after locking.  There's a comment in
> > > DropSubscription that says we get AEL, which is no longer true.
> >
> > Right, will remove that.
> >
> >   In
> > > is_subscription_exists() you should use the index on OID instead of
> > > seqscanning the catalog without a scankey;
> >
> > I thought since launcher is not connected to the database we will not
> > be able to open the index relation.  Otherwise we may just call
> > SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));  Maybe this
> > is not connected because it was not required so far and we can just
> > connect it to template1 ?
> >
> >
> >  also I think the name ought
> > > to be "does" rather than "is".
> >
> > Okay
> >
> >   I think it's really odd that that
> > > function opens and closes a transaction; sounds to me that something
> > > like that really belongs in the caller (frankly the same is true with
> > > the other function that your comment references).  Why isn't
> > > systable_beginscan being used to scan the catalog?
> >
> > You mean for this function or for get_subscription_list() as well,
> > yeah logically systable_beginscan() sounds better.
> >
> > > I think with this coding, the resource owner for this new lock is NULL.
> > > Is this really a good approach?  Maybe there should be a resowner here.
> >
> > As you suggested we should move the transaction to the caller and
> > start it before LockSharedObject() so that we will acquire the lock
> > under the TopTransactionResourceOwner ?
>
> Here is revised version based on what I proposed here
>
> - I have removed the comment in DropSubscription where we acquire the
> lock, as mentioning the ASL is not interesting anymore, instead I am
> explaining in launcher why we are acquiring shared object lock.
> - Connected launcher to "postgres" database so that we can do syscache lookup
>

Won't that add a new restriction that one can't drop postgres database
after this?

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.

-- 
With Regards,
Amit Kapila.