From 69e7235c93e2965cc0e17186bd044e4c54997c19 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 11 Dec 2015 14:30:43 -0300 Subject: 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. --- src/backend/access/transam/commit_ts.c | 39 +++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 12 deletions(-) (limited to 'src/backend/access/transam/commit_ts.c') diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c index 834010ff716..8f09dc83ae7 100644 --- 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. -- cgit v1.2.3