Re: [HACKERS] some review comments on logical rep code

On Tue, Apr 18, 2017 at 5:16 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> Hi,
>>
>> Thank you for the revised version.
>>
>> At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoA+yFJTbw0PCw-ttmh9TsTygM3=iavXF+5Bx4O9-UXdsQ@mail.gmail.com>
>>> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>> > On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
>>> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>>> >> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=FKwjw29mxj0Q@mail.gmail.com>
>>> >>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <masao.fujii@gmail.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.
To fix this issue, we should call AtEOXact_ApplyLauncher() in
FinishPreparedTransaction() or somewhere?

Regards,

-- 
Fujii Masao