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' (this is centainly a kind of false wakeups,
though). We can live with this failure when using two-paase
commmit, but I think it shouldn't happen silently.
How about providing AtPrepare_ApplyLauncher(void) like the
follows and calling it in PrepareTransaction?
void
AtPrepare_ApplyLauncher(void)
{ if (on_commit_launcher_wakeup) ereport(WARNING, (errmsg ("logical replication may not start immediately"),
errhint("PREPARE TRANSACTION can hinder immediate lanching of logical replication worker.")));
}
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center