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
