Re: [HACKERS] some review comments on logical rep code - Mailing list pgsql-hackers

On Thu, Apr 27, 2017 at 5:37 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> 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.

Pushed. Thanks!

Regards,

-- 
Fujii Masao