From 92972a59cdc19cf044fe4c302036871892cbc10a Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 2 Apr 2018 14:39:33 -0700
Subject: [PATCH] rdbms: rename and clarify getTransactionLagStatus method
 regarding begin()

Make sure any value from the last transaction is cleared and not used.

For sanity, make getLagFromPtHeartbeat() use this regardless of the age of
the transaction unless it returns null.

Change-Id: I52df6147f99736ad1a389ae70d347ae968e50c7f
---
 includes/libs/rdbms/database/Database.php     |  9 ++++---
 .../libs/rdbms/database/DatabaseMysqlBase.php | 24 ++++++++++---------
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 699fee7e74..8739cbf706 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -3393,6 +3393,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		$this->trxWriteCallers = [];
 		// First SELECT after BEGIN will establish the snapshot in REPEATABLE-READ.
 		// Get an estimate of the replication lag before any such queries.
+		$this->trxReplicaLag = null; // clear cached value first
 		$this->trxReplicaLag = $this->getApproximateLagStatus()['lag'];
 		// T147697: make explicitTrxActive() return true until begin() finishes. This way, no
 		// caller will think its OK to muck around with the transaction just because startAtomic()
@@ -3666,7 +3667,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	}
 
 	public function getSessionLagStatus() {
-		return $this->getTransactionLagStatus() ?: $this->getApproximateLagStatus();
+		return $this->getRecordedTransactionLagStatus() ?: $this->getApproximateLagStatus();
 	}
 
 	/**
@@ -3677,11 +3678,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	 * is this lag plus transaction duration. If they don't, it is still
 	 * safe to be pessimistic. This returns null if there is no transaction.
 	 *
+	 * This returns null if the lag status for this transaction was not yet recorded.
+	 *
 	 * @return array|null ('lag': seconds or false on error, 'since': UNIX timestamp of BEGIN)
 	 * @since 1.27
 	 */
-	final protected function getTransactionLagStatus() {
-		return $this->trxLevel
+	final protected function getRecordedTransactionLagStatus() {
+		return ( $this->trxLevel && $this->trxReplicaLag !== null )
 			? [ 'lag' => $this->trxReplicaLag, 'since' => $this->trxTimestamp() ]
 			: null;
 	}
diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php
index 0a63bdc6f1..86ec641668 100644
--- a/includes/libs/rdbms/database/DatabaseMysqlBase.php
+++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php
@@ -766,18 +766,20 @@ abstract class DatabaseMysqlBase extends Database {
 	protected function getLagFromPtHeartbeat() {
 		$options = $this->lagDetectionOptions;
 
-		$staleness = $this->trxLevel
-			? microtime( true ) - $this->trxTimestamp()
-			: 0;
-		if ( $staleness > self::LAG_STALE_WARN_THRESHOLD ) {
-			// Avoid returning higher and higher lag value due to snapshot age
-			// given that the isolation level will typically be REPEATABLE-READ
-			$this->queryLogger->warning(
-				"Using cached lag value for {db_server} due to active transaction",
-				$this->getLogContext( [ 'method' => __METHOD__ ] )
-			);
+		$currentTrxInfo = $this->getRecordedTransactionLagStatus();
+		if ( $currentTrxInfo ) {
+			// There is an active transaction and the initial lag was already queried
+			$staleness = microtime( true ) - $currentTrxInfo['since'];
+			if ( $staleness > self::LAG_STALE_WARN_THRESHOLD ) {
+				// Avoid returning higher and higher lag value due to snapshot age
+				// given that the isolation level will typically be REPEATABLE-READ
+				$this->queryLogger->warning(
+					"Using cached lag value for {db_server} due to active transaction",
+					$this->getLogContext( [ 'method' => __METHOD__, 'age' => $staleness ] )
+				);
+			}
 
-			return $this->getTransactionLagStatus()['lag'];
+			return $currentTrxInfo['lag'];
 		}
 
 		if ( isset( $options['conds'] ) ) {
-- 
2.20.1