On Fri, Jul 3, 2026 at 1:38 PM Zhijie Hou (Fujitsu) <[email protected]> wrote: > > On Friday, July 3, 2026 1:53 PM Bertrand Drouvot > <[email protected]> wrote: > > > > > but given the patch's simplicity, I recommend backpatching. > > > > That's right but that would only improve error messages. That said, looking > > closer, they are elog() ones, so "not expected" to occur so yeah backpatch > > does make sense. > > +1 for backpatching, even if it's rare, the "ERROR: tuple concurrently > updated" > message seems confusing to me. >
I also think backpatching makes sense. BTW, I have a comment: + heap_freetuple(tup); + tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, ObjectIdGetDatum(MyDatabaseId), + CStringGetDatum(stmt->subname)); heap_freetuple() could be done before acquiring the lock, is there a reason to keep it after lock? > > > > That said, what about also fixing DropSubscription() like in the 0002 > > attached? > > (that would also produce those elog() messages in case of concurrent DROP or > > ALTER). > > For the patch, I'm not sure if we must repeat the checks twice. Could we > simply move the original checks to after we take the lock? At least, the > GetSubscription() call and the password check can be moved there and old codes > can be deleted. > Isn't the same true for the AlterSubscription() case as well? Also, I noticed that AlterPublication() does the same trick but it uses PUBLICATIONOID cacheid, so shouldn't we use SUBSCRIPTIONOID cacheid here as well? I think this is to prevent the case where the same name pub/sub is recreated after lock. -- With Regards, Amit Kapila.
