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

diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php
index c0855df..24bab7d 100644 (file)
--- 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 c3e36da..118e714 100644 (file)
--- 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 47954b6..cd2bbf6 100644 (file)
--- 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 badba96..981c407 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -1,5 +1,6 @@
 <?php
 
+use Wikimedia\Rdbms\IDatabase;
 use Wikimedia\Rdbms\LikeMatch;
 use Wikimedia\Rdbms\Database;
 
@@ -1366,7 +1367,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
                $this->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()
+                       );
+               }
+       }
+
 }