Re: some review comments on logical rep code

On 26/04/17 01:01, Fujii Masao wrote:
> On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> Hello,
>>
>> At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoBU8mZdGx-sTJ3u+qkaej5RPnOuotW4kfeXc4XdDNF8cw@mail.gmail.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.
> 

Looks like it does not even increase size of the 2pc file, +1 for this.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services