Further tweak commit_timestamp behavior

As pointed out by Fujii Masao, we weren't quite there on a standby
behaving sanely: first because we were failing to acquire the correct
state in the case where no XLOG_PARAMETER_CHANGE message was sent
(because a checkpoint had already happened after the setting was changed
in the master, and then the standby was restarted); and second because
promoting the standby with the feature enabled failed to activate it if
the master had the feature disabled.

This patch fixes both those misbehaviors hopefully without
re-introducing any old problems.

Also change the hint emitted in a standby together with the error
message about the feature being disabled, to make it point out that the
place to chance the setting is the master.  Otherwise, if the setting is
already enabled in the standby, it is very confusing to have it say that
the setting must be enabled ...

Authors: Álvaro Herrera, Petr Jelínek.
Backpatch to 9.5.

diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 349228df1e540625f243350ae98c566102c362a2..f9da7593125fc8f7585ca7ab80a0b6a7eeca82b4 100644 (file)
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -106,6 +106,7 @@ static void SetXidCommitTsInPage(TransactionId xid, int nsubxids,
                                         RepOriginId nodeid, int pageno);
 static void TransactionIdSetCommitTs(TransactionId xid, TimestampTz ts,
                                                 RepOriginId nodeid, int slotno);
+static void error_commit_ts_disabled(void);
 static int     ZeroCommitTsPage(int pageno, bool writeXlog);
 static bool CommitTsPagePrecedes(int page1, int page2);
 static void ActivateCommitTs(void);
@@ -297,11 +298,7 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
 
        /* Error if module not enabled */
        if (!commitTsShared->commitTsActive)
-               ereport(ERROR,
-                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                                errmsg("could not get commit timestamp data"),
-                         errhint("Make sure the configuration parameter \"%s\" is set.",
-                                         "track_commit_timestamp")));
+               error_commit_ts_disabled();
 
        /*
         * If we're asked for the cached value, return that.  Otherwise, fall
@@ -368,11 +365,7 @@ GetLatestCommitTsData(TimestampTz *ts, RepOriginId *nodeid)
 
        /* Error if module not enabled */
        if (!commitTsShared->commitTsActive)
-               ereport(ERROR,
-                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-                                errmsg("could not get commit timestamp data"),
-                         errhint("Make sure the configuration parameter \"%s\" is set.",
-                                         "track_commit_timestamp")));
+               error_commit_ts_disabled();
 
        xid = commitTsShared->xidLastCommit;
        if (ts)
@@ -384,6 +377,19 @@ GetLatestCommitTsData(TimestampTz *ts, RepOriginId *nodeid)
        return xid;
 }
 
+static void
+error_commit_ts_disabled(void)
+{
+       ereport(ERROR,
+                       (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                        errmsg("could not get commit timestamp data"),
+                        RecoveryInProgress() ?
+                        errhint("Make sure the configuration parameter \"%s\" is set in the master server.",
+                                        "track_commit_timestamp") :
+                        errhint("Make sure the configuration parameter \"%s\" is set.",
+                                        "track_commit_timestamp")));
+}
+
 /*
  * SQL-callable wrapper to obtain commit time of a transaction
  */
@@ -510,7 +516,7 @@ BootStrapCommitTs(void)
        /*
         * Nothing to do here at present, unlike most other SLRU modules; segments
         * are created when the server is started with this module enabled. See
-        * StartupCommitTs.
+        * ActivateCommitTs.
         */
 }
 
@@ -544,13 +550,13 @@ ZeroCommitTsPage(int pageno, bool writeXlog)
  * configuration.
  */
 void
-StartupCommitTs(bool force_enable)
+StartupCommitTs(bool enabled)
 {
        /*
         * If the module is not enabled, there's nothing to do here.  The module
         * could still be activated from elsewhere.
         */
-       if (track_commit_timestamp || force_enable)
+       if (enabled)
                ActivateCommitTs();
 }
 

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f17f83417d1d7b00ce8f8ce10f7e9e7d8387aa3e..86debf44123ec01519bd25851ec0e050dc992af7 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6568,6 +6568,10 @@ StartupXLOG(void)
                         * Startup commit log, commit timestamp and subtrans only.
                         * MultiXact has already been started up and other SLRUs are not
                         * maintained during recovery and need not be started yet.
+                        *
+                        * For commit timestamps, we do this based on the control file
+                        * info: in a standby, we want to drive it off the state of the
+                        * master, not local configuration.
                         */
                        StartupCLOG();
                        StartupCommitTs(ControlFile->track_commit_timestamp);
@@ -7339,7 +7343,7 @@ StartupXLOG(void)
        if (standbyState == STANDBY_DISABLED)
        {
                StartupCLOG();
-               StartupCommitTs(false);
+               StartupCommitTs(track_commit_timestamp);
                StartupSUBTRANS(oldestActiveXID);
        }
 

diff --git a/src/include/access/commit_ts.h b/src/include/access/commit_ts.h
index 3844bb30ff252f0708c4ad63a18d297bfbcfe47c..f5b39691e71e2fe152a23361fa94952e47bd45d6 100644 (file)
--- a/src/include/access/commit_ts.h
+++ b/src/include/access/commit_ts.h
@@ -34,7 +34,7 @@ extern Size CommitTsShmemBuffers(void);
 extern Size CommitTsShmemSize(void);
 extern void CommitTsShmemInit(void);
 extern void BootStrapCommitTs(void);
-extern void StartupCommitTs(bool force_enable);
+extern void StartupCommitTs(bool enabled);
 extern void CommitTsParameterChange(bool xlrecvalue, bool pgcontrolvalue);
 extern void CompleteCommitTsInitialization(void);
 extern void ShutdownCommitTs(void);