Re: 024_add_drop_pub.pl might fail due to deadlock

On Tue, 29 Jul 2025 at 14:46, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Tue, Jul 29, 2025 at 1:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > Yes, that makes sense to me. For HEAD and PG18, we can still add a new
> > argument to the API. For other bank branches, it is better to use a
> > new Ex function as suggested by Kuroda-San.
> >
>
> Here are the updated patches.

I noticed the order of LockSharedObject and table lock is different
here compared to disable subscription:
@@ -492,7 +494,17 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
* worker to remove the origin
tracking as if there is any
* error while dropping we won't
restart it to drop the
* origin. So passing missing_ok = true.
+ *
+ * Lock the subscription and origin in
the same order as we
+ * are doing during DDL commands to
avoid deadlocks. See
+ * AlterSubscription_refresh.
*/
+
LockSharedObject(SubscriptionRelationId, MyLogicalRepWorker->subid,
+ 0,
AccessShareLock);
+
+ if (!rel)
+ rel =
table_open(SubscriptionRelRelationId, RowExclusiveLock);

DisableSubscription(Oid subid)
{
....
rel = table_open(SubscriptionRelationId, RowExclusiveLock);
tup = SearchSysCacheCopy1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));

if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for subscription %u", subid);

LockSharedObject(SubscriptionRelationId, subid, 0, AccessShareLock);
....

Do we need to enforce consistent lock ordering here, or is it safe to
ignore because we're only using AccessShareLock?

Regards,
Vignesh