From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | sawada(dot)mshk(at)gmail(dot)com |
Cc: | petr(dot)jelinek(at)2ndquadrant(dot)com, masao(dot)fujii(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: some review comments on logical rep code |
Date: | 2017-04-26 06:47:47 |
Message-ID: | 20170426.154747.187646116.horiguchi.kyotaro@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
At Wed, 26 Apr 2017 14:31:12 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDMy8a6UwMrRh8pigQbDC+JAOQ4m_tXT41VRP4SYp23=w(at)mail(dot)gmail(dot)com>
> On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek
> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> > On 26/04/17 01:01, Fujii Masao wrote:
> >>>> 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?
> >>
> >> Or we should apply the attached patch and handle the 2PC case properly?
> >> I was thinking that it's overkill more than necessary, but that seems not true
> >> as far as I implement that.
> >>
> > Looks like it does not even increase size of the 2pc file, +1 for this.
>
> In my honest opinion, I didn't have a big will that we should handle
> even two-phase commit case, because this case is very rare (I could
> not image such case) and doesn't mean to lead a harmful result such as
> crash of server and returning inconsistent result. it just delays the
> launching worker for at most 3 minutes. We also can deal with this for
> example by making maximum nap time of apply launcher user-configurable
> and document it.
> But if we can deal with it by minimum changes like attached your patch I agree.
This change looks reasonable to me, +1 from me to this.
The patch reads on_commit_launcher_wakeup directly then updates
it via ApplyALuncherWakeupAtCommit() but it's too much to add a
function for the sake of this.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | Konstantin Knizhnik | 2017-04-26 07:00:17 | Re: Cached plans and statement generalization |
Previous Message | Masahiko Sawada | 2017-04-26 06:07:00 | Fix a typo in subscriptioncmd.c |