From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sat, 24 Mar 2018 07:02:24 +0000 (-0700)
Subject: rdbms: make cancelAtomic() handle callbacks and work with DBO_TRX
X-Git-Tag: 1.34.0-rc.0~5668^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=20777d73998fb7024593752b0b3211d9d0e79b79

rdbms: make cancelAtomic() handle callbacks and work with DBO_TRX

Make transaction callbacks aware of cancelled sections. If the
statements of a section are reverted via cancelAtomic(), then the
dependant callbacks are now cancelled as well. Any callbacks for
onTransactionResolution(), which does not depend on COMMIT, will
see the triggering event as a ROLLBACK, since the unit of work it
was part of was rolled back.

Also fix the handling of topmost atomic sections with DBO_TRX.
These still need their own savepoint to make cancelAtomic() work.

Follow-up to 52aeaa7a5.

Change-Id: If4d455c98155283797678cfb9df31d5317dd91a2
---

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index f6817950ed..1af94cc494 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -109,11 +109,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	/** @var bool */
 	protected $opened = false;
 
-	/** @var array[] List of (callable, method name) */
+	/** @var array[] List of (callable, method name, atomic section id) */
 	protected $trxIdleCallbacks = [];
-	/** @var array[] List of (callable, method name) */
+	/** @var array[] List of (callable, method name, atomic section id) */
 	protected $trxPreCommitCallbacks = [];
-	/** @var array[] List of (callable, method name) */
+	/** @var array[] List of (callable, method name, atomic section id) */
 	protected $trxEndCallbacks = [];
 	/** @var callable[] Map of (name => callable) */
 	protected $trxRecurringCallbacks = [];
@@ -274,6 +274,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	/** @var int */
 	protected $nonNativeInsertSelectBatchSize = 10000;
 
+	/** @var string Idiom used when a cancelable atomic section started the transaction */
+	private static $NOT_APPLICABLE = 'n/a';
+	/** @var string Prefix to the atomic section counter used to make savepoint IDs */
+	private static $SAVEPOINT_PREFIX = 'wikimedia_rdbms_atomic';
+
 	/** @var int Transaction is in a error state requiring a full or savepoint rollback */
 	const STATUS_TRX_ERROR = 1;
 	/** @var int Transaction is active and in a normal state */
@@ -3231,7 +3236,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		if ( !$this->trxLevel ) {
 			throw new DBUnexpectedError( $this, "No transaction is active." );
 		}
-		$this->trxEndCallbacks[] = [ $callback, $fname ];
+		$this->trxEndCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ];
 	}
 
 	final public function onTransactionIdle( callable $callback, $fname = __METHOD__ ) {
@@ -3241,7 +3246,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			$this->trxAutomatic = true;
 		}
 
-		$this->trxIdleCallbacks[] = [ $callback, $fname ];
+		$this->trxIdleCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ];
 		if ( !$this->trxLevel ) {
 			$this->runOnTransactionIdleCallbacks( self::TRIGGER_IDLE );
 		}
@@ -3255,7 +3260,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		}
 
 		if ( $this->trxLevel ) {
-			$this->trxPreCommitCallbacks[] = [ $callback, $fname ];
+			$this->trxPreCommitCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ];
 		} else {
 			// No transaction is active nor will start implicitly, so make one for this callback
 			$this->startAtomic( __METHOD__, self::ATOMIC_CANCELABLE );
@@ -3269,6 +3274,72 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		}
 	}
 
+	/**
+	 * @return AtomicSectionIdentifier|null ID of the topmost atomic section level
+	 */
+	private function currentAtomicSectionId() {
+		if ( $this->trxLevel && $this->trxAtomicLevels ) {
+			$levelInfo = end( $this->trxAtomicLevels );
+
+			return $levelInfo[1];
+		}
+
+		return null;
+	}
+
+	/**
+	 * @param AtomicSectionIdentifier $old
+	 * @param AtomicSectionIdentifier $new
+	 */
+	private function reassignCallbacksForSection(
+		AtomicSectionIdentifier $old, AtomicSectionIdentifier $new
+	) {
+		foreach ( $this->trxPreCommitCallbacks as $key => $info ) {
+			if ( $info[2] === $old ) {
+				$this->trxPreCommitCallbacks[$key][2] = $new;
+			}
+		}
+		foreach ( $this->trxIdleCallbacks as $key => $info ) {
+			if ( $info[2] === $old ) {
+				$this->trxIdleCallbacks[$key][2] = $new;
+			}
+		}
+		foreach ( $this->trxEndCallbacks as $key => $info ) {
+			if ( $info[2] === $old ) {
+				$this->trxEndCallbacks[$key][2] = $new;
+			}
+		}
+	}
+
+	/**
+	 * @param AtomicSectionIdentifier[] $sectionIds ID of an actual savepoint
+	 * @throws UnexpectedValueException
+	 */
+	private function modifyCallbacksForCancel( array $sectionIds ) {
+		// Cancel the "on commit" callbacks owned by this savepoint
+		$this->trxIdleCallbacks = array_filter(
+			$this->trxIdleCallbacks,
+			function ( $entry ) use ( $sectionIds ) {
+				return !in_array( $entry[2], $sectionIds, true );
+			}
+		);
+		$this->trxPreCommitCallbacks = array_filter(
+			$this->trxPreCommitCallbacks,
+			function ( $entry ) use ( $sectionIds ) {
+				return !in_array( $entry[2], $sectionIds, true );
+			}
+		);
+		// Make "on resolution" callbacks owned by this savepoint to perceive a rollback
+		foreach ( $this->trxEndCallbacks as $key => $entry ) {
+			if ( in_array( $entry[2], $sectionIds, true ) ) {
+				$callback = $entry[0];
+				$this->trxEndCallbacks[$key][0] = function () use ( $callback ) {
+					return $callback( self::TRIGGER_ROLLBACK );
+				};
+			}
+		}
+	}
+
 	final public function setTransactionListener( $name, callable $callback = null ) {
 		if ( $callback ) {
 			$this->trxRecurringCallbacks[$name] = $callback;
@@ -3442,28 +3513,44 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		$this->query( 'ROLLBACK TO SAVEPOINT ' . $this->addIdentifierQuotes( $identifier ), $fname );
 	}
 
+	/**
+	 * @param string $fname
+	 * @return string
+	 */
+	private function nextSavepointId( $fname ) {
+		$savepointId = self::$SAVEPOINT_PREFIX . ++$this->trxAtomicCounter;
+		if ( strlen( $savepointId ) > 30 ) {
+			// 30 == Oracle's identifier length limit (pre 12c)
+			// With a 22 character prefix, that puts the highest number at 99999999.
+			throw new DBUnexpectedError(
+				$this,
+				'There have been an excessively large number of atomic sections in a transaction'
+				. " started by $this->trxFname (at $fname)"
+			);
+		}
+
+		return $savepointId;
+	}
+
 	final public function startAtomic(
 		$fname = __METHOD__, $cancelable = self::ATOMIC_NOT_CANCELABLE
 	) {
-		$savepointId = $cancelable === self::ATOMIC_CANCELABLE ? 'n/a' : null;
+		$savepointId = $cancelable === self::ATOMIC_CANCELABLE ? self::$NOT_APPLICABLE : null;
+
 		if ( !$this->trxLevel ) {
 			$this->begin( $fname, self::TRANSACTION_INTERNAL );
 			// If DBO_TRX is set, a series of startAtomic/endAtomic pairs will result
 			// in all changes being in one transaction to keep requests transactional.
-			if ( !$this->getFlag( self::DBO_TRX ) ) {
+			if ( $this->getFlag( self::DBO_TRX ) ) {
+				// Since writes could happen in between the topmost atomic sections as part
+				// of the transaction, those sections will need savepoints.
+				$savepointId = $this->nextSavepointId( $fname );
+				$this->doSavepoint( $savepointId, $fname );
+			} else {
 				$this->trxAutomaticAtomic = true;
 			}
 		} 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(
-					'There have been an excessively large number of atomic sections in a transaction'
-					. " started by $this->trxFname, reusing IDs (at $fname)",
-					[ 'trace' => ( new RuntimeException() )->getTraceAsString() ]
-				);
-				$this->trxAtomicCounter = 0;
-				$savepointId = 'wikimedia_rdbms_atomic' . ++$this->trxAtomicCounter;
-			}
+			$savepointId = $this->nextSavepointId( $fname );
 			$this->doSavepoint( $savepointId, $fname );
 		}
 
@@ -3480,7 +3567,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
 		// Check if the current section matches $fname
 		$pos = count( $this->trxAtomicLevels ) - 1;
-		list( $savedFname, , $savepointId ) = $this->trxAtomicLevels[$pos];
+		list( $savedFname, $sectionId, $savepointId ) = $this->trxAtomicLevels[$pos];
 
 		if ( $savedFname !== $fname ) {
 			throw new DBUnexpectedError(
@@ -3489,14 +3576,21 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			);
 		}
 
-		// Remove the last section and re-index the array
-		$this->trxAtomicLevels = array_slice( $this->trxAtomicLevels, 0, $pos );
+		// Remove the last section (no need to re-index the array)
+		array_pop( $this->trxAtomicLevels );
 
 		if ( !$this->trxAtomicLevels && $this->trxAutomaticAtomic ) {
 			$this->commit( $fname, self::FLUSHING_INTERNAL );
-		} elseif ( $savepointId !== null && $savepointId !== 'n/a' ) {
+		} elseif ( $savepointId !== null && $savepointId !== self::$NOT_APPLICABLE ) {
 			$this->doReleaseSavepoint( $savepointId, $fname );
 		}
+
+		// Hoist callback ownership for callbacks in the section that just ended;
+		// all callbacks should have an owner that is present in trxAtomicLevels.
+		$currentSectionId = $this->currentAtomicSectionId();
+		if ( $currentSectionId ) {
+			$this->reassignCallbacksForSection( $sectionId, $currentSectionId );
+		}
 	}
 
 	final public function cancelAtomic(
@@ -3518,12 +3612,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 				throw new DBUnexpectedError( "Atomic section not found (for $fname)" );
 			}
 			// Remove all descendant sections and re-index the array
+			$excisedIds = [];
+			$len = count( $this->trxAtomicLevels );
+			for ( $i = $pos + 1; $i < $len; ++$i ) {
+				$excisedIds[] = $this->trxAtomicLevels[$i][1];
+			}
 			$this->trxAtomicLevels = array_slice( $this->trxAtomicLevels, 0, $pos + 1 );
+			$this->modifyCallbacksForCancel( $excisedIds );
 		}
 
 		// Check if the current section matches $fname
 		$pos = count( $this->trxAtomicLevels ) - 1;
-		list( $savedFname, , $savepointId ) = $this->trxAtomicLevels[$pos];
+		list( $savedFname, $savedSectionId, $savepointId ) = $this->trxAtomicLevels[$pos];
 
 		if ( $savedFname !== $fname ) {
 			throw new DBUnexpectedError(
@@ -3532,12 +3632,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			);
 		}
 
-		// Remove the last section and re-index the array
-		$this->trxAtomicLevels = array_slice( $this->trxAtomicLevels, 0, $pos );
+		// Remove the last section (no need to re-index the array)
+		array_pop( $this->trxAtomicLevels );
+		$this->modifyCallbacksForCancel( [ $savedSectionId ] );
 
 		if ( $savepointId !== null ) {
 			// Rollback the transaction to the state just before this atomic section
-			if ( $savepointId === 'n/a' ) {
+			if ( $savepointId === self::$NOT_APPLICABLE ) {
 				$this->rollback( $fname, self::FLUSHING_INTERNAL );
 			} else {
 				$this->doRollbackToSavepoint( $savepointId, $fname );
diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index 3756da2613..cf10801c49 100644
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -1418,6 +1418,140 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
 		// phpcs:ignore Generic.Files.LineLength
 		$this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; RELEASE SAVEPOINT wikimedia_rdbms_atomic1; ROLLBACK' );
 
+		$fname = __METHOD__;
+		$triggerMap = [
+			'-' => '-',
+			IDatabase::TRIGGER_COMMIT => 'tCommit',
+			IDatabase::TRIGGER_ROLLBACK => 'tRollback'
+		];
+		$callback1 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) {
+			$this->database->query( "SELECT 1, {$triggerMap[$trigger]} AS t", $fname );
+		};
+		$callback2 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) {
+			$this->database->query( "SELECT 2, {$triggerMap[$trigger]} AS t", $fname );
+		};
+		$callback3 = function ( $trigger = '-' ) use ( $fname, $triggerMap ) {
+			$this->database->query( "SELECT 3, {$triggerMap[$trigger]} AS t", $fname );
+		};
+
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionPreCommitOrIdle( $callback1, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->assertLastSql( 'BEGIN; ROLLBACK' );
+
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->assertLastSql( 'BEGIN; ROLLBACK' );
+
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionResolution( $callback1, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->assertLastSql( 'BEGIN; ROLLBACK; SELECT 1, tRollback AS t' );
+
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->onTransactionPreCommitOrIdle( $callback1, __METHOD__ );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback3, __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertLastSql( implode( "; ", [
+			'BEGIN',
+			'SAVEPOINT wikimedia_rdbms_atomic1',
+			'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+			'SELECT 1, - AS t',
+			'SELECT 3, - AS t',
+			'COMMIT'
+		] ) );
+
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionIdle( $callback2, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->onTransactionIdle( $callback3, __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertLastSql( implode( "; ", [
+			'BEGIN',
+			'SAVEPOINT wikimedia_rdbms_atomic1',
+			'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+			'COMMIT',
+			'SELECT 1, tCommit AS t',
+			'SELECT 3, tCommit AS t'
+		] ) );
+
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->onTransactionResolution( $callback1, __METHOD__ );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionResolution( $callback2, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertLastSql( implode( "; ", [
+			'BEGIN',
+			'SAVEPOINT wikimedia_rdbms_atomic1',
+			'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+			'COMMIT',
+			'SELECT 1, tCommit AS t',
+			'SELECT 2, tRollback AS t',
+			'SELECT 3, tCommit AS t'
+		] ) );
+
+		$makeCallback = function ( $id ) use ( $fname, $triggerMap ) {
+			return function ( $trigger = '-' ) use ( $id, $fname, $triggerMap ) {
+				$this->database->query( "SELECT $id, {$triggerMap[$trigger]} AS t", $fname );
+			};
+		};
+
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionResolution( $makeCallback( 1 ), __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertLastSql( implode( "; ", [
+			'BEGIN',
+			'SAVEPOINT wikimedia_rdbms_atomic1',
+			'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+			'COMMIT',
+			'SELECT 1, tRollback AS t'
+		] ) );
+
+		$this->database->startAtomic( __METHOD__ . '_level1', IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionResolution( $makeCallback( 1 ), __METHOD__ );
+		$this->database->startAtomic( __METHOD__ . '_level2' );
+		$this->database->startAtomic( __METHOD__ . '_level3', IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionResolution( $makeCallback( 2 ), __METHOD__ );
+		$this->database->endAtomic( __METHOD__ );
+		$this->database->onTransactionResolution( $makeCallback( 3 ), __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ . '_level3' );
+		$this->database->endAtomic( __METHOD__ . '_level2' );
+		$this->database->onTransactionResolution( $makeCallback( 4 ), __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_level1' );
+		$this->assertLastSql( implode( "; ", [
+			'BEGIN',
+			'SAVEPOINT wikimedia_rdbms_atomic1',
+			'SAVEPOINT wikimedia_rdbms_atomic2',
+			'RELEASE SAVEPOINT wikimedia_rdbms_atomic2',
+			'ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1',
+			'COMMIT; SELECT 1, tCommit AS t',
+			'SELECT 2, tRollback AS t',
+			'SELECT 3, tRollback AS t',
+			'SELECT 4, tCommit AS t'
+		] ) );
+	}
+
+	/**
+	 * @covers \Wikimedia\Rdbms\Database::doSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::doReleaseSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::doRollbackToSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::startAtomic
+	 * @covers \Wikimedia\Rdbms\Database::endAtomic
+	 * @covers \Wikimedia\Rdbms\Database::cancelAtomic
+	 * @covers \Wikimedia\Rdbms\Database::doAtomicSection
+	 */
+	public function testAtomicSectionsRecovery() {
 		$this->database->begin( __METHOD__ );
 		try {
 			$this->database->doAtomicSection(
@@ -1450,10 +1584,169 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
 		} catch ( RuntimeException $ex ) {
 			$this->assertSame( 'Test exception', $ex->getMessage() );
 		}
+		try {
+			$this->database->commit( __METHOD__ );
+			$this->fail( 'Test exception not thrown' );
+		} catch ( DBTransactionError $ex ) {
+			$this->assertSame(
+				'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.',
+				$ex->getMessage()
+			);
+		}
 		$this->database->rollback( __METHOD__ );
 		$this->assertLastSql( 'BEGIN; ROLLBACK' );
 	}
 
+	/**
+	 * @covers \Wikimedia\Rdbms\Database::doSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::doReleaseSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::doRollbackToSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::startAtomic
+	 * @covers \Wikimedia\Rdbms\Database::endAtomic
+	 * @covers \Wikimedia\Rdbms\Database::cancelAtomic
+	 * @covers \Wikimedia\Rdbms\Database::doAtomicSection
+	 */
+	public function testAtomicSectionsCallbackCancellation() {
+		$fname = __METHOD__;
+		$callback1Called = null;
+		$callback1 = function ( $trigger = '-' ) use ( $fname, &$callback1Called ) {
+			$callback1Called = $trigger;
+			$this->database->query( "SELECT 1", $fname );
+		};
+		$callback2Called = null;
+		$callback2 = function ( $trigger = '-' ) use ( $fname, &$callback2Called ) {
+			$callback2Called = $trigger;
+			$this->database->query( "SELECT 2", $fname );
+		};
+		$callback3Called = null;
+		$callback3 = function ( $trigger = '-' ) use ( $fname, &$callback3Called ) {
+			$callback3Called = $trigger;
+			$this->database->query( "SELECT 3", $fname );
+		};
+
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__ . '_inner' );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_inner' );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertNull( $callback1Called );
+		$this->assertNull( $callback2Called );
+		$this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+		// phpcs:ignore Generic.Files.LineLength
+		$this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1; COMMIT; SELECT 3' );
+
+		$callback1Called = null;
+		$callback2Called = null;
+		$callback3Called = null;
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__ . '_inner', IDatabase::ATOMIC_CANCELABLE );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_inner' );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertNull( $callback1Called );
+		$this->assertNull( $callback2Called );
+		$this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+		// phpcs:ignore Generic.Files.LineLength
+		$this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; SAVEPOINT wikimedia_rdbms_atomic2; RELEASE SAVEPOINT wikimedia_rdbms_atomic2; ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1; COMMIT; SELECT 3' );
+
+		$callback1Called = null;
+		$callback2Called = null;
+		$callback3Called = null;
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$atomicId = $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__ . '_inner' );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__, $atomicId );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertNull( $callback1Called );
+		$this->assertNull( $callback2Called );
+		$this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+
+		$callback1Called = null;
+		$callback2Called = null;
+		$callback3Called = null;
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$atomicId = $this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__ . '_inner' );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		try {
+			$this->database->cancelAtomic( __METHOD__ . '_X', $atomicId );
+		} catch ( DBUnexpectedError $e ) {
+			$m = __METHOD__;
+			$this->assertSame(
+				"Invalid atomic section ended (got {$m}_X but expected {$m}).",
+				$e->getMessage()
+			);
+		}
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertNull( $callback1Called );
+		$this->assertNull( $callback2Called );
+		$this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__ . '_inner' );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		$this->database->cancelAtomic( __METHOD__ . '_inner' );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertNull( $callback1Called );
+		$this->assertNull( $callback2Called );
+		$this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+
+		$wrapper = TestingAccessWrapper::newFromObject( $this->database );
+		$callback1Called = null;
+		$callback2Called = null;
+		$callback3Called = null;
+		$this->database->startAtomic( __METHOD__ . '_outer' );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->startAtomic( __METHOD__ . '_inner' );
+		$this->database->onTransactionIdle( $callback1, __METHOD__ );
+		$this->database->onTransactionPreCommitOrIdle( $callback2, __METHOD__ );
+		$this->database->onTransactionResolution( $callback3, __METHOD__ );
+		$wrapper->trxStatus = Database::STATUS_TRX_ERROR;
+		$this->database->cancelAtomic( __METHOD__ . '_inner' );
+		$this->database->cancelAtomic( __METHOD__ );
+		$this->database->endAtomic( __METHOD__ . '_outer' );
+		$this->assertNull( $callback1Called );
+		$this->assertNull( $callback2Called );
+		$this->assertEquals( IDatabase::TRIGGER_ROLLBACK, $callback3Called );
+	}
+
+	/**
+	 * @covers \Wikimedia\Rdbms\Database::doSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::doReleaseSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::doRollbackToSavepoint
+	 * @covers \Wikimedia\Rdbms\Database::startAtomic
+	 * @covers \Wikimedia\Rdbms\Database::endAtomic
+	 * @covers \Wikimedia\Rdbms\Database::cancelAtomic
+	 * @covers \Wikimedia\Rdbms\Database::doAtomicSection
+	 */
+	public function testAtomicSectionsTrxRound() {
+		$this->database->setFlag( IDatabase::DBO_TRX );
+		$this->database->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
+		$this->database->query( 'SELECT 1', __METHOD__ );
+		$this->database->endAtomic( __METHOD__ );
+		$this->database->commit( __METHOD__, IDatabase::FLUSHING_ALL_PEERS );
+		// phpcs:ignore Generic.Files.LineLength
+		$this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; SELECT 1; RELEASE SAVEPOINT wikimedia_rdbms_atomic1; COMMIT' );
+	}
+
 	public static function provideAtomicSectionMethodsForErrors() {
 		return [
 			[ 'endAtomic' ],
diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index 94b4e0d995..e328cba6aa 100644
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -310,14 +310,18 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
 		$lb = $lbFactory->getMainLB();
 		$conn = $lb->openConnection( $lb->getWriterIndex() );
 		$this->assertSame( $db, $conn, 'Same DB instance' );
-		$this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
 
+		$this->assertFalse( $lb->hasMasterChanges() );
+		$this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
 		$called = false;
 		$callback = function () use ( &$called ) {
 			$called = true;
 		};
 		$db->onTransactionPreCommitOrIdle( $callback, __METHOD__ );
 		$this->assertTrue( $called, 'Called when idle if DBO_TRX is set' );
+		$called = false;
+		$lbFactory->commitMasterChanges();
+		$this->assertFalse( $called );
 
 		$called = false;
 		$lbFactory->beginMasterChanges( __METHOD__ );