On Sat, Apr 01, 2017 at 02:25:54AM +0200, Petr Jelinek wrote: > 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?
[Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
