Horton

Thread

Commits

  1. Prevent logical rep workers with removed subscriptions from starting.

  1. Cannot shutdown subscriber after DROP SUBSCRIPTION

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2017-02-01T08:36:23Z

    Hello, while looking another bug, I found that standby cannot
shutdown after DROP SUBSCRIPTION.

standby=# CREATE SUBSCRPTION sub1 ...
standby=# ....
standby=# DROP SUBSCRIPTION sub1;

Ctrl-C to the standby fails to work. ApplyLauncherMain is waiting
LogicalRepLauncherLock forever.

The culprit is DropSbuscription. It acquires
LogicalRepLauncherLock but never releases.

The attached patch fixes it. Most part of the fucntion is now
enclosed by PG_TRY-CATCH since some functions can throw
exceptions.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-02-01T17:14:21Z

    On Wed, Feb 1, 2017 at 5:36 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello, while looking another bug, I found that standby cannot
> shutdown after DROP SUBSCRIPTION.
>
> standby=# CREATE SUBSCRPTION sub1 ...
> standby=# ....
> standby=# DROP SUBSCRIPTION sub1;
>
> Ctrl-C to the standby fails to work. ApplyLauncherMain is waiting
> LogicalRepLauncherLock forever.
>
> The culprit is DropSbuscription. It acquires
> LogicalRepLauncherLock but never releases.
>
> The attached patch fixes it. Most part of the fucntion is now
> enclosed by PG_TRY-CATCH since some functions can throw
> exceptions.

The lwlock would be released when an exception occurs, so I don't think
that TRY-CATCH is necessary here. Or it's necessary for another reason?

Regards,

-- 
Fujii Masao

  3. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-01T23:46:11Z

    On Thu, Feb 2, 2017 at 2:14 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> The lwlock would be released when an exception occurs, so I don't think
> that TRY-CATCH is necessary here. Or it's necessary for another reason?

+    PG_CATCH();
+    {
+        LWLockRelease(LogicalRepLauncherLock);
+        PG_RE_THROW();
+    }
+    PG_END_TRY();
Just to do that, a TRY/CATCH block looks like an overkill to me. Why
not just call LWLockRelease in the ERROR and return code paths?
-- 
Michael

  4. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2017-02-02T03:02:15Z

    At Thu, 2 Feb 2017 08:46:11 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in <CAB7nPqR6VQ7aiKck1Ao3_mPVvn4v4ZKnJFq2oawFqpaePHd18A@mail.gmail.com>
> On Thu, Feb 2, 2017 at 2:14 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > The lwlock would be released when an exception occurs, so I don't think
> > that TRY-CATCH is necessary here. Or it's necessary for another reason?
> 
> +    PG_CATCH();
> +    {
> +        LWLockRelease(LogicalRepLauncherLock);
> +        PG_RE_THROW();
> +    }
> +    PG_END_TRY();
> Just to do that, a TRY/CATCH block looks like an overkill to me. Why
> not just call LWLockRelease in the ERROR and return code paths?

I though the same first. The modification at the "if (wrconn =="
is the remains of that. It is reverted inthe attached patch.

Then, the reason for the TRY-CATCH cluase is that I found that
some functions called from there can throw exceptions.

logicalrep_worker_stop and replorigin_drop have ereport in its path.
load_library apparently can throw exception.
(walrcv_(libpq_) functions don't seeem to.)

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-02T05:13:12Z

    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.

			regards, tom lane

  6. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-02T05:36:52Z

    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.

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.
-- 
Michael

  7. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-02-02T16:02:47Z

    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.

> 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.

    /*
    * If we found worker but it does not have proc set it is starting up,
    * wait for it to finish and then kill it.
    */
    while (worker && !worker->proc)
    {

ISTM that the above loop in logicalrep_worker_stop() is not necessary
because LogicalRepLauncherLock ensures that the above condition is
always false. Thought? Am I missing something?

If the above condition is true, which means that there is the worker slot
having the "subid" of the worker to kill, but its "proc" has not been set yet.
Without LogicalRepLauncherLock, this situation can happen after "subid"
is set by the launcher and before "proc" is set by the worker. But
LogicalRepLauncherLock protects those operations, so logicalrep_worker_stop()
called while holding the lock should always think the above condition is false.

Regards,

-- 
Fujii Masao

  8. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2017-02-03T01:56:35Z

    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 found worker but it does not have proc set it is starting up,
>     * wait for it to finish and then kill it.
>     */
>     while (worker && !worker->proc)
>     {
> 
> ISTM that the above loop in logicalrep_worker_stop() is not necessary
> because LogicalRepLauncherLock ensures that the above condition is
> always false. Thought? Am I missing something?

The lock exists only to keep the launcher from starting a
worker. Creating a subscription and starting a worker for the
slot run independently.

> If the above condition is true, which means that there is the worker slot
> having the "subid" of the worker to kill, but its "proc" has not been set yet.

Yes. The situation happens after launcher sets subid and before
ApplyWorkerMain attaches the slot.  The lock doesn't protect the
section. If someone can drop a subscription just after its
creation, it happens.

> Without LogicalRepLauncherLock, this situation can happen after "subid"
> is set by the launcher and before "proc" is set by the worker. But
> LogicalRepLauncherLock protects those operations, so logicalrep_worker_stop()
> called while holding the lock should always think the above condition is false.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  9. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-02-03T15:49:31Z

    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 found worker but it does not have proc set it is starting up,
>>     * wait for it to finish and then kill it.
>>     */
>>     while (worker && !worker->proc)
>>     {
>>
>> ISTM that the above loop in logicalrep_worker_stop() is not necessary
>> because LogicalRepLauncherLock ensures that the above condition is
>> always false. Thought? Am I missing something?
>
> The lock exists only to keep the launcher from starting a
> worker. Creating a subscription and starting a worker for the
> slot run independently.
>
>> If the above condition is true, which means that there is the worker slot
>> having the "subid" of the worker to kill, but its "proc" has not been set yet.
>
> Yes. The situation happens after launcher sets subid and before
> ApplyWorkerMain attaches the slot.  The lock doesn't protect the
> section.

No. logicalrep_worker_launch() calls WaitForReplicationWorkerAttach()
and waits for the worker to attach to the slot. Then LogicalRepLauncherLock
is released. So both "subid" and "proc" should be set while the lock is being
held.

Regards,

-- 
Fujii Masao

  10. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-02-03T18:38:08Z

    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.

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.

Regards,

-- 
Fujii Masao

  11. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-02-04T20:11:20Z

    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

  12. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-02-06T16:33:17Z

    On Sun, Feb 5, 2017 at 5:11 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> 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.

OK, I understood why you used the lock in that way. But using LWLock
for that purpose is not valid.

>> 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.

Yes, let's remove LogicalRepLauncherLock and lock pg_subscription
with AccessExclusive mode at the beginning of DROP SUBSCRIPTION.
Attached patch does this.

Regards,

-- 
Fujii Masao

  13. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2017-02-06T17:13:12Z

    On 06/02/17 17:33, Fujii Masao wrote:
> On Sun, Feb 5, 2017 at 5:11 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> 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.
> 
> OK, I understood why you used the lock in that way. But using LWLock
> for that purpose is not valid.
> 

Yeah, I just tried to avoid what we are doing now really hard :)

>>> 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.
> 
> Yes, let's remove LogicalRepLauncherLock and lock pg_subscription
> with AccessExclusive mode at the beginning of DROP SUBSCRIPTION.
> Attached patch does this.
> 

Okay, looks reasonable to me.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

  14. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-02-07T15:04:49Z

    On Tue, Feb 7, 2017 at 2:13 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 06/02/17 17:33, Fujii Masao wrote:
>> On Sun, Feb 5, 2017 at 5:11 AM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> 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.
>>
>> OK, I understood why you used the lock in that way. But using LWLock
>> for that purpose is not valid.
>>
>
> Yeah, I just tried to avoid what we are doing now really hard :)
>
>>>> 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.
>>
>> Yes, let's remove LogicalRepLauncherLock and lock pg_subscription
>> with AccessExclusive mode at the beginning of DROP SUBSCRIPTION.
>> Attached patch does this.
>>
>
> Okay, looks reasonable to me.

Thanks for the review!
But ISMT that I should suspend committing the patch until we fix the issue
that Sawada reported in other thread. That bugfix may change the related
code and design very much.
https://www.postgresql.org/message-id/CAD21AoD+VO93zZ4ZQtZQb-jZ_wMko3OgGdx1MXO4T+8q_zHDDA@mail.gmail.com

Regards,

-- 
Fujii Masao

  15. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Robert Haas <robertmhaas@gmail.com> — 2017-02-08T21:12:54Z

    On Sat, Feb 4, 2017 at 3:11 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> 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.

Holding an LWLock until end-of-transaction is a phenomenally bad idea,
both because you lose interruptibility and because of the deadlock
risk.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Masahiko Sawada <sawada.mshk@gmail.com> — 2017-03-08T12:05:15Z

    On Wed, Feb 8, 2017 at 12:04 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Feb 7, 2017 at 2:13 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> On 06/02/17 17:33, Fujii Masao wrote:
>>> On Sun, Feb 5, 2017 at 5:11 AM, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> 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.
>>>
>>> OK, I understood why you used the lock in that way. But using LWLock
>>> for that purpose is not valid.
>>>
>>
>> Yeah, I just tried to avoid what we are doing now really hard :)
>>
>>>>> 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.
>>>
>>> Yes, let's remove LogicalRepLauncherLock and lock pg_subscription
>>> with AccessExclusive mode at the beginning of DROP SUBSCRIPTION.
>>> Attached patch does this.
>>>
>>
>> Okay, looks reasonable to me.
>
> Thanks for the review!
> But ISMT that I should suspend committing the patch until we fix the issue
> that Sawada reported in other thread. That bugfix may change the related
> code and design very much.
> https://www.postgresql.org/message-id/CAD21AoD+VO93zZ4ZQtZQb-jZ_wMko3OgGdx1MXO4T+8q_zHDDA@mail.gmail.com
>

That patch has been committed. And this issue still happens. Should we
add this to the open item list so it doesn't get missed?

Regards,

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

  17. Re: Cannot shutdown subscriber after DROP SUBSCRIPTION

    Fujii Masao <masao.fujii@gmail.com> — 2017-03-08T16:47:55Z

    On Wed, Mar 8, 2017 at 9:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 12:04 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Tue, Feb 7, 2017 at 2:13 AM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> On 06/02/17 17:33, Fujii Masao wrote:
>>>> On Sun, Feb 5, 2017 at 5:11 AM, Petr Jelinek
>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>> 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.
>>>>
>>>> OK, I understood why you used the lock in that way. But using LWLock
>>>> for that purpose is not valid.
>>>>
>>>
>>> Yeah, I just tried to avoid what we are doing now really hard :)
>>>
>>>>>> 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.
>>>>
>>>> Yes, let's remove LogicalRepLauncherLock and lock pg_subscription
>>>> with AccessExclusive mode at the beginning of DROP SUBSCRIPTION.
>>>> Attached patch does this.
>>>>
>>>
>>> Okay, looks reasonable to me.
>>
>> Thanks for the review!
>> But ISMT that I should suspend committing the patch until we fix the issue
>> that Sawada reported in other thread. That bugfix may change the related
>> code and design very much.
>> https://www.postgresql.org/message-id/CAD21AoD+VO93zZ4ZQtZQb-jZ_wMko3OgGdx1MXO4T+8q_zHDDA@mail.gmail.com
>>
>
> That patch has been committed. And this issue still happens. Should we
> add this to the open item list so it doesn't get missed?

Thanks for ping. Pushed the patch.

Regards,

-- 
Fujii Masao