From 8006aa946aaf1ef948323f4b717c0359f60f1c2e Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 23 Jun 2014 17:40:08 -0700
Subject: [PATCH] Avoid key conflict errors in User::addToDatabase

* Also cleaned up the IDBAccessObject constants to cover more cases.

Bug: 66185
Change-Id: Ide28af552b3c59428923b373c0f5764414d50a1f
---
 includes/User.php                | 30 +++++++++++++++++++++---------
 includes/dao/IDBAccessObject.php | 13 ++++++++-----
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/includes/User.php b/includes/User.php
index f2d3d9b18d..9b6d31b0c7 100644
--- a/includes/User.php
+++ b/includes/User.php
@@ -56,7 +56,7 @@ class PasswordError extends MWException {
  * for rendering normal pages are set in the cookie to minimize use
  * of the database.
  */
-class User {
+class User implements IDBAccessObject {
 	/**
 	 * Global constants made accessible as class constants so that autoloader
 	 * magic can be used.
@@ -1169,9 +1169,10 @@ class User {
 	 * Load user and user_group data from the database.
 	 * $this->mId must be set, this is how the user is identified.
 	 *
+	 * @param integer $flags Supports User::READ_LOCKING
 	 * @return bool True if the user exists, false if the user is anonymous
 	 */
-	public function loadFromDatabase() {
+	public function loadFromDatabase( $flags = 0 ) {
 		// Paranoia
 		$this->mId = intval( $this->mId );
 
@@ -1186,7 +1187,10 @@ class User {
 			'user',
 			self::selectFields(),
 			array( 'user_id' => $this->mId ),
-			__METHOD__
+			__METHOD__,
+			( $flags & self::READ_LOCKING == self::READ_LOCKING )
+				? array( 'LOCK IN SHARE MODE' )
+				: array()
 		);
 
 		wfRunHooks( 'UserLoadFromDatabase', array( $this, &$s ) );
@@ -3605,17 +3609,25 @@ class User {
 			array( 'IGNORE' )
 		);
 		if ( !$dbw->affectedRows() ) {
-			if ( !$inWrite ) {
-				// XXX: Get out of REPEATABLE-READ so the SELECT below works.
-				// Often this case happens early in views before any writes.
-				// This shows up at least with CentralAuth.
+			// The queries below cannot happen in the same REPEATABLE-READ snapshot.
+			// Handle this by COMMIT, if possible, or by LOCK IN SHARE MODE otherwise.
+			if ( $inWrite ) {
+				// Can't commit due to pending writes that may need atomicity.
+				// This may cause some lock contention unlike the case below.
+				$options = array( 'LOCK IN SHARE MODE' );
+				$flags = self::READ_LOCKING;
+			} else {
+				// Often, this case happens early in views before any writes when
+				// using CentralAuth. It's should be OK to commit and break the snapshot.
 				$dbw->commit( __METHOD__, 'flush' );
+				$options = array();
+				$flags = 0;
 			}
 			$this->mId = $dbw->selectField( 'user', 'user_id',
-				array( 'user_name' => $this->mName ), __METHOD__ );
+				array( 'user_name' => $this->mName ), __METHOD__, $options );
 			$loaded = false;
 			if ( $this->mId ) {
-				if ( $this->loadFromDatabase() ) {
+				if ( $this->loadFromDatabase( $flags ) ) {
 					$loaded = true;
 				}
 			}
diff --git a/includes/dao/IDBAccessObject.php b/includes/dao/IDBAccessObject.php
index 4eb6ff3ed2..3690735ed5 100644
--- a/includes/dao/IDBAccessObject.php
+++ b/includes/dao/IDBAccessObject.php
@@ -28,10 +28,12 @@
  * 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
+ * There are four 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 (shared) the records
+ *   - READ_EXCLUSIVE : Up-to-date read as of now, that locks (exclusive) the records
+ * All record locks persist for the duration of 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
@@ -47,7 +49,8 @@
 interface IDBAccessObject {
 	// 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"
+	const READ_LOCKING = 3; // READ_LATEST (1) and "LOCK IN SHARE MODE" (2)
+	const READ_EXCLUSIVE = 7; // READ_LOCKING (3) and "FOR UPDATE" (4)
 
 	// Convenience constant for callers to explicitly request slave data
 	const READ_NORMAL = 0; // read from the slave
-- 
2.20.1