On Wed, Sep 19, 2018 at 12:12 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Sat, 15 Sep 2018 19:26:39 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoAxSNorp3TjvJhrOAk+8q5yshSnW-n8buwz4bdU7qOtPA@mail.gmail.com>
>> >> To fix that maybe we can disable commitTs if
>> >> controlFile->track_commit_timestamp == false and the
>> >> track_commit_timestamp == true even in crash recovery.
>> >
>> > Hmm, so keep it off while crash recovery runs, and once it's out of that
>> > then enable it automatically?
>>
>> Yes. The attached patch changes it to check
>> controlFile->track_commit_timestamp even the crash recover case. If
>> track_commit_timestamp is set to true in the config file, it's enabled
>> at end of the recovery.
>>
>> > That might work -- by definition we don't
>> > care about the commit TSs of the transaction replayed during crash
>> > recovery, since they were executed in the primary that didn't have
>> > commitTS enable anyway.
>> >
>> > It seems like the first thing we need is TAP cases that reproduce these
>> > two crash scenarios.
>>
>> I attached TAP test that reproduces this issue. We can reproduce it
>> even with single server; making postgres replay a commit WAL in the
>> crash recovery after consumed transactions and enabled
>> track_commit_timestamp.
>
> The fix looks good to me. The TAP test works fine.
Thank you for looking at this patch.
>
> In the TAP test:
>
> ====
> The test script lacks a general description about its objective.
>
> ====
> +$node->append_conf('postgresql.conf',
> + "track_commit_timestamp = off");
> +$node->start;
> +
> +# When we start firstly from the initdb the PARAMETER_CHANGES
> +# is emitted at end of the recovery, which disables the
> +# track_commit_timestamp if the crash recovery replay that
> +# WAL. Therefore we restart the server so that we can recovery
> +# from the point where doesn't contain that WAL.
> +$node->restart;
>
> The first startup actually doesn't emit a PARAMETER_CHAGES. If
> track_commit_timestamp were set to on, we get one. However, I
> agree that it is reasonable to eliminate the possiblity of being
> affected by the record. How about something like this?
>
> +# We don't want to replay PARAMETER_CHANGES record while the
> +# crash recovery test below. It is not expected to be emitted
> +# thus far, but we restart the server to get rid of it just in
> +# case.
>
>
> ====
> +# During the crash recovery we replay the commit WAL that sets
> +# the commit timestamp to a new page.
> +$node->start;
>
> The comment is mentioning the unexpected symptom. Shouldn't it be
> the desired behavior?
>
> +# During crash recovery server will replay COMMIT records
> +# emitted while commit timestamp was off. The server can start
> +# if we correctly avoid processing commit timestamp for the
> +# records.
>
I agreed with your all review comments. Attached the updated patch.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center