Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Storage: Remove transaction cancel callback from NameTableStore

Browse source 
What it's trying to achieve can be done much simpler by just making an
out of transaction connection via setting CONN_TRX_AUTOCOMMIT flag.

Removing this allows us to get rid of a large portion of transaction
management in rdbms library as this is the only place that uses cancel
callbacks.

Split out of I3cb1e30611b83c0

Bug: T372169
Change-Id: Idad029b6da6b09e084d466d282ef1145ebd8fe73
This commit is contained in:
Amir Sarabadani 2024-08-13 00:23:30 +02:00 committed by Ladsgroup
parent 750aa11700
commit 505bd8e70f
2 changed files with 12 additions and 254 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

114
includes/Storage/NameTableStore.php
View file

@ -20,7 +20,6 @@
namespace MediaWiki\Storage;
use Exception;
use Psr\Log\LoggerInterface;
use WANObjectCache;
use Wikimedia\Assert\Assert;
@ -29,7 +28,6 @@ use Wikimedia\Rdbms\Database;
use Wikimedia\Rdbms\IDatabase;
use Wikimedia\Rdbms\ILoadBalancer;
use Wikimedia\Rdbms\IReadableDatabase;
use Wikimedia\RequestTimeout\TimeoutException;
/**
* @since 1.31
@ -393,111 +391,23 @@ class NameTableStore {
Assert::parameter( $name !== '', '$name', 'should not be an empty string' );
// Note: this is only called internally so normalization of $name has already occurred.
$dbw = $this->getDBConnection( DB_PRIMARY );
$dbw = $this->getDBConnection( DB_PRIMARY, ILoadBalancer::CONN_TRX_AUTOCOMMIT );
$id = null;
$dbw->doAtomicSection(
__METHOD__,
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->newInsertQueryBuilder()
->insertInto( $this->table )
->ignore()
->row( $this->getFieldsToStore( $name ) )
->caller( $fname )->execute();
$dbw->newInsertQueryBuilder()
->insertInto( $this->table )
->ignore()
->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.'
);
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
);
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, $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->newInsertQueryBuilder()
->insertInto( $this->table )
->row( $this->getFieldsToStore( $name, $id ) )
->caller( $fname )->execute();
// 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 ) {
$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
if ( $dbw->affectedRows() === 0 ) {
$this->logger->info(
'Tried to insert name into table ' . $this->table . ', but value already existed.'
);
} catch ( TimeoutException $e ) {
throw $e;
} catch ( Exception $ex ) {
$this->logger->error(
'Re-insertion of name into table ' . $this->table . ' failed: ' . $ex->getMessage()
);
} 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 null;
}
return $dbw->insertId();
}
/**

152
tests/phpunit/includes/Storage/NameTableStoreTest.php
View file

@ -7,7 +7,6 @@ use MediaWiki\Storage\NameTableStore;
use MediaWikiIntegrationTestCase;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\NullLogger;
use RuntimeException;
use WANObjectCache;
use Wikimedia\ObjectCache\BagOStuff;
use Wikimedia\ObjectCache\EmptyBagOStuff;
@ -68,7 +67,6 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
'insertId' => null,
'getSessionLagStatus' => null,
'onTransactionPreCommitOrIdle' => null,
'onAtomicSectionCancel' => null,
'doAtomicSection' => null,
'begin' => null,
'rollback' => null,
@ -335,154 +333,4 @@ class NameTableStoreTest extends MediaWikiIntegrationTestCase {
);
$this->assertSame( 7251, $store->acquireId( 'A' ) );
}
public function testTransactionRollback() {
$lb = $this->getServiceContainer()->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'
);
$db = $this->getDb();
$db->begin( __METHOD__ );
$fooId = $store1->acquireId( 'foo' );
$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( static function () use ( &$insertCalls ) {
$insertCalls++;
switch ( $insertCalls ) {
case 1:
return true;
case 2:
throw new RuntimeException( 'Testing' );
}
return true;
} );
$db->method( 'newInsertQueryBuilder' )->willReturnCallback( static fn () => new InsertQueryBuilder( $db ) );
$lb = $this->createMock( LoadBalancer::class );
$lb->method( 'getConnection' )
->willReturn( $db );
// 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->getDb()->begin( __METHOD__ );
$store1->acquireId( 'foo' );
$this->getDb()->rollback( __METHOD__ );
$this->assertArrayNotHasKey( 'foo', $store1->getMap() );
}
public function testTransactionRollbackWithInterference() {
// FIXME: https://phabricator.wikimedia.org/T259085
$this->markTestSkippedIfDbType( 'sqlite' );
$lb = $this->getServiceContainer()->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->getDb()->begin( __METHOD__ );
$quuxId = null;
$this->getDb()->onTransactionResolution(
static function () use ( $store1, &$quuxId ) {
$quuxId = $store1->acquireId( 'quux' );
},
__METHOD__
);
$store1->acquireId( 'foo' );
$this->getDb()->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 = $this->getServiceContainer()->getDBLoadBalancer();
$store = new NameTableStore(
$lb,
$this->getHashWANObjectCache( new HashBagOStuff() ),
new NullLogger(),
'slot_roles', 'role_id', 'role_name'
);
// Nested atomic sections
$db = $this->getDb();
$atomic1 = $db->startAtomic( $fname, $this->getDb()::ATOMIC_CANCELABLE );
$atomic2 = $db->startAtomic( $fname, $this->getDb()::ATOMIC_CANCELABLE );
// Acquire ID
$id = $store->acquireId( 'foo' );
// Oops, rolled back
$db->cancelAtomic( $fname, $atomic2 );
// Should have been re-inserted
$store->reloadMap();
$this->assertSame( $id, $store->getId( 'foo' ) );
// Oops, re-insert was rolled back too.
$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
}
}
}