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. > > 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. ------------------------- Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
