Re: some review comments on logical rep code

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:
>> On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> 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?
>>
>> 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.
>>

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.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center