On Sun, Jun 25, 2017 at 7:35 PM, Petr Jelinek
<petr.jelinek@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
