Re: some review comments on logical rep code

At Tue, 25 Apr 2017 10:11:06 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoBAXRMBBaH5-mYXxksPaTis0xt_-yvvb2Y+jG2m-EWAoQ@mail.gmail.com>
> On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > Hello,
> >
> > At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoBU8mZdGx-sTJ3u+qkaej5RPnOuotW4kfeXc4XdDNF8cw@mail.gmail.com>
> >> >> 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.
> >
> > The following "However" may need a follwoing comma.
> >
> >> 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.
> >
> > I agree this as a whole. But I think that the main issue here is
> > not false wakeups, but 'possible delay of launching new workers
> > by 3 minutes at most'
> 
> In what case does launching of the apply worker delay 3 minutes at most?

In the "while (!got_SIGTERM)" loop in ApplyLauncherMain, if we
tried to run enabled subscriptions but no subscription is
enabled, it waits for 3 minutes. If we turn on a subscription but
the trigger is consumed by the false wakeup, the loop sees no
enabled subscription and will sleep for 3 minutes unless any
other trigger comes.

# I haven't tried that, though.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center