JanZerebecki has uploaded a new change for review. https://gerrit.wikimedia.org/r/233743
Change subject: EntityUsageTable: Wrap batches in transactions ...................................................................... EntityUsageTable: Wrap batches in transactions Also reduces the default batch size to 100. As per jcrespo's comment https://phabricator.wikimedia.org/T107319#1570912 Bug: T107319 Change-Id: I992f0a8bd6494ebc225e80e4cdae830583612478 --- M client/includes/Usage/Sql/EntityUsageTable.php M client/includes/Usage/Sql/SqlUsageTracker.php M client/includes/store/sql/ConsistentReadConnectionManager.php M client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php 4 files changed, 45 insertions(+), 13 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/43/233743/1 diff --git a/client/includes/Usage/Sql/EntityUsageTable.php b/client/includes/Usage/Sql/EntityUsageTable.php index f68b864..89e1a1a 100644 --- a/client/includes/Usage/Sql/EntityUsageTable.php +++ b/client/includes/Usage/Sql/EntityUsageTable.php @@ -4,6 +4,7 @@ use ArrayIterator; use DatabaseBase; +use Exception; use InvalidArgumentException; use Iterator; use Wikibase\Client\Usage\EntityUsage; @@ -131,6 +132,7 @@ * @param string $touched timestamp */ private function touchUsageBatch( array $rowIds, $touched ) { + $this->connection->begin( __METHOD__ ); $this->connection->update( $this->tableName, array( @@ -141,6 +143,7 @@ ), __METHOD__ ); + $this->connection->commit( __METHOD__ ); } /** @@ -195,8 +198,12 @@ $c = 0; foreach ( $batches as $rows ) { + $this->connection->begin( __METHOD__ ); + $this->connection->insert( $this->tableName, $rows, __METHOD__, array( 'IGNORE' ) ); $c += $this->connection->affectedRows(); + + $this->connection->commit( __METHOD__ ); } return $c; @@ -344,6 +351,7 @@ $batches = array_chunk( $idStrings, $this->batchSize ); foreach ( $batches as $batch ) { + $this->connection->begin( __METHOD__ ); $this->connection->delete( $this->tableName, array( @@ -351,6 +359,7 @@ ), __METHOD__ ); + $this->connection->commit( __METHOD__ ); } } diff --git a/client/includes/Usage/Sql/SqlUsageTracker.php b/client/includes/Usage/Sql/SqlUsageTracker.php index 34356dc..fd35634 100644 --- a/client/includes/Usage/Sql/SqlUsageTracker.php +++ b/client/includes/Usage/Sql/SqlUsageTracker.php @@ -37,7 +37,7 @@ /** * @var int */ - private $batchSize = 1000; + private $batchSize = 100; /** * @param EntityIdParser $idParser @@ -129,7 +129,9 @@ return; } - $db = $this->connectionManager->beginAtomicSection( __METHOD__ ); + // NOTE: while logically we'd like the below to be atomic, we don't wrap it in a + // transaction to prevent long lock retention during big updates. + $db = $this->connectionManager->getWriteConnection(); try { $usageTable = $this->newUsageTable( $db ); @@ -145,9 +147,9 @@ $usageTable->touchUsages( $pageId, $keep, $touched ); $usageTable->addUsages( $pageId, $added, $touched ); - $this->connectionManager->commitAtomicSection( $db, __METHOD__ ); + $this->connectionManager->releaseConnection( $db ); } catch ( Exception $ex ) { - $this->connectionManager->rollbackAtomicSection( $db, __METHOD__ ); + $this->connectionManager->releaseConnection( $db ); if ( $ex instanceof DBError ) { throw new UsageTrackerException( $ex->getMessage(), $ex->getCode(), $ex ); @@ -168,16 +170,18 @@ * @throws UsageTrackerException */ public function pruneStaleUsages( $pageId, $lastUpdatedBefore ) { - $db = $this->connectionManager->beginAtomicSection( __METHOD__ ); + // NOTE: while logically we'd like the below to be atomic, we don't wrap it in a + // transaction to prevent long lock retention during big updates. + $db = $this->connectionManager->getWriteConnection(); try { $usageTable = $this->newUsageTable( $db ); $pruned = $usageTable->pruneStaleUsages( $pageId, $lastUpdatedBefore ); - $this->connectionManager->commitAtomicSection( $db, __METHOD__ ); + $this->connectionManager->releaseConnection( $db ); return $pruned; } catch ( Exception $ex ) { - $this->connectionManager->rollbackAtomicSection( $db, __METHOD__ ); + $this->connectionManager->releaseConnection( $db ); if ( $ex instanceof DBError ) { throw new UsageTrackerException( $ex->getMessage(), $ex->getCode(), $ex ); @@ -200,15 +204,17 @@ return; } - $db = $this->connectionManager->beginAtomicSection( __METHOD__ ); + // NOTE: while logically we'd like the below to be atomic, we don't wrap it in a + // transaction to prevent long lock retention during big updates. + $db = $this->connectionManager->getWriteConnection(); try { $usageTable = $this->newUsageTable( $db ); $usageTable->removeEntities( $entityIds ); - $this->connectionManager->commitAtomicSection( $db, __METHOD__ ); + $this->connectionManager->releaseConnection( $db ); } catch ( Exception $ex ) { - $this->connectionManager->rollbackAtomicSection( $db, __METHOD__ ); + $this->connectionManager->releaseConnection( $db ); if ( $ex instanceof DBError ) { throw new UsageTrackerException( $ex->getMessage(), $ex->getCode(), $ex ); diff --git a/client/includes/store/sql/ConsistentReadConnectionManager.php b/client/includes/store/sql/ConsistentReadConnectionManager.php index 9e48c45..503a3b0 100644 --- a/client/includes/store/sql/ConsistentReadConnectionManager.php +++ b/client/includes/store/sql/ConsistentReadConnectionManager.php @@ -70,7 +70,8 @@ } /** - * Returns a database connection for reading. + * Returns a database connection for reading. The connection should later be released by + * calling releaseConnection(). * * @note: If forceMaster() or beginAtomicSection() were previously called on this * ConsistentReadConnectionManager instance, this method will return a connection to the master database, @@ -84,11 +85,12 @@ } /** - * Returns a connection to the master DB, for updating. + * Returns a connection to the master DB, for updating. The connection should later be released + * by calling releaseConnection(). * * @return DatabaseBase */ - private function getWriteConnection() { + public function getWriteConnection() { return $this->loadBalancer->getConnection( DB_MASTER, array(), $this->dbName ); } diff --git a/client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php b/client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php index 6190ac9..cfaeb9c 100644 --- a/client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php +++ b/client/tests/phpunit/includes/store/sql/ConsistentReadConnectionManagerTest.php @@ -43,6 +43,21 @@ $this->assertSame( $connection, $actual ); } + public function testGetWriteConnection() { + $connection = $this->getConnectionMock(); + $lb = $this->getLoadBalancerMock(); + + $lb->expects( $this->once() ) + ->method( 'getConnection' ) + ->with( DB_MASTER ) + ->will( $this->returnValue( $connection ) ); + + $manager = new ConsistentReadConnectionManager( $lb ); + $actual = $manager->getWriteConnection(); + + $this->assertSame( $connection, $actual ); + } + public function testForceMaster() { $connection = $this->getConnectionMock(); $lb = $this->getLoadBalancerMock(); -- To view, visit https://gerrit.wikimedia.org/r/233743 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I992f0a8bd6494ebc225e80e4cdae830583612478 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: wmf/1.26wmf20 Gerrit-Owner: JanZerebecki <jan.wikime...@zerebecki.de> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
