I have committed this version. I have omitted all the talk about 2PC. There are discussions ongoing about changing the transaction behavior of CREATE SUBSCRIPTION, which might interfere with that. If someone wants to rebase and propose the parts about 2PC separately, I don't object, but it can be handled separately.
I will close this open item now, since I think we have covered everything that was originally reported. Please start new threads if there are any issues that were missed or that have been discovered during the discussion. On 4/23/17 22:18, Masahiko Sawada wrote: > On Sat, Apr 22, 2017 at 4:26 AM, Fujii Masao <masao.fu...@gmail.com> wrote: >> On Fri, Apr 21, 2017 at 4:02 PM, Masahiko Sawada <sawada.m...@gmail.com> >> wrote: >>> On Fri, Apr 21, 2017 at 1:19 AM, Fujii Masao <masao.fu...@gmail.com> wrote: >>>> On Tue, Apr 18, 2017 at 5:16 PM, Masahiko Sawada <sawada.m...@gmail.com> >>>> wrote: >>>>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI >>>>> <horiguchi.kyot...@lab.ntt.co.jp> wrote: >>>>>> Hi, >>>>>> >>>>>> Thank you for the revised version. >>>>>> >>>>>> At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada >>>>>> <sawada.m...@gmail.com> wrote in >>>>>> <CAD21AoA+yFJTbw0PCw-ttmh9TsTygM3=iavxf+5bx4o9-ux...@mail.gmail.com> >>>>>>> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada >>>>>>> <sawada.m...@gmail.com> wrote: >>>>>>>> On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI >>>>>>>> <horiguchi.kyot...@lab.ntt.co.jp> wrote: >>>>>>>>> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada >>>>>>>>> <sawada.m...@gmail.com> wrote in >>>>>>>>> <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=fkwjw29mx...@mail.gmail.com> >>>>>>>>>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <masao.fu...@gmail.com> >>>>>>>>>> wrote: >>>>>>>>>> 1. >>>>>>>>>>> >>>>>>>>>>> In ApplyWorkerMain(), DatumGetInt32() should be used to get integer >>>>>>>>>>> value from the argument, instead of DatumGetObjectId(). >>>>>>>>>> >>>>>>>>>> Attached 001 patch fixes it. >>>>>>>>> >>>>>>>>> Hmm. I looked at the launcher side and found that it assigns bare >>>>>>>>> integer to bgw_main_arg. It also should use Int32GetDatum. >>>>>>>> >>>>>>>> Yeah, I agreed. Will fix it. >>>>>> >>>>>> Thanks. >>>>>> >>>>>>>>>> 2. >>>>>>>>>>> >>>>>>>>>>> No one resets on_commit_launcher_wakeup flag to false. >>>>>>>>>> >>>>>>>>>> Attached 002 patch makes on_commit_launcher_wakeup turn off after >>>>>>>>>> woke >>>>>>>>>> up the launcher process. >>>>>>>>> >>>>>>>>> It is omitting the abort case. Maybe it should be >>>>>>>>> AtEOXact_ApplyLauncher(bool commit)? >>>>>>>> >>>>>>>> Should we wake up the launcher process even when abort? >>>>>> >>>>>> No, I meant that on_commit_launcher_wakeup should be just reset >>>>>> without waking up launcher on abort. >>>>> >>>>> I understood. Sounds reasonable. >>>> >>>> ROLLBACK PREPARED should not wake up the launcher, but not even with the >>>> patch. >>> >>> Yeah, I've missed it. But on_commit_launcher_wakeup dosen't correspond >>> to a prepared transaction. Even COMMIT PREPARED might wake up the >>> launcher process but might not wake up it. ROLLBACK PREPARED is also >>> the same. For example, in the following situation we wake up the >>> launcher at COMMIT, not at COMMIT PREPARED. >>> >>> (BTW, CreateSubscription, is the only one function that sets >>> on_commit_launcher_wakeup for now, cannot be called in a transaction >>> block. So it doesn't actually happen that we wake up the launcher when >>> a ROLLBACK PREPARED. But considering waking up the launcher by ALTER >>> SUBSCRIPTION ENABLE, we need to deal with it.) >> >> We can run CREATE SUBSCRIPTION with NOCREATE SLOT option >> within the transaction block. > > Oops, I'd missed it. > >> >>> >>> BEGIN; >>> ALTER SUBSCRIPTION hoge_sub ENABLE; >>> PREPARE TRANSACTION 'g'; >>> BEGIN; >>> SELECT 1; >>> COMMIT; -- wake up the launcher at this point. >>> COMMIT PREPARED 'g'; >>> >>> Even if we wake up the launcher even when a ROLLBACK PREPARED, it's >>> not a big deal to the launcher process actually. Compared with the >>> complexly of changing the logic so that on_commit_launcher_wakeup >>> corresponds to prepared transaction, we might want to accept this >>> behavior. >> >> on_commit_launcher_wakeup needs to be recoreded in 2PC state >> file to handle this issue properly. But I agree with you, that would >> be overkill for small gain. So I'm thinking to add the following >> comment into your patch and push it. Thought? >> >> ------------------------- >> Note that on_commit_launcher_wakeup flag doesn't work as expected in 2PC >> case. >> For example, COMMIT PREPARED on the transaction enabling the flag >> doesn't wake up >> the logical replication launcher if ROLLBACK on another transaction >> runs before it. >> To handle this case properly, the flag needs to be recorded in 2PC >> state file so that >> COMMIT PREPARED and ROLLBACK PREPARED can determine whether to wake up >> the launcher. However this is overkill for small gain and false wakeup >> of the launcher >> is not so harmful (probably we can live with that), so we do nothing >> here for this issue. >> ------------------------- >> > > Agreed. > > I added this comment to the patch and attached it. > > Regards, > > -- > Masahiko Sawada > NIPPON TELEGRAPH AND TELEPHONE CORPORATION > NTT Open Source Software Center > > > > -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
