Re: some review comments on logical rep code

On 26/04/17 18:36, Fujii Masao wrote:
> On Thu, Apr 27, 2017 at 1:28 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Wed, Apr 26, 2017 at 3:47 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>>> At Wed, 26 Apr 2017 14:31:12 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoDMy8a6UwMrRh8pigQbDC+JAOQ4m_tXT41VRP4SYp23=w@mail.gmail.com>
>>>> On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek
>>>> <petr.jelinek@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