On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.m...@gmail.com> 
> wrote:
>> On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
>> <petr.jeli...@2ndquadrant.com> wrote:
>>> On 15/02/17 06:43, Masahiko Sawada wrote:
>>>> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fu...@gmail.com> wrote:
>>>>> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.m...@gmail.com> 
>>>>> wrote:
>>>>>> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
>>>>>> <petr.jeli...@2ndquadrant.com> wrote:
>>>>>>> On 10/02/17 19:55, Masahiko Sawada wrote:
>>>>>>>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
>>>>>>>> <petr.jeli...@2ndquadrant.com> wrote:
>>>>>>>>> On 08/02/17 07:40, Masahiko Sawada wrote:
>>>>>>>>>> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
>>>>>>>>>> <michael.paqu...@gmail.com> wrote:
>>>>>>>>>>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fu...@gmail.com> 
>>>>>>>>>>> wrote:
>>>>>>>>>>>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>>>>>>>>>>> <petr.jeli...@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 
>>>>> 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
parse because DROP is a keyword, not IDENT. Attached 000 patch fixes
it, and 001 patches fixes the original issue on this thread.

Please review these.


Masahiko Sawada
NTT Open Source Software Center

Attachment: 001_disallow_sub_ddls_in_transaction_block_v3.patch
Description: Binary data

Attachment: 000_fix_drop_sub_drop_slot.patch
Description: Binary data

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to