From 04d591935cf117a890ae890912ec59b560da4f54 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 28 Jun 2019 20:21:28 -0700 Subject: [PATCH] rdbms: normalize Database/LBFactory logging and add snapshot flushing warnings Make flushSnapshot() logging more detailed and check for explicit transaction rounds. Also removing periods and trailing newlines from log/exception messages. Change-Id: I0f6520f563680ab3a65b6338ced59ba25a2ec7b5 --- includes/libs/rdbms/database/DBConnRef.php | 6 +- includes/libs/rdbms/database/Database.php | 132 ++++++++++-------- includes/libs/rdbms/database/IDatabase.php | 16 ++- includes/libs/rdbms/lbfactory/ILBFactory.php | 2 + includes/libs/rdbms/lbfactory/LBFactory.php | 34 +++-- .../libs/rdbms/loadbalancer/ILoadBalancer.php | 4 +- .../libs/rdbms/loadbalancer/LoadBalancer.php | 4 +- .../libs/rdbms/database/DatabaseSQLTest.php | 22 +-- 8 files changed, 134 insertions(+), 86 deletions(-) diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index 76701f853a..2cc2c90d69 100644 --- a/includes/libs/rdbms/database/DBConnRef.php +++ b/includes/libs/rdbms/database/DBConnRef.php @@ -649,15 +649,15 @@ class DBConnRef implements IDatabase { return $this->__call( __FUNCTION__, func_get_args() ); } - public function commit( $fname = __METHOD__, $flush = '' ) { + public function commit( $fname = __METHOD__, $flush = self::FLUSHING_ONE ) { return $this->__call( __FUNCTION__, func_get_args() ); } - public function rollback( $fname = __METHOD__, $flush = '' ) { + public function rollback( $fname = __METHOD__, $flush = self::FLUSHING_ONE ) { return $this->__call( __FUNCTION__, func_get_args() ); } - public function flushSnapshot( $fname = __METHOD__ ) { + public function flushSnapshot( $fname = __METHOD__, $flush = self::FLUSHING_ONE ) { return $this->__call( __FUNCTION__, func_get_args() ); } diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 92b94716d8..e3c15fb35a 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -270,7 +270,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ final public function initConnection() { if ( $this->isOpen() ) { - throw new LogicException( __METHOD__ . ': already connected.' ); + throw new LogicException( __METHOD__ . ': already connected' ); } // Establish the connection $this->doInitConnection(); @@ -294,7 +294,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->connectionParams['tablePrefix'] ); } else { - throw new InvalidArgumentException( "No database user provided." ); + throw new InvalidArgumentException( "No database user provided" ); } } @@ -541,7 +541,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware public function dbSchema( $schema = null ) { if ( strlen( $schema ) && $this->getDBname() === null ) { - throw new DBUnexpectedError( $this, "Cannot set schema to '$schema'; no database set." ); + throw new DBUnexpectedError( $this, "Cannot set schema to '$schema'; no database set" ); } $old = $this->currentDomain->getSchema(); @@ -726,7 +726,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware public function setFlag( $flag, $remember = self::REMEMBER_NOTHING ) { if ( ( $flag & self::DBO_IGNORE ) ) { - throw new UnexpectedValueException( "Modifying DBO_IGNORE is not allowed." ); + throw new UnexpectedValueException( "Modifying DBO_IGNORE is not allowed" ); } if ( $remember === self::REMEMBER_PRIOR ) { @@ -737,7 +737,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware public function clearFlag( $flag, $remember = self::REMEMBER_NOTHING ) { if ( ( $flag & self::DBO_IGNORE ) ) { - throw new UnexpectedValueException( "Modifying DBO_IGNORE is not allowed." ); + throw new UnexpectedValueException( "Modifying DBO_IGNORE is not allowed" ); } if ( $remember === self::REMEMBER_PRIOR ) { @@ -875,7 +875,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $levels = $this->flatAtomicSectionList(); $exception = new DBUnexpectedError( $this, - __METHOD__ . ": atomic sections $levels are still open." + __METHOD__ . ": atomic sections $levels are still open" ); } elseif ( $this->trxAutomatic ) { // Only the connection manager can commit non-empty DBO_TRX transactions @@ -884,7 +884,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $exception = new DBUnexpectedError( $this, __METHOD__ . - ": mass commit/rollback of peer transaction required (DBO_TRX set)." + ": mass commit/rollback of peer transaction required (DBO_TRX set)" ); } } else { @@ -892,14 +892,14 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware // back, even if empty. $exception = new DBUnexpectedError( $this, - __METHOD__ . ": transaction is still open (from {$this->trxFname})." + __METHOD__ . ": transaction is still open (from {$this->trxFname})" ); } if ( $this->trxEndCallbacksSuppressed ) { $exception = $exception ?: new DBUnexpectedError( $this, - __METHOD__ . ': callbacks are suppressed; cannot properly commit.' + __METHOD__ . ': callbacks are suppressed; cannot properly commit' ); } @@ -929,7 +929,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $fnames = $this->pendingWriteAndCallbackCallers(); if ( $fnames ) { throw new RuntimeException( - "Transaction callbacks are still pending:\n" . implode( ', ', $fnames ) + "Transaction callbacks are still pending: " . implode( ', ', $fnames ) ); } } @@ -947,7 +947,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ final protected function assertHasConnectionHandle() { if ( !$this->isOpen() ) { - throw new DBUnexpectedError( $this, "DB connection was already closed." ); + throw new DBUnexpectedError( $this, "DB connection was already closed" ); } } @@ -961,7 +961,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $this->getLBInfo( 'replica' ) === true ) { throw new DBReadOnlyRoleError( $this, - 'Write operations are not allowed on replica database connections.' + 'Write operations are not allowed on replica database connections' ); } $reason = $this->getReadOnlyReason(); @@ -1401,7 +1401,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware private function assertQueryIsCurrentlyAllowed( $sql, $fname ) { $verb = $this->getQueryVerb( $sql ); if ( $verb === 'USE' ) { - throw new DBUnexpectedError( $this, "Got USE query; use selectDomain() instead." ); + throw new DBUnexpectedError( $this, "Got USE query; use selectDomain() instead" ); } if ( $verb === 'ROLLBACK' ) { // transaction/savepoint @@ -1411,7 +1411,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $this->trxStatus < self::STATUS_TRX_OK ) { throw new DBTransactionStateError( $this, - "Cannot execute query from $fname while transaction status is ERROR.", + "Cannot execute query from $fname while transaction status is ERROR", [], $this->trxStatusCause ); @@ -1552,7 +1552,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ public function reportQueryError( $error, $errno, $sql, $fname, $ignore = false ) { if ( $ignore ) { - $this->queryLogger->debug( "SQL ERROR (ignored): $error\n" ); + $this->queryLogger->debug( "SQL ERROR (ignored): $error" ); } else { $exception = $this->getQueryExceptionAndLog( $error, $errno, $sql, $fname ); @@ -1580,7 +1580,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware 'trace' => ( new RuntimeException() )->getTraceAsString() ] ) ); - $this->queryLogger->debug( "SQL ERROR: " . $error . "\n" ); + $this->queryLogger->debug( "SQL ERROR: " . $error . "" ); if ( $this->wasQueryTimeout( $error, $errno ) ) { $e = new DBQueryTimeoutError( $this, $error, $errno, $sql, $fname ); } elseif ( $this->wasConnectionError( $errno ) ) { @@ -1813,7 +1813,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware // functions. Discourage use of such queries to encourage compatibility. call_user_func( $this->deprecationLogger, - __METHOD__ . ": aggregation used with a locking SELECT ($fname)." + __METHOD__ . ": aggregation used with a locking SELECT ($fname)" ); } @@ -2010,7 +2010,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } elseif ( count( $var ) == 1 ) { $column = $var[0] ?? reset( $var ); } else { - throw new DBUnexpectedError( $this, __METHOD__ . ': got multiple columns.' ); + throw new DBUnexpectedError( $this, __METHOD__ . ': got multiple columns' ); } } else { $column = $var; @@ -2380,7 +2380,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $name instanceof Subquery ) { throw new DBUnexpectedError( $this, - __METHOD__ . ': got Subquery instance when expecting a string.' + __METHOD__ . ': got Subquery instance when expecting a string' ); } @@ -2401,7 +2401,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware # surrounded by symbols which may be considered word breaks. if ( preg_match( '/(^|\s)(DISTINCT|JOIN|ON|AS)(\s|$)/i', $name ) !== 0 ) { $this->queryLogger->warning( - __METHOD__ . ": use of subqueries is not supported this way.", + __METHOD__ . ": use of subqueries is not supported this way", [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] ); @@ -2524,12 +2524,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } elseif ( $table instanceof Subquery ) { $quotedTable = (string)$table; } else { - throw new InvalidArgumentException( "Table must be a string or Subquery." ); + throw new InvalidArgumentException( "Table must be a string or Subquery" ); } if ( $alias === false || $alias === $table ) { if ( $table instanceof Subquery ) { - throw new InvalidArgumentException( "Subquery table missing alias." ); + throw new InvalidArgumentException( "Subquery table missing alias" ); } return $quotedTable; @@ -3162,8 +3162,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware public function limitResult( $sql, $limit, $offset = false ) { if ( !is_numeric( $limit ) ) { - throw new DBUnexpectedError( $this, - "Invalid non-numeric limit passed to limitResult()\n" ); + throw new DBUnexpectedError( + $this, + "Invalid non-numeric limit passed to " . __METHOD__ + ); } // This version works in MySQL and SQLite. It will very likely need to be // overridden for most other RDBMS subclasses. @@ -3370,7 +3372,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function onTransactionResolution( callable $callback, $fname = __METHOD__ ) { if ( !$this->trxLevel() ) { - throw new DBUnexpectedError( $this, "No transaction is active." ); + throw new DBUnexpectedError( $this, "No transaction is active" ); } $this->trxEndCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ]; } @@ -3416,7 +3418,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function onAtomicSectionCancel( callable $callback, $fname = __METHOD__ ) { if ( !$this->trxLevel() || !$this->trxAtomicLevels ) { - throw new DBUnexpectedError( $this, "No atomic section is open (got $fname)." ); + throw new DBUnexpectedError( $this, "No atomic section is open (got $fname)" ); } $this->trxSectionCancelCallbacks[] = [ $callback, $fname, $this->currentAtomicSectionId() ]; } @@ -3552,7 +3554,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ public function runOnTransactionIdleCallbacks( $trigger ) { if ( $this->trxLevel() ) { // sanity - throw new DBUnexpectedError( $this, __METHOD__ . ': a transaction is still open.' ); + throw new DBUnexpectedError( $this, __METHOD__ . ': a transaction is still open' ); } if ( $this->trxEndCallbacksSuppressed ) { @@ -3809,7 +3811,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function endAtomic( $fname = __METHOD__ ) { if ( !$this->trxLevel() || !$this->trxAtomicLevels ) { - throw new DBUnexpectedError( $this, "No atomic section is open (got $fname)." ); + throw new DBUnexpectedError( $this, "No atomic section is open (got $fname)" ); } // Check if the current section matches $fname @@ -3820,7 +3822,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $savedFname !== $fname ) { throw new DBUnexpectedError( $this, - "Invalid atomic section ended (got $fname but expected $savedFname)." + "Invalid atomic section ended (got $fname but expected $savedFname)" ); } @@ -3845,7 +3847,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $fname = __METHOD__, AtomicSectionIdentifier $sectionId = null ) { if ( !$this->trxLevel() || !$this->trxAtomicLevels ) { - throw new DBUnexpectedError( $this, "No atomic section is open (got $fname)." ); + throw new DBUnexpectedError( $this, "No atomic section is open (got $fname)" ); } $excisedIds = []; @@ -3887,7 +3889,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $savedFname !== $fname ) { throw new DBUnexpectedError( $this, - "Invalid atomic section ended (got $fname but expected $savedFname)." + "Invalid atomic section ended (got $fname but expected $savedFname)" ); } @@ -3914,7 +3916,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->trxStatus = self::STATUS_TRX_ERROR; $this->trxStatusCause = new DBUnexpectedError( $this, - "Uncancelable atomic section canceled (got $fname)." + "Uncancelable atomic section canceled (got $fname)" ); } } finally { @@ -3945,24 +3947,24 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT ) { static $modes = [ self::TRANSACTION_EXPLICIT, self::TRANSACTION_INTERNAL ]; if ( !in_array( $mode, $modes, true ) ) { - throw new DBUnexpectedError( $this, "$fname: invalid mode parameter '$mode'." ); + throw new DBUnexpectedError( $this, "$fname: invalid mode parameter '$mode'" ); } // Protect against mismatched atomic section, transaction nesting, and snapshot loss if ( $this->trxLevel() ) { if ( $this->trxAtomicLevels ) { $levels = $this->flatAtomicSectionList(); - $msg = "$fname: Got explicit BEGIN while atomic section(s) $levels are open."; + $msg = "$fname: Got explicit BEGIN while atomic section(s) $levels are open"; throw new DBUnexpectedError( $this, $msg ); } elseif ( !$this->trxAutomatic ) { - $msg = "$fname: Explicit transaction already active (from {$this->trxFname})."; + $msg = "$fname: Explicit transaction already active (from {$this->trxFname})"; throw new DBUnexpectedError( $this, $msg ); } else { - $msg = "$fname: Implicit transaction already active (from {$this->trxFname})."; + $msg = "$fname: Implicit transaction already active (from {$this->trxFname})"; throw new DBUnexpectedError( $this, $msg ); } } elseif ( $this->getFlag( self::DBO_TRX ) && $mode !== self::TRANSACTION_INTERNAL ) { - $msg = "$fname: Implicit transaction expected (DBO_TRX set)."; + $msg = "$fname: Implicit transaction expected (DBO_TRX set)"; throw new DBUnexpectedError( $this, $msg ); } @@ -4008,7 +4010,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function commit( $fname = __METHOD__, $flush = self::FLUSHING_ONE ) { static $modes = [ self::FLUSHING_ONE, self::FLUSHING_ALL_PEERS, self::FLUSHING_INTERNAL ]; if ( !in_array( $flush, $modes, true ) ) { - throw new DBUnexpectedError( $this, "$fname: invalid flush parameter '$flush'." ); + throw new DBUnexpectedError( $this, "$fname: invalid flush parameter '$flush'" ); } if ( $this->trxLevel() && $this->trxAtomicLevels ) { @@ -4016,7 +4018,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $levels = $this->flatAtomicSectionList(); throw new DBUnexpectedError( $this, - "$fname: Got COMMIT while atomic sections $levels are still open." + "$fname: Got COMMIT while atomic sections $levels are still open" ); } @@ -4026,17 +4028,17 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } elseif ( !$this->trxAutomatic ) { throw new DBUnexpectedError( $this, - "$fname: Flushing an explicit transaction, getting out of sync." + "$fname: Flushing an explicit transaction, getting out of sync" ); } } elseif ( !$this->trxLevel() ) { $this->queryLogger->error( - "$fname: No transaction to commit, something got out of sync." ); + "$fname: No transaction to commit, something got out of sync" ); return; // nothing to do } elseif ( $this->trxAutomatic ) { throw new DBUnexpectedError( $this, - "$fname: Expected mass commit of all peer transactions (DBO_TRX set)." + "$fname: Expected mass commit of all peer transactions (DBO_TRX set)" ); } @@ -4089,7 +4091,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware ) { throw new DBUnexpectedError( $this, - "$fname: Expected mass rollback of all peer transactions (DBO_TRX set)." + "$fname: Expected mass rollback of all peer transactions (DBO_TRX set)" ); } @@ -4151,13 +4153,32 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } } - public function flushSnapshot( $fname = __METHOD__ ) { - if ( $this->writesOrCallbacksPending() || $this->explicitTrxActive() ) { + public function flushSnapshot( $fname = __METHOD__, $flush = self::FLUSHING_ONE ) { + if ( $this->explicitTrxActive() ) { + // Committing this transaction would break callers that assume it is still open + throw new DBUnexpectedError( + $this, + "$fname: Cannot flush snapshot; " . + "explicit transaction '{$this->trxFname}' is still open" + ); + } elseif ( $this->writesOrCallbacksPending() ) { // This only flushes transactions to clear snapshots, not to write data $fnames = implode( ', ', $this->pendingWriteAndCallbackCallers() ); throw new DBUnexpectedError( $this, - "$fname: Cannot flush snapshot because writes are pending ($fnames)." + "$fname: Cannot flush snapshot; " . + "writes from transaction {$this->trxFname} are still pending ($fnames)" + ); + } elseif ( + $this->trxLevel() && + $this->getTransactionRoundId() && + $flush !== self::FLUSHING_INTERNAL && + $flush !== self::FLUSHING_ALL_PEERS + ) { + $this->queryLogger->warning( + "$fname: Expected mass snapshot flush of all peer transactions " . + "in the explicit transactions round '{$this->getTransactionRoundId()}'", + [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] ); } @@ -4413,7 +4434,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware Wikimedia\restoreWarnings(); if ( $fp === false ) { - throw new RuntimeException( "Could not open \"{$filename}\".\n" ); + throw new RuntimeException( "Could not open \"{$filename}\"" ); } if ( !$fname ) { @@ -4630,7 +4651,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $fnames = implode( ', ', $this->pendingWriteAndCallbackCallers() ); throw new DBUnexpectedError( $this, - "$fname: Cannot flush pre-lock snapshot because writes are pending ($fnames)." + "$fname: Cannot flush pre-lock snapshot; " . + "writes from transaction {$this->trxFname} are still pending ($fnames)" ); } @@ -4669,7 +4691,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function lockTables( array $read, array $write, $method ) { if ( $this->writesOrCallbacksPending() ) { - throw new DBUnexpectedError( $this, "Transaction writes or callbacks still pending." ); + throw new DBUnexpectedError( $this, "Transaction writes or callbacks still pending" ); } if ( $this->tableLocksHaveTransactionScope() ) { @@ -4794,7 +4816,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( !$this->conn ) { throw new DBUnexpectedError( $this, - 'DB connection was already closed or the connection dropped.' + 'DB connection was already closed or the connection dropped' ); } @@ -4827,8 +4849,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ public function __clone() { $this->connLogger->warning( - "Cloning " . static::class . " is not recommended; forking connection:\n" . - ( new RuntimeException() )->getTraceAsString() + "Cloning " . static::class . " is not recommended; forking connection", + [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] ); if ( $this->isOpen() ) { @@ -4856,7 +4878,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ public function __sleep() { throw new RuntimeException( 'Database serialization may cause problems, since ' . - 'the connection is not restored on wakeup.' ); + 'the connection is not restored on wakeup' ); } /** @@ -4864,13 +4886,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ public function __destruct() { if ( $this->trxLevel() && $this->trxDoneWrites ) { - trigger_error( "Uncommitted DB writes (transaction from {$this->trxFname})." ); + trigger_error( "Uncommitted DB writes (transaction from {$this->trxFname})" ); } $danglingWriters = $this->pendingWriteAndCallbackCallers(); if ( $danglingWriters ) { $fnames = implode( ', ', $danglingWriters ); - trigger_error( "DB transaction writes or callbacks still pending ($fnames)." ); + trigger_error( "DB transaction writes or callbacks still pending ($fnames)" ); } if ( $this->conn ) { diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index ad6d4d2f45..3b9d1afa4f 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -1944,7 +1944,7 @@ interface IDatabase { * * @throws DBError */ - public function commit( $fname = __METHOD__, $flush = '' ); + public function commit( $fname = __METHOD__, $flush = self::FLUSHING_ONE ); /** * Rollback a transaction previously started using begin(). @@ -1966,7 +1966,7 @@ interface IDatabase { * @throws DBError * @since 1.23 Added $flush parameter */ - public function rollback( $fname = __METHOD__, $flush = '' ); + public function rollback( $fname = __METHOD__, $flush = self::FLUSHING_ONE ); /** * Commit any transaction but error out if writes or callbacks are pending @@ -1977,10 +1977,20 @@ interface IDatabase { * useful to call on a replica DB after waiting on replication to catch up to the master. * * @param string $fname Calling function name + * @param string $flush Flush flag, set to situationally valid IDatabase::FLUSHING_* + * constant to disable warnings about explicitly committing implicit transactions, + * or calling commit when no transaction is in progress. + * + * This will trigger an exception if there is an ongoing explicit transaction. + * + * Only set the flush flag if you are sure that these warnings are not applicable, + * and no explicit transactions are open. + * * @throws DBError * @since 1.28 + * @since 1.34 Added $flush parameter */ - public function flushSnapshot( $fname = __METHOD__ ); + public function flushSnapshot( $fname = __METHOD__, $flush = self::FLUSHING_ONE ); /** * Convert a timestamp in one of the formats accepted by wfTimestamp() diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index 35c953912c..812064a772 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/includes/libs/rdbms/lbfactory/ILBFactory.php @@ -180,6 +180,8 @@ interface ILBFactory { /** * Commit all replica DB transactions so as to flush any REPEATABLE-READ or SSI snapshot * + * This is useful for getting rid of stale data from an implicit transaction round + * * @param string $fname Caller name */ public function flushReplicaSnapshots( $fname = __METHOD__ ); diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index e20f6de638..d94d24d88e 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -234,6 +234,12 @@ abstract class LBFactory implements ILBFactory { } public function flushReplicaSnapshots( $fname = __METHOD__ ) { + if ( $this->trxRoundId !== false && $this->trxRoundId !== $fname ) { + $this->queryLogger->warning( + "$fname: transaction round '{$this->trxRoundId}' still running", + [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] + ); + } $this->forEachLBCallMethod( 'flushReplicaSnapshots', [ $fname ] ); } @@ -249,7 +255,7 @@ abstract class LBFactory implements ILBFactory { if ( $this->trxRoundId !== false ) { throw new DBTransactionError( null, - "$fname: transaction round '{$this->trxRoundId}' already started." + "$fname: transaction round '{$this->trxRoundId}' already started" ); } $this->trxRoundId = $fname; @@ -264,7 +270,7 @@ abstract class LBFactory implements ILBFactory { if ( $this->trxRoundId !== false && $this->trxRoundId !== $fname ) { throw new DBTransactionError( null, - "$fname: transaction round '{$this->trxRoundId}' still running." + "$fname: transaction round '{$this->trxRoundId}' still running" ); } /** @noinspection PhpUnusedLocalVariableInspection */ @@ -456,8 +462,10 @@ abstract class LBFactory implements ILBFactory { public function getEmptyTransactionTicket( $fname ) { if ( $this->hasMasterChanges() ) { - $this->queryLogger->error( __METHOD__ . ": $fname does not have outer scope.\n" . - ( new RuntimeException() )->getTraceAsString() ); + $this->queryLogger->error( + __METHOD__ . ": $fname does not have outer scope", + [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] + ); return null; } @@ -467,8 +475,10 @@ abstract class LBFactory implements ILBFactory { final public function commitAndWaitForReplication( $fname, $ticket, array $opts = [] ) { if ( $ticket !== $this->ticket ) { - $this->perfLogger->error( __METHOD__ . ": $fname does not have outer scope.\n" . - ( new RuntimeException() )->getTraceAsString() ); + $this->perfLogger->error( + __METHOD__ . ": $fname does not have outer scope", + [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] + ); return false; } @@ -476,7 +486,7 @@ abstract class LBFactory implements ILBFactory { // The transaction owner and any caller with the empty transaction ticket can commit // so that getEmptyTransactionTicket() callers don't risk seeing DBTransactionError. if ( $this->trxRoundId !== false && $fname !== $this->trxRoundId ) { - $this->queryLogger->info( "$fname: committing on behalf of {$this->trxRoundId}." ); + $this->queryLogger->info( "$fname: committing on behalf of {$this->trxRoundId}" ); $fnameEffective = $this->trxRoundId; } else { $fnameEffective = $fname; @@ -530,11 +540,13 @@ abstract class LBFactory implements ILBFactory { } elseif ( $this->memStash instanceof EmptyBagOStuff ) { // No where to store any DB positions and wait for them to appear $this->chronProt->setEnabled( false ); - $this->replLogger->info( 'Cannot use ChronologyProtector with EmptyBagOStuff.' ); + $this->replLogger->info( 'Cannot use ChronologyProtector with EmptyBagOStuff' ); } - $this->replLogger->debug( __METHOD__ . ': using request info ' . - json_encode( $this->requestInfo, JSON_PRETTY_PRINT ) ); + $this->replLogger->debug( + __METHOD__ . ': request info ' . + json_encode( $this->requestInfo, JSON_PRETTY_PRINT ) + ); return $this->chronProt; } @@ -726,7 +738,7 @@ abstract class LBFactory implements ILBFactory { public function setRequestInfo( array $info ) { if ( $this->chronProt ) { - throw new LogicException( 'ChronologyProtector already initialized.' ); + throw new LogicException( 'ChronologyProtector already initialized' ); } $this->requestInfo = $info + $this->requestInfo; diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 2f9857fe9d..8b4ace64d7 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -673,7 +673,9 @@ interface ILoadBalancer { /** * Wait for a replica DB to reach a specified master position * - * This will connect to the master to get an accurate position if $pos is not given + * If $conn is not a replica server connection, then this will return true. + * Otherwise, if $pos is not provided, this will connect to the master server + * to get an accurate position. * * @param IDatabase $conn Replica DB * @param DBMasterPos|bool $pos Master position; default: current position diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index b640dc024b..c1d7a0f4a4 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -127,9 +127,9 @@ class LoadBalancer implements ILoadBalancer { /** @var bool Whether any connection has been attempted yet */ private $connectionAttempted = false; - /** @var int|null An integer ID of the managing LBFactory instance or null */ + /** @var int|null Integer ID of the managing LBFactory instance or null if none */ private $ownerId; - /** @var string|bool String if a requested DBO_TRX transaction round is active */ + /** @var string|bool Explicit DBO_TRX transaction round active or false if none */ private $trxRoundId = false; /** @var string Stage of the current transaction round in the transaction round life-cycle */ private $trxRoundStage = self::ROUND_CURSORY; diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index e5ca3df73e..58f6c05b10 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -1702,7 +1702,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Test exception not thrown' ); } catch ( DBTransactionError $ex ) { $this->assertSame( - 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.', + 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR', $ex->getMessage() ); } @@ -1819,7 +1819,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } catch ( DBUnexpectedError $e ) { $m = __METHOD__; $this->assertSame( - "Invalid atomic section ended (got {$m}_X but expected {$m}).", + "Invalid atomic section ended (got {$m}_X but expected {$m})", $e->getMessage() ); } @@ -1934,7 +1934,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Expected exception not thrown' ); } catch ( DBUnexpectedError $ex ) { $this->assertSame( - 'No atomic section is open (got ' . __METHOD__ . ').', + 'No atomic section is open (got ' . __METHOD__ . ')', $ex->getMessage() ); } @@ -1953,7 +1953,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } catch ( DBUnexpectedError $ex ) { $this->assertSame( 'Invalid atomic section ended (got ' . __METHOD__ . ' but expected ' . - __METHOD__ . 'X).', + __METHOD__ . 'X)', $ex->getMessage() ); } @@ -1970,7 +1970,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Expected exception not thrown' ); } catch ( DBTransactionError $ex ) { $this->assertSame( - 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.', + 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR', $ex->getMessage() ); } @@ -1986,7 +1986,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Expected exception not thrown' ); } catch ( DBUnexpectedError $ex ) { $this->assertSame( - 'No atomic section is open (got ' . __METHOD__ . ').', + 'No atomic section is open (got ' . __METHOD__ . ')', $ex->getMessage() ); } @@ -2074,7 +2074,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Expected exception not thrown' ); } catch ( DBTransactionError $e ) { $this->assertEquals( - 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.', + 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR', $e->getMessage() ); } @@ -2083,7 +2083,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Expected exception not thrown' ); } catch ( DBTransactionError $e ) { $this->assertEquals( - 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.', + 'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR', $e->getMessage() ); } @@ -2187,7 +2187,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->fail( 'Expected exception not thrown' ); } catch ( DBUnexpectedError $ex ) { $this->assertSame( - "Wikimedia\Rdbms\Database::close: transaction is still open (from $fname).", + "Wikimedia\Rdbms\Database::close: transaction is still open (from $fname)", $ex->getMessage() ); } @@ -2216,7 +2216,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } catch ( DBUnexpectedError $ex ) { $this->assertSame( 'Wikimedia\Rdbms\Database::close: atomic sections ' . - 'DatabaseSQLTest::testPrematureClose2 are still open.', + 'DatabaseSQLTest::testPrematureClose2 are still open', $ex->getMessage() ); } @@ -2239,7 +2239,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } catch ( DBUnexpectedError $ex ) { $this->assertSame( 'Wikimedia\Rdbms\Database::close: ' . - 'mass commit/rollback of peer transaction required (DBO_TRX set).', + 'mass commit/rollback of peer transaction required (DBO_TRX set)', $ex->getMessage() ); } -- 2.20.1