From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 8 Aug 2012 06:22:49 +0000 (-0700)
Subject: Revision and WikiPage cleanup with IDBAccessObject interface.
X-Git-Tag: 1.31.0-rc.0~22801
X-Git-Url: http://git.cyclocoop.org/url?a=commitdiff_plain;h=eb183bac87cb58ca910140a31a5f8b8c7916302f;p=lhc%2Fweb%2Fwiklou.git

Revision and WikiPage cleanup with IDBAccessObject interface.

* Replaced WikiPage::DATA_FROM_* constants with IDBAccessObject ones.
* Renamed IDBAccessObject constants a bit for visual consistency.
* Removed AVOID_MASTER parameter and replaced calling instances with READ_NORMAL.
  Instead of getting page_latest from the master and the revision from a
  slave, just get it all from the master in one RTT. Most callers used
  AVOID_MASTER (and now READ_NORMAL), so this case is barely hit anymore.

Change-Id: Ifbefdcd4490094b38e49bbb46c95fdb71b5c9e1a
---

diff --git a/includes/Revision.php b/includes/Revision.php
index 13eaae4ef0..731a5f2b09 100644
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -41,12 +41,13 @@ class Revision implements IDBAccessObject {
 	protected $mTitle;
 	protected $mCurrent;
 
+	// Revision deletion constants
 	const DELETED_TEXT = 1;
 	const DELETED_COMMENT = 2;
 	const DELETED_USER = 4;
 	const DELETED_RESTRICTED = 8;
-	// Convenience field
-	const SUPPRESSED_USER = 12;
+	const SUPPRESSED_USER = 12; // convenience
+
 	// Audience options for accessors
 	const FOR_PUBLIC = 1;
 	const FOR_THIS_USER = 2;
@@ -57,9 +58,8 @@ class Revision implements IDBAccessObject {
 	 * Returns null if no such revision can be found.
 	 *
 	 * $flags include:
-	 *      IDBAccessObject::LATEST_READ  : Select the data from the master
-	 *      IDBAccessObject::LOCKING_READ : Select & lock the data from the master
-	 *      IDBAccessObject::AVOID_MASTER : Avoid master queries; data may be stale
+	 *      Revision::READ_LATEST  : Select the data from the master
+	 *      Revision::READ_LOCKING : Select & lock the data from the master
 	 *
 	 * @param $id Integer
 	 * @param $flags Integer (optional)
@@ -75,16 +75,15 @@ class Revision implements IDBAccessObject {
 	 * to that title, will return null.
 	 *
 	 * $flags include:
-	 *      IDBAccessObject::LATEST_READ  : Select the data from the master
-	 *      IDBAccessObject::LOCKING_READ : Select & lock the data from the master
-	 *      IDBAccessObject::AVOID_MASTER : Avoid master queries; data may be stale
+	 *      Revision::READ_LATEST  : Select the data from the master
+	 *      Revision::READ_LOCKING : Select & lock the data from the master
 	 *
 	 * @param $title Title
 	 * @param $id Integer (optional)
 	 * @param $flags Integer Bitfield (optional)
 	 * @return Revision or null
 	 */
-	public static function newFromTitle( $title, $id = 0, $flags = 0 ) {
+	public static function newFromTitle( $title, $id = 0, $flags = null ) {
 		$conds = array(
 			'page_namespace' => $title->getNamespace(),
 			'page_title' 	 => $title->getDBkey()
@@ -92,19 +91,13 @@ class Revision implements IDBAccessObject {
 		if ( $id ) {
 			// Use the specified ID
 			$conds['rev_id'] = $id;
-		} elseif ( !( $flags & self::AVOID_MASTER ) && wfGetLB()->getServerCount() > 1 ) {
-			// Get the latest revision ID from the master
-			$dbw = wfGetDB( DB_MASTER );
-			$latest = $dbw->selectField( 'page', 'page_latest', $conds, __METHOD__ );
-			if ( $latest === false ) {
-				return null; // page does not exist
-			}
-			$conds['rev_id'] = $latest;
 		} else {
 			// Use a join to get the latest revision
 			$conds[] = 'rev_id=page_latest';
+			// Callers assume this will be up-to-date
+			$flags = is_int( $flags ) ? $flags : self::READ_LATEST; // b/c
 		}
-		return self::newFromConds( $conds, $flags );
+		return self::newFromConds( $conds, (int)$flags );
 	}
 
 	/**
@@ -113,31 +106,25 @@ class Revision implements IDBAccessObject {
 	 * Returns null if no such revision can be found.
 	 *
 	 * $flags include:
-	 *      IDBAccessObject::LATEST_READ  : Select the data from the master
-	 *      IDBAccessObject::LOCKING_READ : Select & lock the data from the master
-	 *      IDBAccessObject::AVOID_MASTER : Avoid master queries; data may be stale
+	 *      Revision::READ_LATEST  : Select the data from the master
+	 *      Revision::READ_LOCKING : Select & lock the data from the master
 	 *
 	 * @param $revId Integer
 	 * @param $pageId Integer (optional)
 	 * @param $flags Integer Bitfield (optional)
 	 * @return Revision or null
 	 */
-	public static function newFromPageId( $pageId, $revId = 0, $flags = 0 ) {
+	public static function newFromPageId( $pageId, $revId = 0, $flags = null ) {
 		$conds = array( 'page_id' => $pageId );
 		if ( $revId ) {
 			$conds['rev_id'] = $revId;
-		} elseif ( !( $flags & self::AVOID_MASTER ) && wfGetLB()->getServerCount() > 1 ) {
-			// Get the latest revision ID from the master
-			$dbw = wfGetDB( DB_MASTER );
-			$latest = $dbw->selectField( 'page', 'page_latest', $conds, __METHOD__ );
-			if ( $latest === false ) {
-				return null; // page does not exist
-			}
-			$conds['rev_id'] = $latest;
 		} else {
+			// Use a join to get the latest revision
 			$conds[] = 'rev_id = page_latest';
+			// Callers assume this will be up-to-date
+			$flags = is_int( $flags ) ? $flags : self::READ_LATEST; // b/c
 		}
-		return self::newFromConds( $conds, $flags );
+		return self::newFromConds( $conds, (int)$flags );
 	}
 
 	/**
@@ -265,10 +252,10 @@ class Revision implements IDBAccessObject {
 	 * @return Revision or null
 	 */
 	private static function newFromConds( $conditions, $flags = 0 ) {
-		$db = wfGetDB( ( $flags & self::LATEST_READ ) ? DB_MASTER : DB_SLAVE );
+		$db = wfGetDB( ( $flags & self::READ_LATEST ) ? DB_MASTER : DB_SLAVE );
 		$rev = self::loadFromConds( $db, $conditions, $flags );
 		if ( is_null( $rev ) && wfGetLB()->getServerCount() > 1 ) {
-			if ( !( $flags & self::LATEST_READ ) && !( $flags & self::AVOID_MASTER ) ) {
+			if ( !( $flags & self::READ_LATEST ) ) {
 				$dbw = wfGetDB( DB_MASTER );
 				$rev = self::loadFromConds( $dbw, $conditions, $flags );
 			}
@@ -332,7 +319,7 @@ class Revision implements IDBAccessObject {
 			self::selectUserFields()
 		);
 		$options = array( 'LIMIT' => 1 );
-		if ( $flags & self::FOR_UPDATE ) {
+		if ( $flags & self::READ_LOCKING ) {
 			$options[] = 'FOR UPDATE';
 		}
 		return $db->select(
diff --git a/includes/WikiPage.php b/includes/WikiPage.php
index 1378ce1e23..764a831b92 100644
--- a/includes/WikiPage.php
+++ b/includes/WikiPage.php
@@ -33,29 +33,9 @@ abstract class Page {}
  *
  * @internal documentation reviewed 15 Mar 2010
  */
-class WikiPage extends Page {
+class WikiPage extends Page implements IDBAccessObject {
 	// Constants for $mDataLoadedFrom and related
 
-	/**
-	 * Data has not been loaded yet (or the object was cleared)
-	 */
-	const DATA_NOT_LOADED = 0;
-
-	/**
-	 * Data has been loaded from a slave database
-	 */
-	const DATA_FROM_SLAVE = 1;
-
-	/**
-	 * Data has been loaded from the master database
-	 */
-	const DATA_FROM_MASTER = 2;
-
-	/**
-	 * Data has been loaded from the master database using FOR UPDATE
-	 */
-	const DATA_FOR_UPDATE = 3;
-
 	/**
 	 * @var Title
 	 */
@@ -71,9 +51,9 @@ class WikiPage extends Page {
 	/**@}}*/
 
 	/**
-	 * @var int; one of the DATA_* constants
+	 * @var int; one of the READ_* constants
 	 */
-	protected $mDataLoadedFrom = self::DATA_NOT_LOADED;
+	protected $mDataLoadedFrom = self::READ_NONE;
 
 	/**
 	 * @var Title
@@ -142,14 +122,14 @@ class WikiPage extends Page {
 	 *
 	 * @param $id Int article ID to load
 	 * @param $from string|int one of the following values:
-	 *        - "fromdb" or self::DATA_FROM_SLAVE to select from a slave database
-	 *        - "fromdbmaster" or self::DATA_FROM_MASTER to select from the master database
+	 *        - "fromdb" or WikiPage::READ_NORMAL to select from a slave database
+	 *        - "fromdbmaster" or WikiPage::READ_LATEST to select from the master database
 	 *
 	 * @return WikiPage|null
 	 */
 	public static function newFromID( $id, $from = 'fromdb' ) {
 		$from = self::convertSelectType( $from );
-		$db = wfGetDB( $from === self::DATA_FROM_MASTER ? DB_MASTER : DB_SLAVE );
+		$db = wfGetDB( $from === self::READ_LATEST ? DB_MASTER : DB_SLAVE );
 		$row = $db->selectRow( 'page', self::selectFields(), array( 'page_id' => $id ), __METHOD__ );
 		if ( !$row ) {
 			return null;
@@ -164,9 +144,9 @@ class WikiPage extends Page {
 	 * @param $row object: database row containing at least fields returned
 	 *        by selectFields().
 	 * @param $from string|int: source of $data:
-	 *        - "fromdb" or self::DATA_FROM_SLAVE: from a slave DB
-	 *        - "fromdbmaster" or self::DATA_FROM_MASTER: from the master DB
-	 *        - "forupdate" or self::DATA_FOR_UPDATE: from the master DB using SELECT FOR UPDATE
+	 *        - "fromdb" or WikiPage::READ_NORMAL: from a slave DB
+	 *        - "fromdbmaster" or WikiPage::READ_LATEST: from the master DB
+	 *        - "forupdate" or WikiPage::READ_LOCKING: from the master DB using SELECT FOR UPDATE
 	 * @return WikiPage
 	 */
 	public static function newFromRow( $row, $from = 'fromdb' ) {
@@ -176,7 +156,7 @@ class WikiPage extends Page {
 	}
 
 	/**
-	 * Convert 'fromdb', 'fromdbmaster' and 'forupdate' to DATA_* constants.
+	 * Convert 'fromdb', 'fromdbmaster' and 'forupdate' to READ_* constants.
 	 *
 	 * @param $type object|string|int
 	 * @return mixed
@@ -184,11 +164,11 @@ class WikiPage extends Page {
 	private static function convertSelectType( $type ) {
 		switch ( $type ) {
 		case 'fromdb':
-			return self::DATA_FROM_SLAVE;
+			return self::READ_NORMAL;
 		case 'fromdbmaster':
-			return self::DATA_FROM_MASTER;
+			return self::READ_LATEST;
 		case 'forupdate':
-			return self::DATA_FOR_UPDATE;
+			return self::READ_LOCKING;
 		default:
 			// It may already be an integer or whatever else
 			return $type;
@@ -223,7 +203,7 @@ class WikiPage extends Page {
 	 */
 	public function clear() {
 		$this->mDataLoaded = false;
-		$this->mDataLoadedFrom = self::DATA_NOT_LOADED;
+		$this->mDataLoadedFrom = self::READ_NONE;
 
 		$this->clearCacheFields();
 	}
@@ -317,9 +297,9 @@ class WikiPage extends Page {
 	 *
 	 * @param $from object|string|int One of the following:
 	 *        - A DB query result object
-	 *        - "fromdb" or self::DATA_FROM_SLAVE to get from a slave DB
-	 *        - "fromdbmaster" or self::DATA_FROM_MASTER to get from the master DB
-	 *        - "forupdate"  or self::DATA_FOR_UPDATE to get from the master DB using SELECT FOR UPDATE
+	 *        - "fromdb" or WikiPage::READ_NORMAL to get from a slave DB
+	 *        - "fromdbmaster" or WikiPage::READ_LATEST to get from the master DB
+	 *        - "forupdate"  or WikiPage::READ_LOCKING to get from the master DB using SELECT FOR UPDATE
 	 *
 	 * @return void
 	 */
@@ -330,25 +310,25 @@ class WikiPage extends Page {
 			return;
 		}
 
-		if ( $from === self::DATA_FOR_UPDATE ) {
+		if ( $from === self::READ_LOCKING ) {
 			$data = $this->pageDataFromTitle( wfGetDB( DB_MASTER ), $this->mTitle, array( 'FOR UPDATE' ) );
-		} elseif ( $from === self::DATA_FROM_MASTER ) {
+		} elseif ( $from === self::READ_LATEST ) {
 			$data = $this->pageDataFromTitle( wfGetDB( DB_MASTER ), $this->mTitle );
-		} elseif ( $from === self::DATA_FROM_SLAVE ) {
+		} elseif ( $from === self::READ_NORMAL ) {
 			$data = $this->pageDataFromTitle( wfGetDB( DB_SLAVE ), $this->mTitle );
 			# Use a "last rev inserted" timestamp key to dimish the issue of slave lag.
 			# Note that DB also stores the master position in the session and checks it.
 			$touched = $this->getCachedLastEditTime();
 			if ( $touched ) { // key set
 				if ( !$data || $touched > wfTimestamp( TS_MW, $data->page_touched ) ) {
-					$from = self::DATA_FROM_MASTER;
+					$from = self::READ_LATEST;
 					$data = $this->pageDataFromTitle( wfGetDB( DB_MASTER ), $this->mTitle );
 				}
 			}
 		} else {
 			// No idea from where the caller got this data, assume slave database.
 			$data = $from;
-			$from = self::DATA_FROM_SLAVE;
+			$from = self::READ_NORMAL;
 		}
 
 		$this->loadFromRow( $data, $from );
@@ -361,9 +341,9 @@ class WikiPage extends Page {
 	 * @param $data object: database row containing at least fields returned
 	 *        by selectFields()
 	 * @param $from string|int One of the following:
-	 *        - "fromdb" or self::DATA_FROM_SLAVE if the data comes from a slave DB
-	 *        - "fromdbmaster" or self::DATA_FROM_MASTER if the data comes from the master DB
-	 *        - "forupdate"  or self::DATA_FOR_UPDATE if the data comes from from
+	 *        - "fromdb" or WikiPage::READ_NORMAL if the data comes from a slave DB
+	 *        - "fromdbmaster" or WikiPage::READ_LATEST if the data comes from the master DB
+	 *        - "forupdate"  or WikiPage::READ_LOCKING if the data comes from from
 	 *          the master DB using SELECT FOR UPDATE
 	 */
 	public function loadFromRow( $data, $from ) {
@@ -546,7 +526,7 @@ class WikiPage extends Page {
 		// also gets the revision row FOR UPDATE; otherwise, it may not find it since a page row
 		// UPDATE and revision row INSERT by S2 may have happened after the first S1 SELECT.
 		// http://dev.mysql.com/doc/refman/5.0/en/set-transaction.html#isolevel_repeatable-read.
-		$flags = ( $this->mDataLoadedFrom == self::DATA_FOR_UPDATE ) ? Revision::LOCKING_READ : 0;
+		$flags = ( $this->mDataLoadedFrom == self::READ_LOCKING ) ? Revision::READ_LOCKING : 0;
 		$revision = Revision::newFromPageId( $this->getId(), $latest, $flags );
 		if ( $revision ) { // sanity
 			$this->setLastEdit( $revision );
diff --git a/includes/dao/IDBAccessObject.php b/includes/dao/IDBAccessObject.php
index cd5dda92da..e30522a552 100644
--- a/includes/dao/IDBAccessObject.php
+++ b/includes/dao/IDBAccessObject.php
@@ -22,11 +22,34 @@
  */
 
 /**
- * Interface for database access objects
+ * Interface for database access objects.
+ *
+ * Classes using this support a set of constants in a bitfield argument to their data loading
+ * functions. In general, objects should assume READ_NORMAL if no flags are explicitly given,
+ * though certain objects may assume READ_LATEST for common use case or legacy reasons.
+ *
+ * There are three types of reads:
+ *   - READ_NORMAL  : Potentially cached read of data (e.g. from a slave or stale replica)
+ *   - READ_LATEST  : Up-to-date read as of transaction start (e.g. from master or a quorum read)
+ *   - READ_LOCKING : Up-to-date read as of now, that locks the records for the transaction
+ *
+ * Callers should use READ_NORMAL (or pass in no flags) unless the read determines a write.
+ * In theory, such cases may require READ_LOCKING, though to avoid contention, READ_LATEST is
+ * often good enough. If UPDATE race condition checks are required on a row and expensive code
+ * must run after the row is fetched to determine the UPDATE, it may help to do something like:
+ *   - a) Read the current row
+ *   - b) Determine the new row (expensive, so we don't want to hold locks now)
+ *   - c) Re-read the current row with READ_LOCKING; if it changed then bail out
+ *   - d) otherwise, do the updates
  */
 interface IDBAccessObject {
-	const LATEST_READ  = 1; // read from the master
-	const FOR_UPDATE   = 2; // lock the rows read
-	const LOCKING_READ = 3; // LATEST_READ | FOR_UPDATE
-	const AVOID_MASTER = 4; // avoiding checking the master
+	// Constants for object loading bitfield flags (higher => higher QoS)
+	const READ_LATEST  = 1; // read from the master
+	const READ_LOCKING = 3; // READ_LATEST and "FOR UPDATE"
+
+	// Convenience constant for callers to explicitly request slave data
+	const READ_NORMAL = 0; // read from the slave
+
+	// Convenience constant for tracking how data was loaded (higher => higher QoS)
+	const READ_NONE = -1; // not loaded yet (or the object was cleared)
 }
diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 0dbbdd46a8..d9092da863 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -1030,7 +1030,7 @@ class DifferenceEngine extends ContextSource {
 		// Load the new revision object
 		$this->mNewRev = $this->mNewid
 			? Revision::newFromId( $this->mNewid )
-			: Revision::newFromTitle( $this->getTitle(), false, Revision::AVOID_MASTER );
+			: Revision::newFromTitle( $this->getTitle(), false, Revision::READ_NORMAL );
 
 		if ( !$this->mNewRev instanceof Revision ) {
 			return false;
diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index 4db1f5fdad..1a86e71547 100644
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -1456,7 +1456,7 @@ class LocalFile extends File {
 	 */
 	function getDescriptionText() {
 		global $wgParser;
-		$revision = Revision::newFromTitle( $this->title, false, Revision::AVOID_MASTER );
+		$revision = Revision::newFromTitle( $this->title, false, Revision::READ_NORMAL );
 		if ( !$revision ) return false;
 		$text = $revision->getText();
 		if ( !$text ) return false;
diff --git a/includes/job/RefreshLinksJob.php b/includes/job/RefreshLinksJob.php
index 2eac3264a8..6b8dede820 100644
--- a/includes/job/RefreshLinksJob.php
+++ b/includes/job/RefreshLinksJob.php
@@ -55,7 +55,7 @@ class RefreshLinksJob extends Job {
 			wfGetLB()->waitFor( $this->params['masterPos'] );
 		}
 
-		$revision = Revision::newFromTitle( $this->title, 0, Revision::AVOID_MASTER );
+		$revision = Revision::newFromTitle( $this->title, 0, Revision::READ_NORMAL );
 		if ( !$revision ) {
 			$this->error = 'refreshLinks: Article not found "' .
 				$this->title->getPrefixedDBkey() . '"';
@@ -185,7 +185,7 @@ class RefreshLinksJob2 extends Job {
 			}
 			# Re-parse each page that transcludes this page and update their tracking links...
 			foreach ( $titles as $title ) {
-				$revision = Revision::newFromTitle( $title, 0, Revision::AVOID_MASTER );
+				$revision = Revision::newFromTitle( $title, 0, Revision::READ_NORMAL );
 				if ( !$revision ) {
 					$this->error = 'refreshLinks: Article not found "' .
 						$title->getPrefixedDBkey() . '"';
diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index 28957dfb47..66da408872 100644
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -3552,7 +3552,7 @@ class Parser {
 			# Get the revision
 			$rev = $id
 				? Revision::newFromId( $id )
-				: Revision::newFromTitle( $title, 0, Revision::AVOID_MASTER );
+				: Revision::newFromTitle( $title, 0, Revision::READ_NORMAL );
 			$rev_id = $rev ? $rev->getId() : 0;
 			# If there is no current revision, there is no page
 			if ( $id === false && !$rev ) {
diff --git a/includes/search/SearchEngine.php b/includes/search/SearchEngine.php
index a8e04374ff..b95499f6bd 100644
--- a/includes/search/SearchEngine.php
+++ b/includes/search/SearchEngine.php
@@ -757,7 +757,7 @@ class SearchResult {
 		$this->mTitle = $title;
 		if ( !is_null( $this->mTitle ) ) {
 			$this->mRevision = Revision::newFromTitle(
-				$this->mTitle, false, Revision::AVOID_MASTER );
+				$this->mTitle, false, Revision::READ_NORMAL );
 			if ( $this->mTitle->getNamespace() === NS_FILE )
 				$this->mImage = wfFindFile( $this->mTitle );
 		}
diff --git a/includes/specials/SpecialBooksources.php b/includes/specials/SpecialBooksources.php
index 87ba0cb1a6..bf7de3f5a5 100644
--- a/includes/specials/SpecialBooksources.php
+++ b/includes/specials/SpecialBooksources.php
@@ -143,7 +143,7 @@ class SpecialBookSources extends SpecialPage {
 		$page = $this->msg( 'booksources' )->inContentLanguage()->text();
 		$title = Title::makeTitleSafe( NS_PROJECT, $page ); # Show list in content language
 		if( is_object( $title ) && $title->exists() ) {
-			$rev = Revision::newFromTitle( $title, false, Revision::AVOID_MASTER );
+			$rev = Revision::newFromTitle( $title, false, Revision::READ_NORMAL );
 			$this->getOutput()->addWikiText( str_replace( 'MAGICNUMBER', $this->isbn, $rev->getText() ) );
 			return true;
 		}