At Sat, 15 Sep 2018 19:26:39 +0900, Masahiko Sawada <sawada.m...@gmail.com> wrote in <cad21aoaxsnorp3tjvjhroak+8q5yshsnw-n8buwz4bdu7qo...@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. 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. ==== The following error message is seen when the issue happens. > DETAIL: Could not read from file "pg_commit_ts/0000" at offset 8192: Success. This seems what 811b6e36a9 tried to fix. This should be like the follows. > DETAIL: Could not read from file "pg_commit_ts/0000" at offset 8192: read 0 > of 8192" I, as one of reviewers of it, didn't remember how it was overlooked butthis also needs a fix in this patch or as a separate issue. regards. -- Kyotaro Horiguchi NTT Open Source Software Center