Re: DROP SUBSCRIPTION and ROLLBACK

On Wed, Feb 22, 2017 at 2:17 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>>> 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.
>>>>
>>>> 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.

Attached updated version patches. Please review these.

Regards,

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