Re: DROP SUBSCRIPTION and ROLLBACK

On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> On 15/02/17 06:43, Masahiko Sawada wrote:
>> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
>>>> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>>>> On 10/02/17 19:55, Masahiko Sawada wrote:
>>>>>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
>>>>>> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>>>>>> On 08/02/17 07:40, Masahiko Sawada wrote:
>>>>>>>> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
>>>>>>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>>>>>>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>>>>>>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>>>>>>>>> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>>>>>>>>>> For example what happens if apply crashes during the DROP
>>>>>>>>>>> SUBSCRIPTION/COMMIT and is not started because the delete from catalog
>>>>>>>>>>> is now visible so the subscription is no longer there?
>>>>>>>>>>
>>>>>>>>>> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
>>>>>>>>>> make it emit an error if it's executed within user's transaction block.
>>>>>>>>>
>>>>>>>>> It seems to me that this is exactly Petr's point: using
>>>>>>>>> PreventTransactionChain() to prevent things to happen.
>>>>>>>>
>>>>>>>> Agreed. It's better to prevent to be executed inside user transaction
>>>>>>>> block. And I understood there is too many failure scenarios we need to
>>>>>>>> handle.
>>>>>>>>
>>>>>>>>>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
>>>>>>>>>> after removing the entry from pg_subscription, then connect to the publisher
>>>>>>>>>> and remove the replication slot.
>>>>>>>>>
>>>>>>>>> For consistency that may be important.
>>>>>>>>
>>>>>>>> Agreed.
>>>>>>>>
>>>>>>>> Attached patch, please give me feedback.
>>>>>>>>
>>>>>>>
>>>>>>> This looks good (and similar to what initial patch had btw). Works fine
>>>>>>> for me as well.
>>>>>>>
>>>>>>> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
>>>>>>> similar failure scenarios there, should we prevent it from running
>>>>>>> inside transaction as well?
>>>>>>>
>>>>>>
>>>>>> Hmm, after thought I suspect current discussing approach. For
>>>>>> example, please image the case where CRAETE SUBSCRIPTION creates
>>>>>> subscription successfully but fails to create replication slot for
>>>>>> whatever reason, and then DROP SUBSCRIPTION drops the subscription but
>>>>>> dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
>>>>>> and DROP SUBSCRIPTION return ERROR but the subscription is created and
>>>>>> dropped successfully. I think that this behaviour confuse the user.
>>>>>>
>>>>>> I think we should just prevent calling DROP SUBSCRIPTION in user's
>>>>>> transaction block. Or I guess that it could be better to separate the
>>>>>> starting/stopping logical replication from subscription management.
>>>>>>
>>>>>
>>>>> We need to stop the replication worker(s) in order to be able to drop
>>>>> the slot. There is no such issue with startup of the worker as that one
>>>>> is launched by launcher after the transaction has committed.
>>>>>
>>>>> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
>>>>> transaction block and don't do any commits inside of those (so that
>>>>> there are no rollbacks, which solves your initial issue I believe). That
>>>>> way failure to create/drop slot will result in subscription not being
>>>>> created/dropped which is what we want.
>>>
>>> On second thought, +1.
>>>
>>>> I basically agree this option, but why do we need to change CREATE
>>>> SUBSCRIPTION as well?
>>>
>>> Because the window between the creation of replication slot and the transaction
>>> commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
>>> during that window, the replication slot unexpectedly remains while there is no
>>> corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
>>> from being executed within user's transaction block, there is still such
>>> window. But we can reduce the possibility of that problem.
>>
>> Thank you for the explanation. I understood and agree.
>>
>> I think we should disallow to call ALTER SUBSCRIPTION inside a user's
>> transaction block as well.
>
> Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
>

Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.

Regards,

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