Yaron Koren has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/375785 )
Change subject: Use auto-commit for temporary table operations ...................................................................... Use auto-commit for temporary table operations When running MySQL 5.7 with the --enforce-gtid-consistency option, operations on temporary tables are only permitted in auto-commit mode, outside of any transaction. As such, we must make sure that Semantic Drilldown performs all temporary table operations by properly committing any open transactions and setting the DBO_TRX flag for the query. See also: https://dev.mysql.com/doc/refman/5.7/en/replication-gtids-restrictions.html Bug: T174908 Change-Id: I51268b3eeaddb7cc0d49a19bc501117d5c248479 --- M SemanticDrilldown.php M includes/SD_Filter.php M includes/SD_Utils.php A includes/TemporaryTableManager.php M specials/SD_BrowseData.php A tests/phpunit/TemporaryTableManagerTest.php 6 files changed, 177 insertions(+), 6 deletions(-) Approvals: Yaron Koren: Looks good to me, approved jenkins-bot: Verified diff --git a/SemanticDrilldown.php b/SemanticDrilldown.php index 18f77d3..1fff8de 100644 --- a/SemanticDrilldown.php +++ b/SemanticDrilldown.php @@ -61,6 +61,7 @@ $wgAutoloadClasses['SDAppliedFilter'] = $sdgIP . '/includes/SD_AppliedFilter.php'; $wgAutoloadClasses['SDPageSchemas'] = $sdgIP . '/includes/SD_PageSchemas.php'; $wgAutoloadClasses['SDParserFunctions'] = $sdgIP . '/includes/SD_ParserFunctions.php'; +$wgAutoloadClasses['TemporaryTableManager'] = "$sdgIP/includes/TemporaryTableManager.php"; // register all special pages and other classes $wgSpecialPages['BrowseData'] = 'SDBrowseData'; @@ -72,6 +73,7 @@ $wgHooks['ParserBeforeTidy'][] = 'SDUtils::handleShowAndHide'; $wgHooks['PageSchemasRegisterHandlers'][] = 'SDPageSchemas::registerClass'; $wgHooks['ParserFirstCallInit'][] = 'SDParserFunctions::registerFunctions'; +$wgHooks['UnitTestsList'][] = 'SDUtils::onUnitTestsList'; # ## # This is the path to your installation of Semantic Drilldown as diff --git a/includes/SD_Filter.php b/includes/SD_Filter.php index d238a97..1cf7c6b 100644 --- a/includes/SD_Filter.php +++ b/includes/SD_Filter.php @@ -417,10 +417,12 @@ */ function createTempTable() { $dbr = wfGetDB( DB_SLAVE ); + $smw_ids = $dbr->tableName( SDUtils::getIDsTableName() ); + $valuesTable = $dbr->tableName( $this->getTableName() ); $value_field = $this->getValueField(); - $property_field = 'p_id'; + $query_property = $this->escaped_property; $sql = <<<END @@ -434,7 +436,9 @@ $sql .= " JOIN $smw_ids o_ids ON $valuesTable.o_id = o_ids.smw_id\n"; } $sql .= " WHERE p_ids.smw_title = '$query_property'"; - $dbr->query( $sql ); + + $temporaryTableManager = new TemporaryTableManager( $dbr ); + $temporaryTableManager->queryWithAutoCommit( $sql, __METHOD__ ); } /** @@ -445,6 +449,8 @@ // DROP TEMPORARY TABLE would be marginally safer, but it's // not supported on all RDBMS's. $sql = "DROP TABLE semantic_drilldown_filter_values"; - $dbr->query( $sql ); + + $temporaryTableManager = new TemporaryTableManager( $dbr ); + $temporaryTableManager->queryWithAutoCommit( $sql, __METHOD__ ); } } diff --git a/includes/SD_Utils.php b/includes/SD_Utils.php index 8816695..79e7fc1 100644 --- a/includes/SD_Utils.php +++ b/includes/SD_Utils.php @@ -488,4 +488,15 @@ return true; } + + /** + * Register extension unit tests with old versions of MediaWiki + * + * @param string[] $paths + * @return bool + */ + public static function onUnitTestsList( &$paths ) { + $paths[] = dirname( __FILE__ ) . '/../tests/phpunit'; + return true; + } } diff --git a/includes/TemporaryTableManager.php b/includes/TemporaryTableManager.php new file mode 100644 index 0000000..848208b --- /dev/null +++ b/includes/TemporaryTableManager.php @@ -0,0 +1,40 @@ +<?php +/** + * Provides helper method to execute SQL queries in auto-commit mode + */ + +class TemporaryTableManager { + /** @var \Wikimedia\Rdbms\IDatabase|DatabaseBase $databaseConnection */ + private $databaseConnection; + + /** + * TemporaryTableManager constructor. + * @param \Wikimedia\Rdbms\IDatabase|DatabaseBase $databaseConnection the DB connection to execute queries against + */ + public function __construct( $databaseConnection ) { + $this->databaseConnection = $databaseConnection; + } + + /** + * Execute the given SQL query against the database in auto-commit mode. + * If a transaction was already open (via DBO_TRX flag), it is commited. + * + * @param string $sqlQuery SQL query to execute + * @param string $method method name to log for query, defaults to this method + */ + public function queryWithAutoCommit( $sqlQuery, $method = __METHOD__ ) { + $wasAutoTrx = $this->databaseConnection->getFlag( DBO_TRX ); + $this->databaseConnection->clearFlag( DBO_TRX ); + + // If a transaction was automatically started on first query, make sure we commit it + if ( $wasAutoTrx && $this->databaseConnection->trxLevel() ) { + $this->databaseConnection->commit( __METHOD__ ); + } + + $this->databaseConnection->query( $sqlQuery, $method ); + + if ( $wasAutoTrx ) { + $this->databaseConnection->setFlag( DBO_TRX ); + } + } +} diff --git a/specials/SD_BrowseData.php b/specials/SD_BrowseData.php index 6f574bb..0608848 100644 --- a/specials/SD_BrowseData.php +++ b/specials/SD_BrowseData.php @@ -221,13 +221,18 @@ */ function createTempTable( $category, $subcategory, $subcategories, $applied_filters ) { $dbr = wfGetDB( DB_SLAVE ); + + $temporaryTableManager = new TemporaryTableManager( $dbr ); + $sql1 = "CREATE TEMPORARY TABLE semantic_drilldown_values ( id INT NOT NULL )"; - $dbr->query( $sql1 ); + $temporaryTableManager->queryWithAutoCommit( $sql1, __METHOD__ ); + $sql2 = "CREATE INDEX id_index ON semantic_drilldown_values ( id )"; - $dbr->query( $sql2 ); + $temporaryTableManager->queryWithAutoCommit( $sql2, __METHOD__ ); + $sql3 = "INSERT INTO semantic_drilldown_values SELECT ids.smw_id AS id\n"; $sql3 .= $this->getSQLFromClause( $category, $subcategory, $subcategories, $applied_filters ); - $dbr->query( $sql3 ); + $temporaryTableManager->queryWithAutoCommit( $sql3, __METHOD__ ); } /** diff --git a/tests/phpunit/TemporaryTableManagerTest.php b/tests/phpunit/TemporaryTableManagerTest.php new file mode 100644 index 0000000..bb0b4c7 --- /dev/null +++ b/tests/phpunit/TemporaryTableManagerTest.php @@ -0,0 +1,107 @@ +<?php + +class TemporaryTableManagerTest extends PHPUnit_Framework_TestCase { + /** @var DatabaseBase|\Wikimedia\Rdbms\IDatabase|PHPUnit_Framework_MockObject_MockObject */ + private $databaseConnectionMock; + + /** @var TemporaryTableManager $temporaryTableManager */ + private $temporaryTableManager; + + protected function setUp() { + parent::setUp(); + + // Database::commit is final, cannot be mocked - we must use interface :/ + $dbClassName = class_exists( 'DatabaseType' ) ? 'DatabaseType' : 'IDatabase'; + $this->databaseConnectionMock = $this->getMockBuilder( $dbClassName ) + ->disableOriginalConstructor() + ->getMock(); + + $this->temporaryTableManager = new TemporaryTableManager( $this->databaseConnectionMock ); + } + + /** + * @dataProvider sqlProvider + * @param string $sqlQuery + */ + public function testTransactionStateAndFlagsAreNotManipulatedWhenDboTrxIsNotSet( $sqlQuery ) { + $this->databaseConnectionMock->expects( $this->any() ) + ->method( 'getFlag' ) + ->with( DBO_TRX ) + ->willReturn( false ); + + $this->databaseConnectionMock->expects( $this->once() ) + ->method( 'query' ) + ->with( $sqlQuery, $this->anything() ); + + $this->databaseConnectionMock->expects( $this->never() ) + ->method( 'commit' ); + $this->databaseConnectionMock->expects( $this->never() ) + ->method( 'setFlag' ); + + $this->temporaryTableManager->queryWithAutoCommit( $sqlQuery ); + } + + /** + * @dataProvider sqlProvider + * @param string $sqlQuery + */ + public function testDboTrxFlagIsPreservedButCommitIsNotCalledIfDboTrxIsSetWithNoOpenTransaction( + $sqlQuery + ) { + $this->databaseConnectionMock->expects( $this->any() ) + ->method( 'getFlag' ) + ->with( DBO_TRX ) + ->willReturn( true ); + $this->databaseConnectionMock->expects( $this->any() ) + ->method( 'trxLevel' ) + ->willReturn( false ); + + $this->databaseConnectionMock->expects( $this->once() ) + ->method( 'query' ) + ->with( $sqlQuery, $this->anything() ); + + $this->databaseConnectionMock->expects( $this->never() ) + ->method( 'commit' ); + $this->databaseConnectionMock->expects( $this->once() ) + ->method( 'setFlag' ) + ->with( DBO_TRX ); + + $this->temporaryTableManager->queryWithAutoCommit( $sqlQuery ); + } + + /** + * @dataProvider sqlProvider + * @param string $sqlQuery + */ + public function testDboTrxFlagIsPreservedAndCommitIsCalledIfDboTrxIsSetWithOpenTransaction( + $sqlQuery + ) { + $this->databaseConnectionMock->expects( $this->any() ) + ->method( 'getFlag' ) + ->with( DBO_TRX ) + ->willReturn( true ); + $this->databaseConnectionMock->expects( $this->any() ) + ->method( 'trxLevel' ) + ->willReturn( true ); + + $this->databaseConnectionMock->expects( $this->once() ) + ->method( 'query' ) + ->with( $sqlQuery, $this->anything() ); + + $this->databaseConnectionMock->expects( $this->once() ) + ->method( 'commit' ); + $this->databaseConnectionMock->expects( $this->once() ) + ->method( 'setFlag' ) + ->with( DBO_TRX ); + + $this->temporaryTableManager->queryWithAutoCommit( $sqlQuery ); + } + + public function sqlProvider() { + return array( + array( 'CREATE TEMPORARY TABLE semantic_drilldown_values ( id INT NOT NULL )' ), + array( 'CREATE INDEX id_index ON semantic_drilldown_values ( id )' ), + array( 'INSERT INTO semantic_drilldown_values SELECT ids.smw_id AS id\n' ), + ); + } +} -- To view, visit https://gerrit.wikimedia.org/r/375785 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I51268b3eeaddb7cc0d49a19bc501117d5c248479 Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/extensions/SemanticDrilldown Gerrit-Branch: master Gerrit-Owner: TK-999 <tk.999.wi...@gmail.com> Gerrit-Reviewer: TK-999 <tk.999.wi...@gmail.com> Gerrit-Reviewer: Yaron Koren <yaro...@gmail.com> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
