From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
To: Fujii Masao <masao.fujii@gmail.com>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Cannot shutdown subscriber after DROP SUBSCRIPTION
Date: Sat, 4 Feb 2017 21:11:20 +0100
Message-ID: <dd53e623-901c-3d64-0631-41b619596380@2ndquadrant.com> (raw)
In-Reply-To: <CAHGQGwHPi8ky-yANFfe0sgmhKtsYcQLTnKx07bW9S7-Rn1746w@mail.gmail.com>
References: <1289.1486012392@sss.pgh.pa.us>
<CAB7nPqSEnBZ0vbLs9dMdXgiHMeu_=wxeuAgRzstNkHwUJSu6aw@mail.gmail.com>
<CAHGQGwHqQVHmQ7wM=eLNnp1_oy-GVSSAcaJXWjE4nc2twSqXRg@mail.gmail.com>
<20170203.105635.240968059.horiguchi.kyotaro@lab.ntt.co.jp>
<CAHGQGwER_X8=aXacQJUgyQZjuEwQvhewEKsBAfv_uWVeN+abGA@mail.gmail.com>
<CAHGQGwHPi8ky-yANFfe0sgmhKtsYcQLTnKx07bW9S7-Rn1746w@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 03/02/17 19:38, Fujii Masao wrote:
> On Sat, Feb 4, 2017 at 12:49 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Fri, Feb 3, 2017 at 10:56 AM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>>> At Fri, 3 Feb 2017 01:02:47 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in <CAHGQGwHqQVHmQ7wM=eLNnp1_oy-GVSSAcaJXWjE4nc2twSqXRg@mail.gmail.com>
>>>> On Thu, Feb 2, 2017 at 2:36 PM, Michael Paquier
>>>> <michael.paquier@gmail.com> wrote:
>>>>> On Thu, Feb 2, 2017 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>>> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
>>>>>>> Then, the reason for the TRY-CATCH cluase is that I found that
>>>>>>> some functions called from there can throw exceptions.
>>>>>>
>>>>>> Yes, but all LWLocks should be released by normal error recovery.
>>>>>> It should not be necessary for this code to clean that up by hand.
>>>>>> If it were necessary, there would be TRY-CATCH around every single
>>>>>> LWLockAcquire in the backend, and we'd have an unreadable and
>>>>>> unmaintainable system. Please don't add a TRY-CATCH unless it's
>>>>>> *necessary* -- and you haven't explained why this one is.
>>>>
>>>> Yes.
>>>
>>> Thank you for the suggestion. I minunderstood that.
>>>
>>>>> Putting hands into the code and at the problem, I can see that
>>>>> dropping a subscription on a node makes it unresponsive in case of a
>>>>> stop. And that's just because calls to LWLockRelease are missing as in
>>>>> the patch attached. A try/catch problem should not be necessary.
>>>>
>>>> Thanks for the patch!
>>>>
>>>> With the patch, LogicalRepLauncherLock is released at the end of
>>>> DropSubscription(). But ISTM that the lock should be released just after
>>>> logicalrep_worker_stop() and there is no need to protect the removal of
>>>> replication slot with the lock.
>>>
>>> That's true. logicalrep_worker_stop returns after confirmig that
>>> worker->proc is cleard, so no false relaunch cannot be caused.
>>> After all, logicalrep_worker_stop is surrounded by
>>> LWLockAcquire/Relase pair. So it can be moved into the funciton
>>> and make the lock secrion to be more narrower.
>
> If we do this, Assert(LWLockHeldByMe(LogicalRepLauncherLock)) should be
> removed and the comment for logicalrep_worker_stop() should be updated.
>
> Your approach may cause the deadlock. The launcher takes LogicalRepWorkerLock
> while holding LogicalRepLauncherLock. OTOH, with your approach,
> logicalrep_worker_stop() takes LogicalRepLauncherLock while holding
> LogicalRepWorkerLock.
>
> Therefore I pushed the simple patch which adds LWLockRelease() just after
> logicalrep_worker_stop().
>
> Another problem that I found while reading the code is that the launcher can
> start up the worker with the subscription that DROP SUBSCRIPTION just removed.
> That is, DROP SUBSCRIPTION removes the target entry from pg_subscription,
> but the launcher can see it and start new worker until the transaction for
> DROP has been committed.
>
That was the reason why DropSubscription didn't release the lock in the
first place. It was supposed to be released at the end of the
transaction though.
> To fix this issue, I think that DROP SUBSCRIPTION should take
> AccessExclusiveLock on pg_subscription, instead of RowExclusiveLock,
> so that the launcher cannot see the entry to be being removed.
>
The whole point of having LogicalRepLauncherLock is to avoid having to
do this, so if we do this we could probably get rid of it.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: petr.jelinek@2ndquadrant.com, masao.fujii@gmail.com, horiguchi.kyotaro@lab.ntt.co.jp, michael.paquier@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: Cannot shutdown subscriber after DROP SUBSCRIPTION
In-Reply-To: <dd53e623-901c-3d64-0631-41b619596380@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox