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. Attached 000 patch fixes it, and 001 patches fixes the original issue on this thread. Please review these. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
001_disallow_sub_ddls_in_transaction_block_v3.patch
Description: Binary data
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: http://www.postgresql.org/mailpref/pgsql-hackers
