Re: Subscription code improvements

On Sat, Jul 8, 2017 at 5:19 AM, Petr Jelinek
<petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> I have done some review of subscription handling (well self-review) and
> here is the result of that (It's slightly improved version from another
> thread [1]).

Thank you for working on this and patches!

> I split it into several patches:
>
> 0001 - Makes subscription worker exit nicely when there is subscription
> missing (ie was removed while it was starting) and also makes disabled
> message look same as the message when disabled state was detected while
> worker is running. This is mostly just nicer behavior for user (ie no
> unexpected errors in log when you just disabled the subscription).
>
> 0002 - This is bugfix - the sync worker should exit when waiting for
> apply and apply dies otherwise there is possibility of not being
> correctly synchronized.
>
> 0003 - Splits the schizophrenic SetSubscriptionRelState function into
> two which don't try to do broken upsert and report proper errors instead.
>
> 0004 - Solves the issue which Masahiko Sawada reported [2] about ALTER
> SUBSCRIPTION handling of workers not being transactional - we now do
> killing of workers transactionally (and we do the same when possible in
> DROP SUBSCRIPTION).
>
> 0005 - Bugfix to allow syscache access to subscription without being
> connected to database - this should work since subscription is pinned
> catalog, it wasn't caught because nothing in core is using it (yet).
>
> 0006 - Makes the locking of subscriptions more granular (this depends on
> 0005). This removes the ugly AccessExclusiveLock on the pg_subscription
> catalog from DROP SUBSCRIPTION and also solves some potential race
> conditions between launcher, ALTER SUBSCRIPTION and
> process_syncing_tables_for_apply(). Also simplifies memory handling in
> launcher as well as logicalrep_worker_stop() function. This basically
> makes subscriptions behave like every other object in the database in
> terms of locking.
>
> Only the 0002, 0004 and 0005 are actual bug fixes, but I'd still like to
> get it all into PG10 as especially the locking now behaves really
> differently than everything else and that does not seem like a good idea.
>

I'm now planing to review 0002, 0004 and 0005 patches first as they
are bug fixes. Should we add them to the open item list?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center