namespace MediaWiki\Storage;
+use Exception;
use IExpiringStore;
use Psr\Log\LoggerInterface;
use WANObjectCache;
* 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
$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;
* @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 () {
$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 );
}
/** @var NameTableStore */
private $contentModelStore;
+ /** @var NameTableStore */
+ private $slotRoleStore;
+
/** @var BlobStore */
private $blobStore;
private function initServices() {
$this->dbw = $this->getDB( DB_MASTER );
$this->contentModelStore = MediaWikiServices::getInstance()->getContentModelStore();
+ $this->slotRoleStore = MediaWikiServices::getInstance()->getSlotRoleStore();
$this->blobStore = MediaWikiServices::getInstance()->getBlobStore();
- $this->mainRoleId = MediaWikiServices::getInstance()->getSlotRoleStore()
- ->acquireId( SlotRecord::MAIN );
+
+ // Don't trust the cache for the NameTableStores, in case something went
+ // wrong during a previous run (see T224949#5325895).
+ $this->contentModelStore->reloadMap();
+ $this->slotRoleStore->reloadMap();
+ $this->mainRoleId = $this->slotRoleStore->acquireId( SlotRecord::MAIN );
}
public function execute() {
use BagOStuff;
use EmptyBagOStuff;
use HashBagOStuff;
+use MediaWiki\MediaWikiServices;
use MediaWiki\Storage\NameTableAccessException;
use MediaWiki\Storage\NameTableStore;
use MediaWikiTestCase;
+use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\NullLogger;
+use RuntimeException;
use WANObjectCache;
use Wikimedia\Rdbms\IDatabase;
use Wikimedia\Rdbms\LoadBalancer;
return $mock;
}
- private function getCallCheckingDb( $insertCalls, $selectCalls ) {
+ /**
+ * @param null $insertCalls
+ * @param null $selectCalls
+ *
+ * @return MockObject|IDatabase
+ */
+ private function getProxyDb( $insertCalls = null, $selectCalls = null ) {
$proxiedMethods = [
'select' => $selectCalls,
'insert' => $insertCalls,
'insertId' => null,
'getSessionLagStatus' => null,
'writesPending' => null,
- 'onTransactionPreCommitOrIdle' => null
+ 'onTransactionPreCommitOrIdle' => null,
+ 'onAtomicSectionCancel' => null,
+ 'doAtomicSection' => null,
+ 'begin' => null,
+ 'rollback' => null,
+ 'commit' => null,
];
$mock = $this->getMockBuilder( IDatabase::class )
->disableOriginalConstructor()
$insertCallback = null
) {
return new NameTableStore(
- $this->getMockLoadBalancer( $this->getCallCheckingDb( $insertCalls, $selectCalls ) ),
+ $this->getMockLoadBalancer( $this->getProxyDb( $insertCalls, $selectCalls ) ),
$this->getHashWANObjectCache( $cacheBag ),
new NullLogger(),
'slot_roles', 'role_id', 'role_name',
$this->assertSame( 7251, $store->acquireId( 'A' ) );
}
+ public function testTransactionRollback() {
+ $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
+
+ // Two instances hitting the real database using separate caches.
+ $store1 = new NameTableStore(
+ $lb,
+ $this->getHashWANObjectCache( new HashBagOStuff() ),
+ new NullLogger(),
+ 'slot_roles', 'role_id', 'role_name'
+ );
+ $store2 = new NameTableStore(
+ $lb,
+ $this->getHashWANObjectCache( new HashBagOStuff() ),
+ new NullLogger(),
+ 'slot_roles', 'role_id', 'role_name'
+ );
+
+ $this->db->begin( __METHOD__ );
+ $fooId = $store1->acquireId( 'foo' );
+ $this->db->rollback( __METHOD__ );
+
+ $this->assertSame( $fooId, $store2->getId( 'foo' ) );
+ $this->assertSame( $fooId, $store1->getId( 'foo' ) );
+ }
+
+ public function testTransactionRollbackWithFailedRedo() {
+ $insertCalls = 0;
+
+ $db = $this->getProxyDb( 2 );
+ $db->method( 'insert' )
+ ->willReturnCallback( function () use ( &$insertCalls, $db ) {
+ $insertCalls++;
+ switch ( $insertCalls ) {
+ case 1:
+ return true;
+ case 2:
+ throw new RuntimeException( 'Testing' );
+ }
+
+ return true;
+ } );
+
+ $lb = $this->getMockBuilder( LoadBalancer::class )
+ ->disableOriginalConstructor()
+ ->getMock();
+ $lb->method( 'getConnectionRef' )
+ ->willReturn( $db );
+ $lb->method( 'resolveDomainID' )
+ ->willReturnArgument( 0 );
+
+ // Two instances hitting the real database using separate caches.
+ $store1 = new NameTableStore(
+ $lb,
+ $this->getHashWANObjectCache( new HashBagOStuff() ),
+ new NullLogger(),
+ 'slot_roles', 'role_id', 'role_name'
+ );
+
+ $this->db->begin( __METHOD__ );
+ $store1->acquireId( 'foo' );
+ $this->db->rollback( __METHOD__ );
+
+ $this->assertArrayNotHasKey( 'foo', $store1->getMap() );
+ }
+
+ public function testTransactionRollbackWithInterference() {
+ $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
+
+ // Two instances hitting the real database using separate caches.
+ $store1 = new NameTableStore(
+ $lb,
+ $this->getHashWANObjectCache( new HashBagOStuff() ),
+ new NullLogger(),
+ 'slot_roles', 'role_id', 'role_name'
+ );
+ $store2 = new NameTableStore(
+ $lb,
+ $this->getHashWANObjectCache( new HashBagOStuff() ),
+ new NullLogger(),
+ 'slot_roles', 'role_id', 'role_name'
+ );
+
+ $this->db->begin( __METHOD__ );
+
+ $quuxId = null;
+ $this->db->onTransactionResolution(
+ function () use ( $store1, &$quuxId ) {
+ $quuxId = $store1->acquireId( 'quux' );
+ }
+ );
+
+ $store1->acquireId( 'foo' );
+ $this->db->rollback( __METHOD__ );
+
+ // $store2 should know about the insert by $store1
+ $this->assertSame( $quuxId, $store2->getId( 'quux' ) );
+
+ // A "best effort" attempt was made to restore the entry for 'foo'
+ // after the transaction failed. This may succeed on some databases like MySQL,
+ // while it fails on others. Since we are giving no guarantee about this,
+ // the only thing we can test here is that acquireId( 'foo' ) returns an
+ // ID that is distinct from the ID of quux (but might be different from the
+ // value returned by the original call to acquireId( 'foo' ).
+ // Note that $store2 will not know about the ID for 'foo' acquired by $store1,
+ // because it's using a separate cache, and getId() does not fall back to
+ // checking the database.
+ $this->assertNotSame( $quuxId, $store1->acquireId( 'foo' ) );
+ }
+
+ public function testTransactionDoubleRollback() {
+ $fname = __METHOD__;
+
+ $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
+ $store = new NameTableStore(
+ $lb,
+ $this->getHashWANObjectCache( new HashBagOStuff() ),
+ new NullLogger(),
+ 'slot_roles', 'role_id', 'role_name'
+ );
+
+ // Nested atomic sections
+ $atomic1 = $this->db->startAtomic( $fname, $this->db::ATOMIC_CANCELABLE );
+ $atomic2 = $this->db->startAtomic( $fname, $this->db::ATOMIC_CANCELABLE );
+
+ // Acquire ID
+ $id = $store->acquireId( 'foo' );
+
+ // Oops, rolled back
+ $this->db->cancelAtomic( $fname, $atomic2 );
+
+ // Should have been re-inserted
+ $store->reloadMap();
+ $this->assertSame( $id, $store->getId( 'foo' ) );
+
+ // Oops, re-insert was rolled back too.
+ $this->db->cancelAtomic( $fname, $atomic1 );
+
+ // This time, no re-insertion happened.
+ try {
+ $id2 = $store->getId( 'foo' );
+ $this->fail( "Expected NameTableAccessException, got $id2 (originally was $id)" );
+ } catch ( NameTableAccessException $ex ) {
+ // expected
+ }
+ }
+
}
dépôts / lhc / web / wiklou.git / commitdiff