Re: 024_add_drop_pub.pl might fail due to deadlock

On Tue, 29 Jul 2025 at 14:46, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Jul 29, 2025 at 1:13 PM Amit Kapila <amit.kapila16@gmail.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