On 01/04/17 01:57, Petr Jelinek wrote:
> On 01/04/17 01:20, Tom Lane wrote:
>> Petr Jelinek <petr.jeli...@2ndquadrant.com> writes:
>>> But the pg_subscription_rel is also not accessed on heap_open, the
>>> problematic code is called from heap_drop_with_catalog. And VACUUM FULL
>>> pg_class calls heap_drop_with_catalog() when doing the heap swap (and it
>>> goes through performDeletion so through dependency info which is what I
>>> mean by everything else does this).
>>
>> Hmmm. What the heap_drop_with_catalog call is being applied to is a
>> short-lived table that is not pg_class. It happens to own the disk
>> storage that used to belong to pg_class, but it is not pg_class;
>> in particular it doesn't have the same OID, and it is not what would
>> be looked in if someone happened to need to fetch a pg_class row
>> at that point. So there's no circularity involved.
>>
>> On further reflection it seems like you were right, the problem is
>> taking a self-exclusive lock on pg_subscription_rel during low-level
>> catalog operations. We're going to have to find a way to reduce that
>> lock strength, or we're going to have a lot of deadlock problems.
>>
>
> Well we have heavy lock because we were worried about failure scenarios
> in our dumb upsert in SetSubscriptionRelState which does cache lookup
> and if it finds something it updates, otherwise inserts. We have similar
> pattern in other places but they are called from DDL statements usually
> so the worst that can happen is DDL fails with weird errors when done
> concurrently with same name so using RowExclusiveLock is okay.
>
> That being said, looking at use-cases for SetSubscriptionRelState that's
> basically CREATE SUBSCRIPTION, ALTER SUBSCRIPTION REFRESH and tablesync
> worker. So the DDL thing applies to first ones as well and tablesync
> should not be running in case the record does not exist so it's fine if
> it fails. In terms of RemoveSubscriptionRel that's only called from
> heap_drop_with_catalog and tablesync holds relation lock so there is no
> way heap_drop_with_catalog will happen on the same relation. This leads
> me to thinking that RowExclusiveLock is fine for both
> SetSubscriptionRelState and RemoveSubscriptionRel as long as we document
> that callers should be aware that SetSubscriptionRelState has
> concurrency issues and fail on unique index check.
>
And a simple patch to do so. Peter do you see any problem with doing this?
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From a12259efe7b4d0af570e7d2d7cb48b249158a1bd Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Sat, 1 Apr 2017 02:20:16 +0200
Subject: [PATCH] Use weaker locks when updating subscription relation state
---
src/backend/catalog/pg_subscription.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index e7a1634..b0bd248 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -215,6 +215,9 @@ textarray_to_stringlist(ArrayType *textarray)
/*
* Set the state of a subscription table.
+ *
+ * The insert or update logic in this function is not concurrency safe so
+ * it may raise an error.
*/
Oid
SetSubscriptionRelState(Oid subid, Oid relid, char state,
@@ -227,7 +230,7 @@ SetSubscriptionRelState(Oid subid, Oid relid, char state,
Datum values[Natts_pg_subscription_rel];
/* Prevent concurrent changes. */
- rel = heap_open(SubscriptionRelRelationId, ShareRowExclusiveLock);
+ rel = heap_open(SubscriptionRelRelationId, RowExclusiveLock);
/* Try finding existing mapping. */
tup = SearchSysCacheCopy2(SUBSCRIPTIONRELMAP,
@@ -358,8 +361,7 @@ RemoveSubscriptionRel(Oid subid, Oid relid)
HeapTuple tup;
int nkeys = 0;
- /* Prevent concurrent changes (see SetSubscriptionRelState()). */
- rel = heap_open(SubscriptionRelRelationId, ShareRowExclusiveLock);
+ rel = heap_open(SubscriptionRelRelationId, RowExclusiveLock);
if (OidIsValid(subid))
{
@@ -387,7 +389,7 @@ RemoveSubscriptionRel(Oid subid, Oid relid)
}
heap_endscan(scan);
- heap_close(rel, ShareRowExclusiveLock);
+ heap_close(rel, RowExclusiveLock);
}
--
2.7.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
