Re: some review comments on logical rep code

On Sat, Apr 22, 2017 at 4:26 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Apr 21, 2017 at 4:02 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Fri, Apr 21, 2017 at 1:19 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Tue, Apr 18, 2017 at 5:16 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
>>>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>> Hi,
>>>>>
>>>>> Thank you for the revised version.
>>>>>
>>>>> At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoA+yFJTbw0PCw-ttmh9TsTygM3=iavXF+5Bx4O9-UXdsQ(at)mail(dot)gmail(dot)com>
>>>>>> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>>>> > On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
>>>>>> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>>> >> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=FKwjw29mxj0Q(at)mail(dot)gmail(dot)com>
>>>>>> >>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>>> >>> 1.
>>>>>> >>> >
>>>>>> >>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>>>>>> >>> > value from the argument, instead of DatumGetObjectId().
>>>>>> >>>
>>>>>> >>> Attached 001 patch fixes it.
>>>>>> >>
>>>>>> >> Hmm. I looked at the launcher side and found that it assigns bare
>>>>>> >> integer to bgw_main_arg. It also should use Int32GetDatum.
>>>>>> >
>>>>>> > Yeah, I agreed. Will fix it.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>> >>> 2.
>>>>>> >>> >
>>>>>> >>> > No one resets on_commit_launcher_wakeup flag to false.
>>>>>> >>>
>>>>>> >>> Attached 002 patch makes on_commit_launcher_wakeup turn off after woke
>>>>>> >>> up the launcher process.
>>>>>> >>
>>>>>> >> It is omitting the abort case. Maybe it should be
>>>>>> >> AtEOXact_ApplyLauncher(bool commit)?
>>>>>> >
>>>>>> > Should we wake up the launcher process even when abort?
>>>>>
>>>>> No, I meant that on_commit_launcher_wakeup should be just reset
>>>>> without waking up launcher on abort.
>>>>
>>>> I understood. Sounds reasonable.
>>>
>>> ROLLBACK PREPARED should not wake up the launcher, but not even with the patch.
>>
>> Yeah, I've missed it. But on_commit_launcher_wakeup dosen't correspond
>> to a prepared transaction. Even COMMIT PREPARED might wake up the
>> launcher process but might not wake up it. ROLLBACK PREPARED is also
>> the same. For example, in the following situation we wake up the
>> launcher at COMMIT, not at COMMIT PREPARED.
>>
>> (BTW, CreateSubscription, is the only one function that sets
>> on_commit_launcher_wakeup for now, cannot be called in a transaction
>> block. So it doesn't actually happen that we wake up the launcher when
>> a ROLLBACK PREPARED. But considering waking up the launcher by ALTER
>> SUBSCRIPTION ENABLE, we need to deal with it.)
>
> We can run CREATE SUBSCRIPTION with NOCREATE SLOT option
> within the transaction block.

Oops, I'd missed it.

>
>>
>> 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.

Regards,

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