On Wed, Sep 19, 2018 at 12:12 PM, Kyotaro HORIGUCHI
<horiguchi.kyot...@lab.ntt.co.jp> wrote:
> 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.
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
diff --git a/src/test/modules/commit_ts/t/005_recovery.pl b/src/test/modules/commit_ts/t/005_recovery.pl
new file mode 100644
index 0000000..80d52fc
--- /dev/null
+++ b/src/test/modules/commit_ts/t/005_recovery.pl
@@ -0,0 +1,53 @@
+# Testing of recovery from where commit timestamps was off
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 1;
+
+my $node = get_new_node('test');
+$node->init;
+$node->append_conf('postgresql.conf',
+ "track_commit_timestamp = off");
+$node->start;
+
+# 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.
+$node->restart;
+
+# Consume 2000 XIDs to beyond the commitTS page boundary.
+$node->safe_psql(
+ 'postgres',
+ qq(
+CREATE PROCEDURE comsume_xid(cnt int)
+AS \$\$
+DECLARE
+ i int;
+BEGIN
+ FOR i in 1..cnt LOOP
+ EXECUTE 'SELECT txid_current()';
+ COMMIT;
+ END LOOP;
+END;
+\$\$
+LANGUAGE plpgsql;
+));
+$node->safe_psql('postgres', 'CALL comsume_xid(2000)');
+
+$node->teardown_node;
+
+# Enable track_commit_tiemstamp
+$node->append_conf('postgresql.conf',
+ "track_commit_timestamp = on");
+
+# 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.
+$node->start;
+
+# Check if the server launched.
+is($node->psql('postgres', qq(SELECT 1)), 0,
+ 'started from the crash recovery');
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4754e75..0ece043 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6833,11 +6833,12 @@ StartupXLOG(void)
StartupMultiXact();
/*
- * Ditto commit timestamps. In a standby, we do it if setting is enabled
- * in ControlFile; in a master we base the decision on the GUC itself.
+ * Ditto commit timestamps. In both a standby and a master, we do it if
+ * setting is enabled in ControlFile since we don't care about the commit
+ * timestamps of the transaction that were executed when the commit
+ * timestamp is disabled.
*/
- if (ArchiveRecoveryRequested ?
- ControlFile->track_commit_timestamp : track_commit_timestamp)
+ if (ControlFile->track_commit_timestamp)
StartupCommitTs();
/*
