On Sun, Jun 25, 2017 at 7:35 PM, Petr Jelinek <petr.jeli...@2ndquadrant.com> wrote: > (was away for a while, got some time now for this again) > > On 22/06/17 04:43, Peter Eisentraut wrote: >> The alternative is that we use the LockSharedObject() approach that was >> already alluded to, like in the attached patch. Both approaches would >> work equally fine AFAICT. > > I agree, but I think we need bigger overhaul of the locking/management > in general. So here is patch which does much more changes. > > The patch does several important things (in no particular order): > - Split SetSubscriptionRelState into AddSubscriptionRelState and > UpdateSubscriptionRelState for the reasons said upstream, it's cleaner, > there is no half-broken upsert logic and it has proper error checking > for each action. > > - Do LockSharedObject in ALTER SUBSCRIPTION, DROP SUBSCRIPTION (this one > is preexisting but mentioning it for context), SetSubscriptionRelState, > AddSubscriptionRelState, and in the logicalrep_worker_launch. This means > we use granular per object locks to deal with concurrency. > > - Because of above, the AccessExclusiveLock on pg_subscription is no > longer needed, just normal RowExlusiveLock is used now. > > - logicalrep_worker_stop is also simplified due to the proper locking > > - There is new interface logicalrep_worker_stop_at_commit which is used > by ALTER SUBSCRIPTION ... REFRESH PUBLICATION and by transactional > variant of DROP SUBSCRIPTION to only kill workers at the end of transaction. > > - Locking/reading of subscription info is unified between DROP and ALTER > SUBSCRIPTION commands. > > - DROP SUBSCRIPTION will kill all workers associated with subscription, > not just apply. > > - The sync worker checks during startup if the relation is still subscribed. > > - The sync worker will exit when waiting for apply and apply has shut-down. > > - The log messages around workers and removed or disabled subscription > are now more consistent between startup and normal runtime of the worker. > > - Some code deduplication and stylistic changes/simplification in > related areas. > > - Fixed catcache's IndexScanOK() handling of the subscription catalog. > > It's bit bigger patch but solves issues from multiple threads around > handling of ALTER/DROP subscription. > > A lot of the locking that I added is normally done transparently by > dependency handling, but subscriptions and subscription relation status > do not use that much as it was deemed to bloat pg_depend needlessly > during the original patch review (it's also probably why this has > slipped through). >
Thank you for reworking on this! I'll review this patch on Tuesday. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
