Re: DROP SUBSCRIPTION and ROLLBACK

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.

----------------------
DROP SUBSCRIPTION [ IF EXISTS ] name [ WITH (option [, ... ]) ]

where option can be:
| DROP SLOT | NODROP SLOT
----------------------

Regards,

--
Fujii Masao