On 6/19/17 22:54, Masahiko Sawada wrote:
>> It seems to me we could just take a stronger lock around
>> RemoveSubscriptionRel(), so that workers can't write in there concurrently.
> 
> Since we reduced the lock level of updating pg_subscription_rel by
> commit 521fd4795e3e the same deadlock issue will appear if we just
> take a stronger lock level.

I was thinking about a more refined approach, like in the attached
patch.  It just changes the locking when in DropSubscription(), so that
that doesn't fail if workers are doing stuff concurrently.  Everything
else stays the same.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From c101b6da52dd4f9f041390a937b337f20d212e5c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 20 Jun 2017 19:06:42 -0400
Subject: [PATCH] WIP Parametrize locking in RemoveSubscriptionRel

---
 src/backend/catalog/heap.c                | 2 +-
 src/backend/catalog/pg_subscription.c     | 6 +++---
 src/backend/commands/subscriptioncmds.c   | 4 ++--
 src/include/catalog/pg_subscription_rel.h | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4e5b79ef94..643e4bb1d5 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1844,7 +1844,7 @@ heap_drop_with_catalog(Oid relid)
        /*
         * Remove any associated relation synchronization states.
         */
-       RemoveSubscriptionRel(InvalidOid, relid);
+       RemoveSubscriptionRel(InvalidOid, relid, RowExclusiveLock);
 
        /*
         * Forget any ON COMMIT action for the rel
diff --git a/src/backend/catalog/pg_subscription.c 
b/src/backend/catalog/pg_subscription.c
index c69c461b62..88b81920c8 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -369,7 +369,7 @@ GetSubscriptionRelState(Oid subid, Oid relid, XLogRecPtr 
*sublsn,
  * subscription, or for a particular relation, or both.
  */
 void
-RemoveSubscriptionRel(Oid subid, Oid relid)
+RemoveSubscriptionRel(Oid subid, Oid relid, LOCKMODE lockmode)
 {
        Relation        rel;
        HeapScanDesc scan;
@@ -377,7 +377,7 @@ RemoveSubscriptionRel(Oid subid, Oid relid)
        HeapTuple       tup;
        int                     nkeys = 0;
 
-       rel = heap_open(SubscriptionRelRelationId, RowExclusiveLock);
+       rel = heap_open(SubscriptionRelRelationId, lockmode);
 
        if (OidIsValid(subid))
        {
@@ -405,7 +405,7 @@ RemoveSubscriptionRel(Oid subid, Oid relid)
        }
        heap_endscan(scan);
 
-       heap_close(rel, RowExclusiveLock);
+       heap_close(rel, lockmode);
 }
 
 
diff --git a/src/backend/commands/subscriptioncmds.c 
b/src/backend/commands/subscriptioncmds.c
index 5aae7b6f91..02cb7c47b8 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -595,7 +595,7 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
                {
                        char       *namespace;
 
-                       RemoveSubscriptionRel(sub->oid, relid);
+                       RemoveSubscriptionRel(sub->oid, relid, 
RowExclusiveLock);
 
                        logicalrep_worker_stop(sub->oid, relid);
 
@@ -910,7 +910,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool 
isTopLevel)
        deleteSharedDependencyRecordsFor(SubscriptionRelationId, subid, 0);
 
        /* Remove any associated relation synchronization states. */
-       RemoveSubscriptionRel(subid, InvalidOid);
+       RemoveSubscriptionRel(subid, InvalidOid, ExclusiveLock);
 
        /* Kill the apply worker so that the slot becomes accessible. */
        logicalrep_worker_stop(subid, InvalidOid);
diff --git a/src/include/catalog/pg_subscription_rel.h 
b/src/include/catalog/pg_subscription_rel.h
index f5f6191676..b2cadb4b13 100644
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -74,7 +74,7 @@ extern Oid SetSubscriptionRelState(Oid subid, Oid relid, char 
state,
                                                XLogRecPtr sublsn, bool 
update_only);
 extern char GetSubscriptionRelState(Oid subid, Oid relid,
                                                XLogRecPtr *sublsn, bool 
missing_ok);
-extern void RemoveSubscriptionRel(Oid subid, Oid relid);
+extern void RemoveSubscriptionRel(Oid subid, Oid relid, LOCKMODE lockmode);
 
 extern List *GetSubscriptionRelations(Oid subid);
 extern List *GetSubscriptionNotReadyRelations(Oid subid);
-- 
2.13.1

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to