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

On Fri, Apr 21, 2017 at 1:19 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> 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.

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

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.

> To fix this issue, we should call AtEOXact_ApplyLauncher() in
> FinishPreparedTransaction() or somewhere?
>

Regards,

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