On Wed, Feb 22, 2017 at 2:17 AM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > 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.
Attached updated version patches. Please review these. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
001_disallow_sub_ddls_in_transaction_block_v4.patch
Description: Binary data
000_change_drop_sub_option_syntax.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
