On 6/22/17 03:26, Masahiko Sawada wrote:
> Thank you for the patch. Some comment and question.
> DropSubscriptionRelState requests ExclusiveLock but why is it not
> ShareRowExclusiveLock?
fixed
> I test DROP SUBSCIPTION case but even with this patch, "tuple
> concurrently updated" is still occurred.
>
> @@ -405,7 +405,7 @@ RemoveSubscriptionRel(Oid subid, Oid relid)
> }
> heap_endscan(scan);
>
> - heap_close(rel, RowExclusiveLock);
> + heap_close(rel, lockmode);
> }
>
> I think we should not release the table lock here, should be
> heap_close(rel, NoLock) instead? After changed it so on my
> environment, DROP SUBSCRIPTION seems to work fine.
fixed
> Also, ALTER SUBSCRIPTION SET PUBLICATION seems the same. Even with
> Petr's patch, the same error still occurred during ALTER SUBSCRIPTION
> SET PUBLICATION. Currently it acquires RowExclusiveLock on
> pg_subscription_rel but as long as both DDL and table sync worker
> could modify the same record on pg_subscription this error can
> happen.
fixed
> On the other hand if we take a strong lock on pg_subscription
> during DDL the deadlock risk will be increased.
The original problem was that DROP TABLE locked things in the order 1)
user table, 2) pg_subscription_rel, whereas a full-database VACUUM would
lock things in the opposite order. I think this was a pretty wide
window if you have many tables. In this case, however, we are only
dealing with pg_subscription and pg_subscription_rel, and I think even
VACUUM would always processes them in the same order.
Could please try the attached patch so see if it addresses your test
scenarios?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From fc50b9e287e8c3c29be21d82c07c39023cbf3882 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Fri, 23 Jun 2017 21:26:46 -0400
Subject: [PATCH v2] Parametrize locking in RemoveSubscriptionRel
Revising the change in 521fd4795e3ec3d0b263b62e5eb58e1557be9c86, we need
to take a stronger lock when removing entries from pg_subscription_rel
when ALTER/DROP SUBSCRIPTION. Otherwise, users might see "tuple
concurrently updated" when table sync workers make updates at the same
time as DDL commands are run. For DROP TABLE, we keep the lower lock
level that was put in place by the above commit, so that DROP TABLE does
not deadlock with whole-database VACUUM.
---
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 a376b99f1e..8a96bbece6 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..1d80191ca8 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, NoLock);
}
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 9cbd36f646..57954c978b 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,
ShareRowExclusiveLock);
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, ShareRowExclusiveLock);
/* 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 991ca9d552..a1f1db08d0 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
