On Wed, Jun 21, 2017 at 8:10 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> 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.
>
Thank you for the patch. Some comment and question.
DropSubscriptionRelState requests ExclusiveLock but why is it not
ShareRowExclusiveLock?
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.
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. On the other hand if we take a strong lock on pg_subscription
during DDL the deadlock risk will be increased.
One solution I came up with is that we use other than
simple_heap_update/delete so that we accept the return value
HeapTupleUpdated of heap_update/delete. It makes pg_subscription_rel
something like heap table rather than system catalog, though.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
