Fix commit timestamp initialization

This module needs explicit initialization in order to replay WAL records
in recovery, but we had broken this recently following changes to make
other (stranger) scenarios work correctly.  To fix, rework the
initialization sequence so that it always takes place before WAL replay
commences for both master and standby.

I could have gone for a more localized fix that just added a "startup"
call for the master server, but it seemed better to restructure the
existing callers as well so that the whole thing made more sense.  As a
drawback, there is more control logic in xlog.c now than previously, but
doing otherwise meant passing down the ControlFile flag, which seemed
uglier as a whole.

This also meant adding a check to not re-execute ActivateCommitTs if it
had already been called.

Reported by Fujii Masao.

Backpatch to 9.5.

diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 834010ff716f27ac66577e32461e16ac27e91707..8f09dc83ae767e0c0848971a84c1630521802e58 100644 (file)
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -545,19 +545,11 @@ ZeroCommitTsPage(int pageno, bool writeXlog)
 /*
  * This must be called ONCE during postmaster or standalone-backend startup,
  * after StartupXLOG has initialized ShmemVariableCache->nextXid.
- *
- * Caller may choose to enable the feature even when it is turned off in the
- * configuration.
  */
 void
-StartupCommitTs(bool enabled)
+StartupCommitTs(void)
 {
-       /*
-        * If the module is not enabled, there's nothing to do here.  The module
-        * could still be activated from elsewhere.
-        */
-       if (enabled)
-               ActivateCommitTs();
+       ActivateCommitTs();
 }
 
 /*
@@ -570,9 +562,17 @@ CompleteCommitTsInitialization(void)
        /*
         * If the feature is not enabled, turn it off for good.  This also removes
         * any leftover data.
+        *
+        * Conversely, we activate the module if the feature is enabled.  This is
+        * not necessary in a master system because we already did it earlier, but
+        * if we're in a standby server that got promoted which had the feature
+        * enabled and was following a master that had the feature disabled, this
+        * is where we turn it on locally.
         */
        if (!track_commit_timestamp)
                DeactivateCommitTs();
+       else
+               ActivateCommitTs();
 }
 
 /*
@@ -591,6 +591,9 @@ CommitTsParameterChange(bool newvalue, bool oldvalue)
         *
         * If the module is disabled in the master, disable it here too, unless
         * the module is enabled locally.
+        *
+        * Note this only runs in the recovery process, so an unlocked read is
+        * fine.
         */
        if (newvalue)
        {
@@ -620,8 +623,20 @@ CommitTsParameterChange(bool newvalue, bool oldvalue)
 static void
 ActivateCommitTs(void)
 {
-       TransactionId xid = ShmemVariableCache->nextXid;
-       int                     pageno = TransactionIdToCTsPage(xid);
+       TransactionId xid;
+       int                     pageno;
+
+       /* If we've done this already, there's nothing to do */
+       LWLockAcquire(CommitTsLock, LW_EXCLUSIVE);
+       if (commitTsShared->commitTsActive)
+       {
+               LWLockRelease(CommitTsLock);
+               return;
+       }
+       LWLockRelease(CommitTsLock);
+
+       xid = ShmemVariableCache->nextXid;
+       pageno = TransactionIdToCTsPage(xid);
 
        /*
         * Re-Initialize our idea of the latest page number.

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 86debf44123ec01519bd25851ec0e050dc992af7..71fc8ffa272d423789dc59652d09de93865b61ce 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6338,6 +6338,14 @@ 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.
+        */
+       if (ArchiveRecoveryRequested ?
+               ControlFile->track_commit_timestamp : track_commit_timestamp)
+               StartupCommitTs();
+
        /*
         * Recover knowledge about replay progress of known replication partners.
         */
@@ -6565,16 +6573,11 @@ StartupXLOG(void)
                        ProcArrayInitRecovery(ShmemVariableCache->nextXid);
 
                        /*
-                        * Startup commit log, commit timestamp and subtrans only.
-                        * MultiXact has already been started up and other SLRUs are not
+                        * Startup commit log and subtrans only.  MultiXact and commit
+                        * timestamp have 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);
                        StartupSUBTRANS(oldestActiveXID);
 
                        /*
@@ -7337,13 +7340,12 @@ StartupXLOG(void)
        LWLockRelease(ProcArrayLock);
 
        /*
-        * Start up the commit log, commit timestamp and subtrans, if not already
-        * done for hot standby.
+        * Start up the commit log and subtrans, if not already done for hot
+        * standby.  (commit timestamps are started below, if necessary.)
         */
        if (standbyState == STANDBY_DISABLED)
        {
                StartupCLOG();
-               StartupCommitTs(track_commit_timestamp);
                StartupSUBTRANS(oldestActiveXID);
        }
 

diff --git a/src/include/access/commit_ts.h b/src/include/access/commit_ts.h
index f5b39691e71e2fe152a23361fa94952e47bd45d6..425f25870fd6dedb2c5c0c4a29ebd5e98faeae52 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 enabled);
+extern void StartupCommitTs(void);
 extern void CommitTsParameterChange(bool xlrecvalue, bool pgcontrolvalue);
 extern void CompleteCommitTsInitialization(void);
 extern void ShutdownCommitTs(void);