Re: when set track_commit_timestamp on, database system abort startup

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: sawada(dot)mshk(at)gmail(dot)com
Cc: alvherre(at)2ndquadrant(dot)com, hailong(dot)li(at)qunar(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: when set track_commit_timestamp on, database system abort startup
Date: 2018-09-19 03:12:44
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

At Sat, 15 Sep 2018 19:26:39 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoAxSNorp3TjvJhrOAk+8q5yshSnW-n8buwz4bdU7qOtPA(at)mail(dot)gmail(dot)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.

+ "track_commit_timestamp = off");
+# 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.

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.

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.


Kyotaro Horiguchi
NTT Open Source Software Center

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-09-19 03:46:21 Re: when set track_commit_timestamp on, database system abort startup
Previous Message Michael Paquier 2018-09-19 03:06:47 Re: heap_sync seems rather oblivious to partitioned tables (wal_level=minimal)