Fix a deadlock during ALTER SUBSCRIPTION ... DROP PUBLICATION.

A deadlock can occur when the DDL command and the apply worker acquire
catalog locks in different orders while dropping replication origins.

The issue is rare in PG16 and higher branches because, in most cases, the
tablesync worker performs the origin drop in those branches, and its
locking sequence does not conflict with DDL operations.

This patch ensures consistent lock acquisition to prevent such deadlocks.

As per buildfarm.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Ajin Cherian <itsajin@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: vignesh C <vignesh21@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/bab95e12-6cc5-4ebb-80a8-3e41956aa297@gmail.com

diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index add51caadf2a1745b125d55001f5f1aba2d31db5..a520109ce5a8c86651a788add0fc83c77ea550b4 100644 (file)
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -323,8 +323,8 @@ AddSubscriptionRelState(Oid subid, Oid relid, char state,
  * Update the state of a subscription table.
  */
 void
-UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
-                                                  XLogRecPtr sublsn)
+UpdateSubscriptionRelStateEx(Oid subid, Oid relid, char state,
+                                                        XLogRecPtr sublsn, bool already_locked)
 {
        Relation        rel;
        HeapTuple       tup;
@@ -332,9 +332,24 @@ UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
        Datum           values[Natts_pg_subscription_rel];
        bool            replaces[Natts_pg_subscription_rel];
 
-       LockSharedObject(SubscriptionRelationId, subid, 0, AccessShareLock);
+       if (already_locked)
+       {
+#ifdef USE_ASSERT_CHECKING
+               LOCKTAG         tag;
 
-       rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);
+               Assert(CheckRelationOidLockedByMe(SubscriptionRelRelationId,
+                                                                                 RowExclusiveLock, true));
+               SET_LOCKTAG_OBJECT(tag, InvalidOid, SubscriptionRelationId, subid, 0);
+               Assert(LockHeldByMe(&tag, AccessShareLock));
+#endif
+
+               rel = table_open(SubscriptionRelRelationId, NoLock);
+       }
+       else
+       {
+               LockSharedObject(SubscriptionRelationId, subid, 0, AccessShareLock);
+               rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);
+       }
 
        /* Try finding existing mapping. */
        tup = SearchSysCacheCopy2(SUBSCRIPTIONRELMAP,
@@ -368,6 +383,16 @@ UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
        table_close(rel, NoLock);
 }
 
+/*
+ * Update the state of a subscription table.
+ */
+void
+UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
+                                                  XLogRecPtr sublsn)
+{
+       UpdateSubscriptionRelStateEx(subid, relid, state, sublsn, false);
+}
+
 /*
  * Get state of subscription table.
  *

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index e6159acba0091b653e05c0d634c52d2545b9b779..dc0526e2da25cd42cf830a2d937cac6d5f064c6a 100644 (file)
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -379,6 +379,7 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
        static HTAB *last_start_times = NULL;
        ListCell   *lc;
        bool            started_tx = false;
+       Relation        rel = NULL;
 
        Assert(!IsTransactionState());
 
@@ -470,7 +471,16 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
                                 * refresh for the subscription where we remove the table
                                 * state and its origin and by this time the origin might be
                                 * already removed. 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);
+
                                ReplicationOriginNameForTablesync(MyLogicalRepWorker->subid,
                                                                                                  rstate->relid,
                                                                                                  originname,
@@ -480,9 +490,9 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
                                /*
                                 * Update the state to READY only after the origin cleanup.
                                 */
-                               UpdateSubscriptionRelState(MyLogicalRepWorker->subid,
-                                                                                  rstate->relid, rstate->state,
-                                                                                  rstate->lsn);
+                               UpdateSubscriptionRelStateEx(MyLogicalRepWorker->subid,
+                                                                                        rstate->relid, rstate->state,
+                                                                                        rstate->lsn, true);
                        }
                }
                else
@@ -533,7 +543,14 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
                                                 * This is required to avoid any undetected deadlocks
                                                 * due to any existing lock as deadlock detector won't
                                                 * be able to detect the waits on the latch.
+                                                *
+                                                * Also close any tables prior to the commit.
                                                 */
+                                               if (rel)
+                                               {
+                                                       table_close(rel, NoLock);
+                                                       rel = NULL;
+                                               }
                                                CommitTransactionCommand();
                                                pgstat_report_stat(false);
                                        }
@@ -593,6 +610,10 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
                }
        }
 
+       /* Close table if opened */
+       if (rel)
+               table_close(rel, NoLock);
+
        if (started_tx)
        {
                CommitTransactionCommand();

diff --git a/src/include/catalog/pg_subscription_rel.h b/src/include/catalog/pg_subscription_rel.h
index 9df99c3418106ad35592331641a5eba18d7149a8..a2e510511e86ac77ea986f4df998ce6b05451430 100644 (file)
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -84,6 +84,8 @@ extern void AddSubscriptionRelState(Oid subid, Oid relid, char state,
                                                                        XLogRecPtr sublsn);
 extern void UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
                                                                           XLogRecPtr sublsn);
+extern void UpdateSubscriptionRelStateEx(Oid subid, Oid relid, char state,
+                                                                                XLogRecPtr sublsn, bool already_locked);
 extern char GetSubscriptionRelState(Oid subid, Oid relid, XLogRecPtr *sublsn);
 extern void RemoveSubscriptionRel(Oid subid, Oid relid);