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

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 ?

--
Regards,
Dilip Kumar
Google