On 26/04/17 18:36, Fujii Masao wrote: > On Thu, Apr 27, 2017 at 1:28 AM, Fujii Masao <masao.fu...@gmail.com> wrote: >> On Wed, Apr 26, 2017 at 3:47 PM, Kyotaro HORIGUCHI >> <horiguchi.kyot...@lab.ntt.co.jp> wrote: >>> At Wed, 26 Apr 2017 14:31:12 +0900, Masahiko Sawada <sawada.m...@gmail.com> >>> wrote in >>> <CAD21AoDMy8a6UwMrRh8pigQbDC+JAOQ4m_tXT41VRP4SYp23=w...@mail.gmail.com> >>>> On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek >>>> <petr.jeli...@2ndquadrant.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. >> >> OK, so what about the attached patch? I replaced all the calls to >> ApplyLauncherWakeupAtCommit() with the code "on_commit_launcher_wakeup = >> true". > > BTW, while I was reading the code to implement the patch that > I posted upthread, I found that the following condition doesn't > work as expected. This is because "last_start_time" is always 0. > > /* Limit the start retry to once a wal_retrieve_retry_interval */ > if (TimestampDifferenceExceeds(last_start_time, now, > wal_retrieve_retry_interval)) > > "last_start_time" is set to "now" when the launcher starts up new > worker. But "last_start_time" is defined and always initialized with 0 > at the beginning of the main loop in ApplyLauncherMain(), so > the above condition becomes always true. This is obviously a bug. > Attached patch fixes this issue. >
Yes, please go ahead and commit this. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
