X-Git-Url: https://git.cyclocoop.org/?a=blobdiff_plain;f=includes%2FStorage%2FNameTableStore.php;h=d57481becef7806cc3100d79c86e4a9e98f3f5b7;hb=refs%2Ftags%2F1.34.0-rc.1;hp=a14e339020912906322d539370cc207913029d58;hpb=b942fc27c93e54868298608698fe6b965907d33e;p=lhc%2Fweb%2Fwiklou.git diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php index a14e339020..d57481bece 100644 --- a/includes/Storage/NameTableStore.php +++ b/includes/Storage/NameTableStore.php @@ -20,6 +20,7 @@ namespace MediaWiki\Storage; +use Exception; use IExpiringStore; use Psr\Log\LoggerInterface; use WANObjectCache; @@ -66,7 +67,7 @@ class NameTableStore { /** * @param ILoadBalancer $dbLoadBalancer A load balancer for acquiring database connections * @param WANObjectCache $cache A cache manager for caching data. This can be the local - * wiki's default instance even if $wikiId refers to a different wiki, since + * wiki's default instance even if $dbDomain refers to a different wiki, since * makeGlobalKey() is used to constructed a key that allows cached names from * the same database to be re-used between wikis. For example, enwiki and frwiki will * use the same cache keys for names from the wikidatawiki database, regardless @@ -111,7 +112,7 @@ class NameTableStore { * @return IDatabase */ private function getDBConnection( $index, $flags = 0 ) { - return $this->loadBalancer->getConnection( $index, [], $this->domain, $flags ); + return $this->loadBalancer->getConnectionRef( $index, [], $this->domain, $flags ); } /** @@ -145,6 +146,15 @@ 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. + * * @param string $name * @throws NameTableAccessException * @return int @@ -160,10 +170,7 @@ class NameTableStore { if ( $id === null ) { // RACE: $name was already in the db, probably just inserted, so load from master. // Use DBO_TRX to avoid missing inserts due to other threads or REPEATABLE-READs. - // ...but not during unit tests, because we need the fake DB tables of the default - // connection. - $connFlags = defined( 'MW_PHPUNIT_TEST' ) ? 0 : ILoadBalancer::CONN_TRX_AUTOCOMMIT; - $table = $this->reloadMap( $connFlags ); + $table = $this->reloadMap( ILoadBalancer::CONN_TRX_AUTOCOMMIT ); $searchResult = array_search( $name, $table, true ); if ( $searchResult === false ) { @@ -173,12 +180,22 @@ class NameTableStore { $this->logger->error( $m ); throw new NameTableAccessException( $m ); } - } elseif ( isset( $table[$id] ) ) { - throw new NameTableAccessException( - "Expected unused ID from database insert for '$name' " - . " into '{$this->table}', but ID $id is already associated with" - . " the name '{$table[$id]}'! This may indicate database corruption!" ); } 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; @@ -207,6 +224,12 @@ 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_MASTER, $connFlags ); $this->tableCache = $this->loadTable( $dbw ); $dbw->onTransactionPreCommitOrIdle( function () { @@ -378,29 +401,125 @@ class NameTableStore { $dbw = $this->getDBConnection( DB_MASTER ); - $dbw->insert( - $this->table, - $this->getFieldsToStore( $name ), + $id = null; + $dbw->doAtomicSection( __METHOD__, - [ 'IGNORE' ] + function ( IDatabase $unused, $fname ) + use ( $name, &$id, $dbw ) { + // NOTE: use IDatabase from the parent scope here, not the function parameter. + // If $dbw is a wrapper around the actual DB, we need to call the wrapper here, + // not the inner instance. + $dbw->insert( + $this->table, + $this->getFieldsToStore( $name ), + $fname, + [ 'IGNORE' ] + ); + + if ( $dbw->affectedRows() === 0 ) { + $this->logger->info( + 'Tried to insert name into table ' . $this->table . ', but value already existed.' + ); + + return; + } + + $id = $dbw->insertId(); + + // Any open transaction may still be rolled back. If that happens, we have to re-try the + // insertion and restore a consistent state of the cached table. + $dbw->onAtomicSectionCancel( + function ( $trigger, IDatabase $unused ) use ( $name, $id, $dbw ) { + $this->retryStore( $dbw, $name, $id ); + }, + $fname ); + }, + IDatabase::ATOMIC_CANCELABLE ); - if ( $dbw->affectedRows() === 0 ) { - $this->logger->info( - 'Tried to insert name into table ' . $this->table . ', but value already existed.' + return $id; + } + + /** + * After the initial insertion got rolled back, this can be used to try the insertion again, + * and ensure a consistent state of the cache. + * + * @param IDatabase $dbw + * @param string $name + * @param int $id + */ + private function retryStore( IDatabase $dbw, $name, $id ) { + // NOTE: in the closure below, use the IDatabase from the original method call, + // not the one passed to the closure as a parameter. + // If $dbw is a wrapper around the actual DB, we need to call the wrapper, + // not the inner instance. + + try { + $dbw->doAtomicSection( + __METHOD__, + function ( IDatabase $unused, $fname ) use ( $name, $id, &$ok, $dbw ) { + // Try to insert a row with the ID we originally got. + // If that fails (because of a key conflict), we will just try to get another ID again later. + $dbw->insert( + $this->table, + $this->getFieldsToStore( $name, $id ), + $fname + ); + + // Make sure we re-load the map in case this gets rolled back again. + // We could re-try once more, but that bears the risk of an infinite loop. + // So let's just give up on the ID. + $dbw->onAtomicSectionCancel( + function ( $trigger, IDatabase $unused ) use ( $name, $id, $dbw ) { + $this->logger->warning( + 'Re-insertion of name into table ' . $this->table + . ' was rolled back. Giving up and reloading the cache.' + ); + $this->reloadMap( ILoadBalancer::CONN_TRX_AUTOCOMMIT ); + }, + $fname + ); + + $this->logger->info( + 'Re-insert name into table ' . $this->table . ' after failed transaction.' + ); + }, + IDatabase::ATOMIC_CANCELABLE + ); + } catch ( Exception $ex ) { + $this->logger->error( + 'Re-insertion of name into table ' . $this->table . ' failed: ' . $ex->getMessage() ); - return null; + } finally { + // NOTE: we reload regardless of whether the above insert succeeded. There is + // only three possibilities: the insert succeeded, so the new map will have + // the desired $id/$name mapping. Or the insert failed because another + // process already inserted that same $id/$name mapping, in which case the + // new map will also have it. Or another process grabbed the desired ID for + // another name, or the database refuses to insert the given ID into the + // auto increment field - in that case, the new map will not have a mapping + // for $name (or has a different mapping for $name). In that last case, we can + // only hope that the ID produced within the failed transaction has not been + // used outside that transaction. + + $this->reloadMap( ILoadBalancer::CONN_TRX_AUTOCOMMIT ); } - - return $dbw->insertId(); } /** * @param string $name + * @param int|null $id * @return array */ - private function getFieldsToStore( $name ) { - $fields = [ $this->nameField => $name ]; + private function getFieldsToStore( $name, $id = null ) { + $fields = []; + + $fields[$this->nameField] = $name; + + if ( $id !== null ) { + $fields[$this->idField] = $id; + } + if ( $this->insertCallback !== null ) { $fields = call_user_func( $this->insertCallback, $fields ); }