Daniel Kinzler has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/398884 )
Change subject: Fetch revisions from master DB if not found on replica.
......................................................................
Fetch revisions from master DB if not found on replica.
This overhauls the logic that determins when we will fall back
to loading a revision from the master databasase.
Change-Id: I316a1c25ecfe9fd9d0e2d0acdef3be6baec6417a
---
M includes/Storage/RevisionStore.php
1 file changed, 97 insertions(+), 45 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/84/398884/1
diff --git a/includes/Storage/RevisionStore.php
b/includes/Storage/RevisionStore.php
index b8debb8..cb0e11a 100644
--- a/includes/Storage/RevisionStore.php
+++ b/includes/Storage/RevisionStore.php
@@ -842,15 +842,18 @@
*
* MCR migration note: this replaces Revision::newFromId
*
- * $flags include:
- * IDBAccessObject::READ_LATEST: Select the data from the master
- * IDBAccessObject::READ_LOCKING : Select & lock the data from the
master
+ * Depending on $flags, this method may try to load the revision from a
replica
+ * before hitting the master database, or try the master database
immediately.
*
* @param int $id
* @param int $flags (optional)
* @return RevisionRecord|null
*/
public function getRevisionById( $id, $flags = 0 ) {
+ // Since we know the revision ID, we are pretty sure it exists,
so we want to fall
+ // back to hitting master if it'S not found on a replica, even
if no writes were done
+ // in this request. READ_LATEST_IMMUTABLE forces this.
+ $flags |= self::READ_LATEST_IMMUTABLE;
return $this->newRevisionFromConds( [ 'rev_id' => intval( $id )
], $flags );
}
@@ -861,9 +864,9 @@
*
* MCR migration note: this replaces Revision::newFromTitle
*
- * $flags include:
- * IDBAccessObject::READ_LATEST: Select the data from the master
- * IDBAccessObject::READ_LOCKING : Select & lock the data from the
master
+ * Depending on $flags, this method may try to load the revision from a
replica
+ * before hitting the master database, try the master database
immediately,
+ * or give up after trying only the replica.
*
* @param LinkTarget $linkTarget
* @param int $revId (optional)
@@ -881,14 +884,16 @@
// and fall back to master if the page is not found on
a replica.
// Since the caller supplied a revision ID, we are
pretty sure the revision is
// supposed to exist, so we should try hard to find it.
+ // The READ_LATEST_IMMUTABLE flag will force a retry
even if no writes were done
+ // in the present request.
+ $flags |= self::READ_LATEST_IMMUTABLE;
$conds['rev_id'] = $revId;
return $this->newRevisionFromConds( $conds, $flags );
} else {
// Use a join to get the latest revision.
- // Note that we don't use newRevisionFromConds here
because we don't want to retry
- // and fall back to master. The assumption is that we
only want to force the fallback
- // if we are quite sure the revision exists because the
caller supplied a revision ID.
- // If the page isn't found at all on a replica, it
probably simply does not exist.
+ // Note that we don't use newRevisionFromConds since we
do not need to fall back to
+ // master: page_latest is guaranteed to always point to
an existing revision on the same
+ // database server.
$db = $this->getDBConnection( ( $flags &
self::READ_LATEST ) ? DB_MASTER : DB_REPLICA );
$conds[] = 'rev_id=page_latest';
@@ -906,9 +911,9 @@
*
* MCR migration note: this replaces Revision::newFromPageId
*
- * $flags include:
- * IDBAccessObject::READ_LATEST: Select the data from the master
(since 1.20)
- * IDBAccessObject::READ_LOCKING : Select & lock the data from the
master
+ * Depending on $flags, this method may try to load the revision from a
replica
+ * before hitting the master database, try the master database
immediately,
+ * or give up after trying only the replica.
*
* @param int $pageId
* @param int $revId (optional)
@@ -923,14 +928,16 @@
// and fall back to master if the page is not found on
a replica.
// Since the caller supplied a revision ID, we are
pretty sure the revision is
// supposed to exist, so we should try hard to find it.
+ // The READ_LATEST_IMMUTABLE flag will force a retry
even if no writes were done
+ // in the present request.
+ $flags |= self::READ_LATEST_IMMUTABLE;
$conds['rev_id'] = $revId;
return $this->newRevisionFromConds( $conds, $flags );
} else {
// Use a join to get the latest revision.
- // Note that we don't use newRevisionFromConds here
because we don't want to retry
- // and fall back to master. The assumption is that we
only want to force the fallback
- // if we are quite sure the revision exists because the
caller supplied a revision ID.
- // If the page isn't found at all on a replica, it
probably simply does not exist.
+ // Note that we don't use newRevisionFromConds since we
do not need to fall back to
+ // master: page_latest is guaranteed to always point to
an existing revision on the same
+ // database server.
$db = $this->getDBConnection( ( $flags &
self::READ_LATEST ) ? DB_MASTER : DB_REPLICA );
$conds[] = 'rev_id=page_latest';
@@ -1394,8 +1401,11 @@
* Given a set of conditions, fetch a revision
*
* This method should be used if we are pretty sure the revision exists.
- * Unless $flags has READ_LATEST set, this method will first try to
find the revision
- * on a replica before hitting the master database.
+ * If READ_LATEST is set in $flags, only the master database is tried.
+ * If READ_LATEST is not set in $flags, this method will first try to
find the revision
+ * on a replica; if the revision is not found and READ_LATEST_IMMUTABLE
is set or
+ * the database was updated during the present request, this method
will try to find the
+ * revision on the master database.
*
* MCR migration note: this corresponds to Revision::newFromConds
*
@@ -1406,26 +1416,63 @@
* @return RevisionRecord|null
*/
private function newRevisionFromConds( $conditions, $flags = 0, Title
$title = null ) {
- $db = $this->getDBConnection( ( $flags & self::READ_LATEST ) ?
DB_MASTER : DB_REPLICA );
- $rev = $this->loadRevisionFromConds( $db, $conditions, $flags,
$title );
- $this->releaseDBConnection( $db );
+ $row = $this->findRevisionRowFromConds( $conditions, $flags );
+ if ( $row ) {
+ $rev = $this->newRevisionFromRow( $row, $flags, $title
);
- $lb = $this->getDBLoadBalancer();
-
- // Make sure new pending/committed revision are visibile later
on
- // within web requests to certain avoid bugs like T93866 and
T94407.
- if ( !$rev
- && !( $flags & self::READ_LATEST )
- && $lb->getServerCount() > 1
- && $lb->hasOrMadeRecentMasterChanges()
- ) {
- $flags = self::READ_LATEST;
- $db = $this->getDBConnection( DB_MASTER );
- $rev = $this->loadRevisionFromConds( $db, $conditions,
$flags, $title );
- $this->releaseDBConnection( $db );
+ return $rev;
}
- return $rev;
+ return null;
+ }
+
+ /**
+ * Given a set of conditions, return a row with the
+ * fields necessary to build RevisionRecord objects.
+ *
+ * If READ_LATEST is set in $flags, only the master database is tried.
+ * If READ_LATEST is not set in $flags, this method will first try to
find the revision
+ * on a replica; if the revision is not found and READ_LATEST_IMMUTABLE
is set, or
+ * the database was updated during the present request, this method
will try to find the
+ * revision on the master database.
+ *
+ * MCR migration note: this corresponds to Revision::newFromConds
+ *
+ * @param array $conditions
+ * @param int $flags (optional)
+ * @param array &$cacheOpts Cache options, as an associative array;
will be updated with
+ * database lag information according to
Database::getCacheSetOptions.
+ *
+ * @return false|object data row as a raw object
+ */
+ private function findRevisionRowFromConds( $conditions, $flags = 0,
array &$cacheOpts = [] ) {
+ $db = $this->getDBConnection( ( $flags & self::READ_LATEST ) ?
DB_MASTER : DB_REPLICA );
+ $row = $this->selectRevisionRowFromConds( $db, $conditions,
$flags );
+ $this->releaseDBConnection( $db );
+
+ if ( $row ) {
+ $cacheOpts += Database::getCacheSetOptions( $db );
+ } else {
+ $lb = $this->getDBLoadBalancer();
+
+ // Make sure new pending/committed revision are
visibile later on
+ // within web requests to certain avoid bugs like
T93866 and T94407.
+ if ( !( $flags & self::READ_LATEST )
+ && $lb->getServerCount() > 1
+ && ( $lb->hasOrMadeRecentMasterChanges() || (
$flags & self::READ_LATEST_IMMUTABLE ) )
+ ) {
+ $flags = self::READ_LATEST;
+ $db = $this->getDBConnection( DB_MASTER );
+ $row = $this->selectRevisionRowFromConds( $db,
$conditions, $flags );
+ $this->releaseDBConnection( $db );
+ }
+
+ if ( $row ) {
+ $cacheOpts += Database::getCacheSetOptions( $db
);
+ }
+ }
+
+ return $row;
}
/**
@@ -1447,7 +1494,7 @@
$flags = 0,
Title $title = null
) {
- $row = $this->fetchRevisionRowFromConds( $db, $conditions,
$flags );
+ $row = $this->selectRevisionRowFromConds( $db, $conditions,
$flags );
if ( $row ) {
$rev = $this->newRevisionFromRow( $row, $flags, $title
);
@@ -1455,6 +1502,15 @@
}
return null;
+ }
+
+ /**
+ * Returns the domain ID for the wiki this RevisionStore is associated
with
+ *
+ * @return string
+ */
+ private function getDBDomainID() {
+ return $this->wikiId ?: wfWikiID();
}
/**
@@ -1494,7 +1550,7 @@
*
* @return object|false data row as a raw object
*/
- private function fetchRevisionRowFromConds( IDatabase $db, $conditions,
$flags = 0 ) {
+ private function selectRevisionRowFromConds( IDatabase $db,
$conditions, $flags = 0 ) {
$this->checkDatabaseWikiId( $db );
$revQuery = self::getQueryInfo( [ 'page', 'user' ] );
@@ -1863,8 +1919,6 @@
* @return RevisionRecord|bool Returns false if missing
*/
public function getKnownCurrentRevision( Title $title, $revId ) {
- $db = $this->getDBConnectionRef( DB_REPLICA );
-
$pageId = $title->getArticleID();
if ( !$pageId ) {
@@ -1885,18 +1939,16 @@
$row = $this->cache->getWithSetCallback(
// Page/rev IDs passed in from DB to reflect history
merges
- $this->cache->makeGlobalKey( 'revision-row-1.29',
$db->getDomainID(), $pageId, $revId ),
+ $this->cache->makeGlobalKey( 'revision-row-1.29',
$this->getDBDomainID(), $pageId, $revId ),
WANObjectCache::TTL_WEEK,
- function ( $curValue, &$ttl, array &$setOpts ) use (
$db, $pageId, $revId ) {
- $setOpts += Database::getCacheSetOptions( $db );
-
+ function ( $curValue, &$ttl, array &$setOpts ) use (
$pageId, $revId ) {
$conds = [
'rev_page' => intval( $pageId ),
'page_id' => intval( $pageId ),
'rev_id' => intval( $revId ),
];
- $row = $this->fetchRevisionRowFromConds( $db,
$conds );
+ $row = $this->findRevisionRowFromConds( $conds,
self::READ_LATEST_IMMUTABLE, $setOpts );
return $row ?: false; // don't cache negatives
}
);
--
To view, visit https://gerrit.wikimedia.org/r/398884
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I316a1c25ecfe9fd9d0e2d0acdef3be6baec6417a
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
