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
002_Reset_on_commit_launcher_wakeup_v4.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
