nametablestore: simplify NameTableStore::acquireId() and update comments

Make store() handle cache purging upon new ID insertion and fetching
of the winning ID when raced out. This avoids the reloadMap() call.

Remove connection flag hack in reloadMap() given 0cb0f0ba7.

Remove comments about retryStore() logic removed in 505bd8e70f.

Clean up return types for the ID:name arrays.

Change-Id: Ic57eb5e2b59da67c6511b935d9e68ffb595028fa

3 files changed, 82 insertions, 77 deletions

diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php
index 7b5adec3d153..1a3cecb74137 100644
--- a/includes/Storage/NameTableStore.php
+++ b/includes/Storage/NameTableStore.php
@@ -44,7 +44,7 @@ class NameTableStore {
 	/** @var LoggerInterface */
 	private $logger;
 
-	/** @var string[] */
+	/** @var array<int,string>|null */
 	private $tableCache = null;
 
 	/** @var bool|string */
@@ -145,14 +145,12 @@ class NameTableStore {
 	 * Acquire the id of the given name.
 	 * This creates a row in the table if it doesn't already exist.
 	 *
-	 * @note If called within an atomic section, there is a chance for the acquired ID
-	 * to be lost on rollback. A best effort is made to re-insert the mapping
-	 * in this case, and consistency of the cache with the database table is ensured
-	 * by re-loading the map after a failed atomic section. However, there is no guarantee
-	 * that an ID returned by this method is valid outside the transaction in which it
-	 * was produced. This means that calling code should not retain the return value beyond
-	 * the scope of a transaction, but rather call acquireId() again after the transaction
-	 * is complete. In some rare cases, this may produce an ID different from the first call.
+	 * @note If called within an atomic section, there is a chance for the acquired ID to be
+	 * lost on rollback. There is no guarantee that an ID returned by this method is valid
+	 * outside the transaction in which it was produced. This means that calling code should
+	 * not retain the return value beyond the scope of a transaction, but rather call acquireId()
+	 * again after the transaction is complete. In some rare cases, this may produce an ID
+	 * different from the first call.
 	 *
 	 * @param string $name
 	 * @throws NameTableAccessException
@@ -165,44 +163,23 @@ class NameTableStore {
 		$searchResult = array_search( $name, $table, true );
 		if ( $searchResult === false ) {
 			$id = $this->store( $name );
-			if ( $id === null ) {
-				// RACE: $name was already in the db, probably just inserted, so load from primary DB.
-				// Use DBO_TRX to avoid missing inserts due to other threads or REPEATABLE-READs.
-				$table = $this->reloadMap( ILoadBalancer::CONN_TRX_AUTOCOMMIT );
-
-				$searchResult = array_search( $name, $table, true );
-				if ( $searchResult === false ) {
-					// Insert failed due to IGNORE flag, but DB_PRIMARY didn't give us the data
-					$m = "No insert possible but primary DB didn't give us a record for " .
-						"'{$name}' in '{$this->table}'";
-					$this->logger->error( $m );
-					throw new NameTableAccessException( $m );
-				}
-			} else {
-				if ( isset( $table[$id] ) ) {
-					// This can happen when a transaction is rolled back and acquireId is called in
-					// an onTransactionResolution() callback, which gets executed before retryStore()
-					// has a chance to run. The right thing to do in this case is to discard the old
-					// value. According to the contract of acquireId, the caller should not have
-					// used it outside the transaction, so it should not be persisted anywhere after
-					// the rollback.
-					$m = "Got ID $id for '$name' from insert"
-						. " into '{$this->table}', but ID $id was previously associated with"
-						. " the name '{$table[$id]}'. Overriding the old value, which presumably"
-						. " has been removed from the database due to a transaction rollback.";
-
-					$this->logger->warning( $m );
-				}
 
-				$table[$id] = $name;
-				$searchResult = $id;
-
-				// As store returned an ID we know we inserted so delete from WAN cache
-				$dbw = $this->getDBConnection( DB_PRIMARY );
-				$dbw->onTransactionPreCommitOrIdle( function () {
-					$this->cache->delete( $this->getCacheKey(), WANObjectCache::HOLDOFF_TTL_NONE );
-				}, __METHOD__ );
+			if ( isset( $table[$id] ) ) {
+				// This can happen when a name is assigned an ID within a transaction due to
+				// CONN_TRX_AUTOCOMMIT being unable to use a separate connection (e.g. SQLite).
+				// The right thing to do in this case is to discard the old value. According to
+				// the contract of acquireId, the caller should not have used it outside the
+				// transaction, so it should not be persisted anywhere after the rollback.
+				$m = "Got ID $id for '$name' from insert"
+					. " into '{$this->table}', but ID $id was previously associated with"
+					. " the name '{$table[$id]}'. Overriding the old value, which presumably"
+					. " has been removed from the database due to a transaction rollback.";
+				$this->logger->warning( $m );
 			}
+
+			$table[$id] = $name;
+			$searchResult = $id;
+
 			$this->tableCache = $table;
 		}
 
@@ -222,12 +199,6 @@ class NameTableStore {
 	 * @return string[] The freshly reloaded name map
 	 */
 	public function reloadMap( $connFlags = 0 ) {
-		if ( $connFlags !== 0 && defined( 'MW_PHPUNIT_TEST' ) ) {
-			// HACK: We can't use $connFlags while doing PHPUnit tests, because the
-			// fake database tables are bound to a single connection.
-			$connFlags = 0;
-		}
-
 		$dbw = $this->getDBConnection( DB_PRIMARY, $connFlags );
 		$this->tableCache = $this->loadTable( $dbw );
 		$dbw->onTransactionPreCommitOrIdle( function () {
@@ -334,7 +305,7 @@ class NameTableStore {
 	}
 
 	/**
-	 * @return string[]
+	 * @return array<int,string>
 	 */
 	private function getTableFromCachesOrReplica() {
 		if ( $this->tableCache !== null ) {
@@ -360,8 +331,7 @@ class NameTableStore {
 	 * Gets the table from the db
 	 *
 	 * @param IReadableDatabase $db
-	 *
-	 * @return string[]
+	 * @return array<int,string>
 	 */
 	private function loadTable( IReadableDatabase $db ) {
 		$result = $db->newSelectQueryBuilder()
@@ -375,7 +345,7 @@ class NameTableStore {
 
 		$assocArray = [];
 		foreach ( $result as $row ) {
-			$assocArray[$row->id] = $row->name;
+			$assocArray[(int)$row->id] = $row->name;
 		}
 
 		return $assocArray;
@@ -385,7 +355,7 @@ class NameTableStore {
 	 * Stores the given name in the DB, returning the ID when an insert occurs.
 	 *
 	 * @param string $name
-	 * @return int|null int if we know the ID, null if we don't
+	 * @return int The new or colliding ID
 	 */
 	private function store( string $name ) {
 		Assert::parameter( $name !== '', '$name', 'should not be an empty string' );
@@ -399,15 +369,43 @@ class NameTableStore {
 			->row( $this->getFieldsToStore( $name ) )
 			->caller( __METHOD__ )->execute();
 
-		if ( $dbw->affectedRows() === 0 ) {
-			$this->logger->info(
-				'Tried to insert name into table ' . $this->table . ', but value already existed.'
+		if ( $dbw->affectedRows() > 0 ) {
+			$id = $dbw->insertId();
+			// As store returned an ID we know we inserted so delete from WAN cache
+			$dbw->onTransactionPreCommitOrIdle(
+				function () {
+					$this->cache->delete( $this->getCacheKey() );
+				},
+				__METHOD__
 			);
 
-			return null;
+			return $id;
+		}
+
+		$this->logger->info(
+			'Tried to insert name into table ' . $this->table . ', but value already existed.'
+		);
+
+		// Note that in MySQL, even if this method somehow runs in a transaction, a plain
+		// (non-locking) SELECT will see the new row created by the other transaction, even
+		// with REPEATABLE-READ. This is due to how "consistent reads" works: the latest
+		// version of rows become visible to the snapshot after the transaction sees those
+		// rows as either matching an update query or conflicting with an insert query.
+		$id = $dbw->newSelectQueryBuilder()
+			->select( [ 'id' => $this->idField ] )
+			->from( $this->table )
+			->where( [ $this->nameField => $name ] )
+			->caller( __METHOD__ )->fetchField();
+
+		if ( $id === false ) {
+			// Insert failed due to IGNORE flag, but DB_PRIMARY didn't give us the data
+			$m = "No insert possible but primary DB didn't give us a record for " .
+				"'{$name}' in '{$this->table}'";
+			$this->logger->error( $m );
+			throw new NameTableAccessException( $m );
 		}
 
-		return $dbw->insertId();
+		return (int)$id;
 	}
 
 	/**
diff --git a/includes/Storage/NameTableStoreFactory.php b/includes/Storage/NameTableStoreFactory.php
index c51a299e2f73..88dce01d6dc5 100644
--- a/includes/Storage/NameTableStoreFactory.php
+++ b/includes/Storage/NameTableStoreFactory.php
@@ -26,9 +26,9 @@ use WANObjectCache;
 use Wikimedia\Rdbms\ILBFactory;
 
 class NameTableStoreFactory {
-	/** @var array[]|null */
+	/** @var array<string,mixed> */
 	private static $info;
-	/** @var NameTableStore[][] */
+	/** @var array<string,array<string,NameTableStore>> */
 	private $stores = [];
 
 	/** @var ILBFactory */
diff --git a/tests/phpunit/includes/Storage/NameTableStoreTest.php b/tests/phpunit/includes/Storage/NameTableStoreTest.php
index 922331e6a346..1f12f60fb99c 100644
--- a/tests/phpunit/includes/Storage/NameTableStoreTest.php
+++ b/tests/phpunit/includes/Storage/NameTableStoreTest.php
@@ -54,15 +54,17 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 	}
 
 	/**
-	 * @param null $insertCalls
-	 * @param null $selectCalls
+	 * @param int $insertCalls
+	 * @param int $selectCalls
+	 * @param int $selectFieldCalls
 	 *
 	 * @return MockObject&IDatabase
 	 */
-	private function getProxyDb( $insertCalls = null, $selectCalls = null ) {
+	private function getProxyDb( $insertCalls, $selectCalls, $selectFieldCalls ) {
 		$proxiedMethods = [
 			'select' => $selectCalls,
 			'insert' => $insertCalls,
+			'selectField' => $selectFieldCalls,
 			'affectedRows' => null,
 			'insertId' => null,
 			'getSessionLagStatus' => null,
@@ -89,11 +91,14 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 		BagOStuff $cacheBag,
 		$insertCalls,
 		$selectCalls,
+		$selectFieldCalls,
 		$normalizationCallback = null,
 		$insertCallback = null
 	) {
 		return new NameTableStore(
-			$this->getMockLoadBalancer( $this->getProxyDb( $insertCalls, $selectCalls ) ),
+			$this->getMockLoadBalancer(
+				$this->getProxyDb( $insertCalls, $selectCalls, $selectFieldCalls )
+			),
 			$this->getHashWANObjectCache( $cacheBag ),
 			new NullLogger(),
 			'slot_roles', 'role_id', 'role_name',
@@ -146,7 +151,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 		$expectedId
 	) {
 		$this->populateTable( $existingValues );
-		$store = $this->getNameTableSqlStore( $cacheBag, (int)$needsInsert, $selectCalls );
+		$store = $this->getNameTableSqlStore( $cacheBag, (int)$needsInsert, $selectCalls, 0 );
 
 		// Some names will not initially exist
 		try {
@@ -189,6 +194,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 			new EmptyBagOStuff(),
 			1,
 			1,
+			0,
 			$normalizationCallback
 		);
 		$acquiredId = $store->acquireId( $nameIn );
@@ -209,7 +215,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 		$now = microtime( true );
 		$cacheBag->setMockTime( $now );
 		// Check for operations to in-memory cache (IMC) and persistent cache (PC)
-		$store = $this->getNameTableSqlStore( $cacheBag, $insertCalls, $selectCalls );
+		$store = $this->getNameTableSqlStore( $cacheBag, $insertCalls, $selectCalls, 0 );
 
 		// Get 1 ID and make sure getName returns correctly
 		$fooId = $store->acquireId( 'foo' ); // regen PC, set IMC, update IMC, tombstone PC
@@ -237,7 +243,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 	}
 
 	public function testGetName_masterFallback() {
-		$store = $this->getNameTableSqlStore( new EmptyBagOStuff(), 1, 2 );
+		$store = $this->getNameTableSqlStore( new EmptyBagOStuff(), 1, 2, 0 );
 
 		// Insert a new name
 		$fooId = $store->acquireId( 'foo' );
@@ -251,14 +257,14 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 
 	public function testGetMap_empty() {
 		$this->populateTable( [] );
-		$store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 1 );
+		$store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 1, 0 );
 		$table = $store->getMap();
 		$this->assertSame( [], $table );
 	}
 
 	public function testGetMap_twoValues() {
 		$this->populateTable( [ 'foo', 'bar' ] );
-		$store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 1 );
+		$store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 1, 0 );
 
 		// We are using a cache, so 2 calls should only result in 1 select on the db
 		$store->getMap();
@@ -272,7 +278,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 
 	public function testReloadMap() {
 		$this->populateTable( [ 'foo' ] );
-		$store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 2 );
+		$store = $this->getNameTableSqlStore( new HashBagOStuff(), 0, 2, 0 );
 
 		// force load
 		$this->assertCount( 1, $store->getMap() );
@@ -287,9 +293,9 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 
 	public function testCacheRaceCondition() {
 		$wanHashBag = new HashBagOStuff();
-		$store1 = $this->getNameTableSqlStore( $wanHashBag, 1, 1 );
-		$store2 = $this->getNameTableSqlStore( $wanHashBag, 1, 0 );
-		$store3 = $this->getNameTableSqlStore( $wanHashBag, 1, 1 );
+		$store1 = $this->getNameTableSqlStore( $wanHashBag, 1, 1, 0 );
+		$store2 = $this->getNameTableSqlStore( $wanHashBag, 1, 0, 0 );
+		$store3 = $this->getNameTableSqlStore( $wanHashBag, 2, 0, 2 );
 
 		// Cache the current table in the instances we will use
 		// This simulates multiple requests running simultaneously
@@ -308,7 +314,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 		// A new store should be able to get both of these new Ids
 		// Note: before there was a race condition here where acquireId( 'bar' ) would update the
 		//       cache with data missing the 'foo' key that it was not aware of
-		$store4 = $this->getNameTableSqlStore( $wanHashBag, 0, 1 );
+		$store4 = $this->getNameTableSqlStore( $wanHashBag, 0, 1, 0 );
 		$this->assertSame( $fooId, $store4->getId( 'foo' ) );
 		$this->assertSame( $barId, $store4->getId( 'bar' ) );
 
@@ -325,6 +331,7 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
 			new EmptyBagOStuff(),
 			1,
 			1,
+			0,
 			null,
 			static function ( $insertFields ) {
 				$insertFields['role_id'] = 7251;