From 3365e83d960c43fc25082b2dbbdabb366beecbce Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 20 Mar 2018 11:57:04 -0400 Subject: [PATCH] rdbms: Add ATOMIC_CANCELABLE flag for micro-optimization Aaron is concerned about the extra time added to atomic sections within an outer transaction if we do a SAVEPOINT and RELEASE. He wants a flag so callers have to specifically opt-in to use of savepoints. Change-Id: I64cf5033ced464863d28dd49d9173856a9c1e1c0 --- includes/libs/rdbms/database/DBConnRef.php | 4 ++- includes/libs/rdbms/database/Database.php | 29 ++++++++++------- includes/libs/rdbms/database/IDatabase.php | 21 +++++++++---- .../libs/rdbms/database/DatabaseSQLTest.php | 31 ++++++++++++++++--- 4 files changed, 62 insertions(+), 23 deletions(-) diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index c0855dfc2a..24bab7d55a 100644 --- a/includes/libs/rdbms/database/DBConnRef.php +++ b/includes/libs/rdbms/database/DBConnRef.php @@ -511,7 +511,9 @@ class DBConnRef implements IDatabase { return $this->__call( __FUNCTION__, func_get_args() ); } - public function startAtomic( $fname = __METHOD__ ) { + public function startAtomic( + $fname = __METHOD__, $cancelable = IDatabase::ATOMIC_NOT_CANCELABLE + ) { return $this->__call( __FUNCTION__, func_get_args() ); } diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index c3e36dabda..118e7143d3 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -2518,7 +2518,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } try { - $this->startAtomic( $fname ); + $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); $affectedRowCount = 0; foreach ( $rows as $row ) { // Delete rows which collide with this one @@ -2623,7 +2623,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $affectedRowCount = 0; try { - $this->startAtomic( $fname ); + $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); # Update any existing conflicting row(s) if ( $where !== false ) { $ok = $this->update( $table, $set, $where, $fname ); @@ -2779,7 +2779,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware try { $affectedRowCount = 0; - $this->startAtomic( $fname ); + $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); $rows = []; $ok = true; foreach ( $res as $row ) { @@ -3095,7 +3095,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->trxPreCommitCallbacks[] = [ $callback, $fname ]; } else { // No transaction is active nor will start implicitly, so make one for this callback - $this->startAtomic( __METHOD__ ); + $this->startAtomic( __METHOD__, self::ATOMIC_CANCELABLE ); try { call_user_func( $callback ); $this->endAtomic( __METHOD__ ); @@ -3279,7 +3279,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->query( 'ROLLBACK TO SAVEPOINT ' . $this->addIdentifierQuotes( $identifier ), $fname ); } - final public function startAtomic( $fname = __METHOD__ ) { + final public function startAtomic( + $fname = __METHOD__, $cancelable = self::ATOMIC_NOT_CANCELABLE + ) { + $savepointId = $cancelable === self::ATOMIC_CANCELABLE ? 'n/a' : null; if ( !$this->trxLevel ) { $this->begin( $fname, self::TRANSACTION_INTERNAL ); // If DBO_TRX is set, a series of startAtomic/endAtomic pairs will result @@ -3287,8 +3290,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( !$this->getFlag( self::DBO_TRX ) ) { $this->trxAutomaticAtomic = true; } - $savepointId = null; - } else { + } elseif ( $cancelable === self::ATOMIC_CANCELABLE ) { $savepointId = 'wikimedia_rdbms_atomic' . ++$this->trxAtomicCounter; if ( strlen( $savepointId ) > 30 ) { // 30 == Oracle's identifier length limit (pre 12c) $this->queryLogger->warning( @@ -3316,9 +3318,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware throw new DBUnexpectedError( $this, "Invalid atomic section ended (got $fname)." ); } - if ( !$savepointId ) { + if ( !$this->trxAtomicLevels && $this->trxAutomaticAtomic ) { $this->commit( $fname, self::FLUSHING_INTERNAL ); - } else { + } elseif ( $savepointId && $savepointId !== 'n/a' ) { $this->doReleaseSavepoint( $savepointId, $fname ); } } @@ -3333,10 +3335,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $savedFname !== $fname ) { throw new DBUnexpectedError( $this, "Invalid atomic section ended (got $fname)." ); } - if ( !$savepointId ) { + throw new DBUnexpectedError( $this, "Uncancelable atomic section canceled (got $fname)." ); + } + + if ( !$this->trxAtomicLevels && $this->trxAutomaticAtomic ) { $this->rollback( $fname, self::FLUSHING_INTERNAL ); - } else { + } elseif ( $savepointId !== 'n/a' ) { $this->doRollbackToSavepoint( $savepointId, $fname ); } @@ -3344,7 +3349,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } final public function doAtomicSection( $fname, callable $callback ) { - $this->startAtomic( $fname ); + $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); try { $res = call_user_func_array( $callback, [ $this, $fname ] ); } catch ( Exception $e ) { diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 47954b6e9e..cd2bbf6191 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -49,6 +49,11 @@ interface IDatabase { /** @var string Transaction is requested internally via DBO_TRX/startAtomic() */ const TRANSACTION_INTERNAL = 'implicit'; + /** @var string Atomic section is not cancelable */ + const ATOMIC_NOT_CANCELABLE = ''; + /** @var string Atomic section is cancelable */ + const ATOMIC_CANCELABLE = 'cancelable'; + /** @var string Transaction operation comes from service managing all DBs */ const FLUSHING_ALL_PEERS = 'flush'; /** @var string Transaction operation comes from the database class internally */ @@ -1580,11 +1585,11 @@ interface IDatabase { /** * Begin an atomic section of statements * - * If a transaction has been started already, sets a savepoint and tracks - * the given section name to make sure the transaction is not committed - * pre-maturely. This function can be used in layers (with sub-sections), - * so use a stack to keep track of the different atomic sections. If there - * is no transaction, one is started implicitly. + * If a transaction has been started already, (optionally) sets a savepoint + * and tracks the given section name to make sure the transaction is not + * committed pre-maturely. This function can be used in layers (with + * sub-sections), so use a stack to keep track of the different atomic + * sections. If there is no transaction, one is started implicitly. * * The goal of this function is to create an atomic section of SQL queries * without having to start a new transaction if it already exists. @@ -1596,9 +1601,11 @@ interface IDatabase { * * @since 1.23 * @param string $fname + * @param string $cancelable Pass self::ATOMIC_CANCELABLE to use a + * savepoint and enable self::cancelAtomic() for this section. * @throws DBError */ - public function startAtomic( $fname = __METHOD__ ); + public function startAtomic( $fname = __METHOD__, $cancelable = self::ATOMIC_NOT_CANCELABLE ); /** * Ends an atomic section of SQL statements @@ -1626,6 +1633,8 @@ interface IDatabase { * Note that a call to IDatabase::rollback() will also roll back any open * atomic sections. * + * @note As a micro-optimization to save a few DB calls, this method may only + * be called when startAtomic() was called with the ATOMIC_CANCELABLE flag. * @since 1.31 * @see IDatabase::startAtomic * @param string $fname diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index badba9616f..981c4075fe 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -1,5 +1,6 @@ database->endAtomic( __METHOD__ ); $this->assertLastSql( 'BEGIN; COMMIT' ); - $this->database->startAtomic( __METHOD__ ); + $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); $this->database->cancelAtomic( __METHOD__ ); $this->assertLastSql( 'BEGIN; ROLLBACK' ); @@ -1374,18 +1375,24 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->database->startAtomic( __METHOD__ ); $this->database->endAtomic( __METHOD__ ); $this->database->commit( __METHOD__ ); + $this->assertLastSql( 'BEGIN; COMMIT' ); + + $this->database->begin( __METHOD__ ); + $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); + $this->database->endAtomic( __METHOD__ ); + $this->database->commit( __METHOD__ ); // phpcs:ignore Generic.Files.LineLength $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; RELEASE SAVEPOINT wikimedia_rdbms_atomic1; COMMIT' ); $this->database->begin( __METHOD__ ); - $this->database->startAtomic( __METHOD__ ); + $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); $this->database->cancelAtomic( __METHOD__ ); $this->database->commit( __METHOD__ ); // phpcs:ignore Generic.Files.LineLength $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1; COMMIT' ); - $this->database->startAtomic( __METHOD__ ); - $this->database->startAtomic( __METHOD__ ); + $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); + $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE ); $this->database->cancelAtomic( __METHOD__ ); $this->database->endAtomic( __METHOD__ ); // phpcs:ignore Generic.Files.LineLength @@ -1458,4 +1465,20 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } } + /** + * @covers \Wikimedia\Rdbms\Database::cancelAtomic + */ + public function testUncancellableAtomicSection() { + $this->database->startAtomic( __METHOD__ ); + try { + $this->database->cancelAtomic( __METHOD__ ); + $this->fail( 'Expected exception not thrown' ); + } catch ( DBUnexpectedError $ex ) { + $this->assertSame( + 'Uncancelable atomic section canceled (got ' . __METHOD__ . ').', + $ex->getMessage() + ); + } + } + } -- 2.20.1