On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masao <masao.fu...@gmail.com> wrote: > On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada <sawada.m...@gmail.com> > wrote: >> 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 >>>>>>> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
