Re: some review comments on logical rep code

Hello,

At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBU8mZdGx-sTJ3u+qkaej5RPnOuotW4kfeXc4XdDNF8cw(at)mail(dot)gmail(dot)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