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 >>> 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
disallow_sub_ddls_in_transaction_block_v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
