Revision and WikiPage cleanup with IDBAccessObject interface.
authorAaron Schulz <aschulz@wikimedia.org>
Wed, 8 Aug 2012 06:22:49 +0000 (23:22 -0700)
committerGerrit Code Review <gerrit@wikimedia.org>
Wed, 8 Aug 2012 16:34:08 +0000 (16:34 +0000)
* 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

includes/Revision.php
includes/WikiPage.php
includes/dao/IDBAccessObject.php
includes/diff/DifferenceEngine.php
includes/filerepo/file/LocalFile.php
includes/job/RefreshLinksJob.php
includes/parser/Parser.php
includes/search/SearchEngine.php
includes/specials/SpecialBooksources.php

index 13eaae4..731a5f2 100644 (file)
@@ -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(
index 1378ce1..764a831 100644 (file)
@@ -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 );
index cd5dda9..e30522a 100644 (file)
  */
 
 /**
- * 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)
 }
index 0dbbdd4..d9092da 100644 (file)
@@ -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;
index 4db1f5f..1a86e71 100644 (file)
@@ -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;
index 2eac326..6b8dede 100644 (file)
@@ -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() . '"';
index 28957df..66da408 100644 (file)
@@ -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 ) {
index a8e0437..b95499f 100644 (file)
@@ -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 );
                }
index 87ba0cb..bf7de3f 100644 (file)
@@ -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;
                }