Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK

On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>> On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>> On 15/02/17 06:43, Masahiko Sawada wrote:
>>>>>> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>>>> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>>>>>> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
>>>>>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>>>>>> On 10/02/17 19:55, Masahiko Sawada wrote:
>>>>>>>>>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
>>>>>>>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>>>>>>>> On 08/02/17 07:40, Masahiko Sawada wrote:
>>>>>>>>>>>> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
>>>>>>>>>>>> <michael.paquier@gmail.com> wrote:
>>>>>>>>>>>>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>>>>>>>>>>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>>>>>>>>>>>>> <petr.jelinek@2ndquadrant.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.
>>>
>>> We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction
>>> block only when CREATE/DROP SLOT option is set?
>>>
>>> + /*
>>> + * We cannot run CREATE SUBSCRIPTION inside a user transaction
>>> + * block.
>>> + */
>>> + PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");
>>>
>>> I think that more comments about why the command is disallowed inside
>>> a user transaction block are necessary. For example,
>>
>> I agree with you.
>>
>>>
>>> ----------------------
>>> Disallow CREATE SUBSCRIPTION [CREATE SLOT] inside a user transaction block.
>>>
>>> When CREATE SLOT is set, this command creates the replication slot on
>>> the remote server. This operation is not transactional. So, if the transaction
>>> is rollbacked, the created replication slot unexpectedly remains while
>>> there is no corresponding entry in pg_subscription. To reduce the possibility
>>> of this problem, we allow CREATE SLOT option only outside a user transaction
>>> block.
>>>
>>> XXX Note that this restriction cannot completely prevent "orphan" replication
>>> slots. The transaction of CREATE SUBSCRIPTION can still fail after creating
>>> the replication slot on the remote server, though it's basically less likely
>>> to happen.
>>> ----------------------
>>>
>>> + * We cannot run DROP SUBSCRIPTION inside a user transaction
>>> + * block.
>>> + */
>>> + PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");
>>>
>>> Same as above.
>>
>> While writing regression test for this issue, I found an another bug
>> of DROP SUBSCRIPTION; DROP SUBSCRIPTION with DROP SLOT option fails to
>> parse because DROP is a keyword, not IDENT.
>
> Good catch!
>
>> Attached 000 patch fixes it
>
> Or we should change the syntax of DROP SUBSCRIPTION as follows, and
> handle the options in the same way as the options like "CREATE SLOT" in
> CREATE/ALTER SUBSCRIPTION? In CREATE/ALTER commands, the options
> are specified with WITH clause. But only DROP command doesn't accept
> WITH clause. This looks inconsistent.

+1 for adding WITH clause to DROP SUBSCRIPTION option. That way we can
check conflicting or redundant options easier. Will update patches.

Regards,

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