From: Aaron Schulz Date: Thu, 14 Mar 2019 01:56:26 +0000 (-0700) Subject: rdbms: make Database::query() more readable and consistent X-Git-Tag: 1.34.0-rc.0~2513^2 X-Git-Url: http://git.cyclocoop.org/%22.htmlspecialchars%28%24url_syndic%29.%22?a=commitdiff_plain;h=03908112635f9da82e4e1141cb4ed4838c56a84d;p=lhc%2Fweb%2Fwiklou.git rdbms: make Database::query() more readable and consistent Mainly: * Stash trxLevel as the variable $priorTransaction since Database::replaceLostConnection might make it 0 when called. * Factor out Database::beginIfImplied method and call it on each query attempt of query(), not just the first one. * Do not bother setting STATUS_TRX_ERROR if a query fails due to connection issues and was recoverable since requiring ROLLBACK in order to continue has no real advantage. * Do not bother setting trxDoneWrites/lastWriteTime for temporary table operations. * Make Database::handleTransactionLoss() keep TransactionProfiler cleaner by calling Database::transactionWritingOut(). Also: * Make sure Database::wasKnownStatementRollbackError() calls are right after the corresponding queries so it is easy to follow. Having connection attempts in between seems fragile. * Rename Database::doProfiledQuery => Database::attemptQuery and move more logic to that method. * Factor out Database::assertNeitherReplicaNorReadOnly method. * Rename Database::assertOpen => Database::assertHasConnectionHandle. * Fix wording of Database::wasKnownStatementRollbackError comments. * Use $isEffectiveWrite variable name instead of $isNonTempWrite and $isWrite in some places. Bug: T218226 Change-Id: I2063e4080b41d5fc504f9207a56312ce92130ed7 --- diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 4d2da176ef..19195b4173 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -991,16 +991,37 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * Make sure isOpen() returns true as a sanity check + * Make sure there is an open connection handle (alive or not) as a sanity check + * + * This guards against fatal errors to the binding handle not being defined + * in cases where open() was never called or close() was already called * * @throws DBUnexpectedError */ - protected function assertOpen() { + protected function assertHasConnectionHandle() { if ( !$this->isOpen() ) { throw new DBUnexpectedError( $this, "DB connection was already closed." ); } } + /** + * Make sure that this server is not marked as a replica nor read-only as a sanity check + * + * @throws DBUnexpectedError + */ + protected function assertIsWritableMaster() { + if ( $this->getLBInfo( 'replica' ) === true ) { + throw new DBUnexpectedError( + $this, + 'Write operations are not allowed on replica database connections.' + ); + } + $reason = $this->getReadOnlyReason(); + if ( $reason !== false ) { + throw new DBReadOnlyError( $this, "Database is read-only: $reason" ); + } + } + /** * Closes underlying database connection * @since 1.20 @@ -1144,92 +1165,65 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { $this->assertTransactionStatus( $sql, $fname ); + $this->assertHasConnectionHandle(); - # Avoid fatals if close() was called - $this->assertOpen(); - + $priorTransaction = $this->trxLevel; $priorWritesPending = $this->writesOrCallbacksPending(); $this->lastQuery = $sql; - $isWrite = $this->isWriteQuery( $sql ); - if ( $isWrite ) { - $isNonTempWrite = !$this->registerTempTableOperation( $sql ); - } else { - $isNonTempWrite = false; - } - - if ( $isWrite ) { - if ( $this->getLBInfo( 'replica' ) === true ) { - throw new DBError( - $this, - 'Write operations are not allowed on replica database connections.' - ); - } + if ( $this->isWriteQuery( $sql ) ) { # In theory, non-persistent writes are allowed in read-only mode, but due to things # like https://bugs.mysql.com/bug.php?id=33669 that might not work anyway... - $reason = $this->getReadOnlyReason(); - if ( $reason !== false ) { - throw new DBReadOnlyError( $this, "Database is read-only: $reason" ); - } - # Set a flag indicating that writes have been done - $this->lastWriteTime = microtime( true ); + $this->assertIsWritableMaster(); + # Avoid treating temporary table operations as meaningful "writes" + $isEffectiveWrite = !$this->registerTempTableOperation( $sql ); + } else { + $isEffectiveWrite = false; } # Add trace comment to the begin of the sql string, right after the operator. # Or, for one-word queries (like "BEGIN" or COMMIT") add it to the end (T44598) $commentedSql = preg_replace( '/\s|$/', " /* $fname {$this->agent} */ ", $sql, 1 ); - # Start implicit transactions that wrap the request if DBO_TRX is enabled - if ( !$this->trxLevel && $this->getFlag( self::DBO_TRX ) - && $this->isTransactableQuery( $sql ) - ) { - $this->begin( __METHOD__ . " ($fname)", self::TRANSACTION_INTERNAL ); - $this->trxAutomatic = true; - } - - # Keep track of whether the transaction has write queries pending - if ( $this->trxLevel && !$this->trxDoneWrites && $isWrite ) { - $this->trxDoneWrites = true; - $this->trxProfiler->transactionWritingIn( - $this->server, $this->getDomainID(), $this->trxShortId ); - } - - if ( $this->getFlag( self::DBO_DEBUG ) ) { - $this->queryLogger->debug( "{$this->getDomainID()} {$commentedSql}" ); - } - # Send the query to the server and fetch any corresponding errors - $ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname ); + $ret = $this->attemptQuery( $sql, $commentedSql, $isEffectiveWrite, $fname ); $lastError = $this->lastError(); $lastErrno = $this->lastErrno(); - # Try reconnecting if the connection was lost + $recoverableSR = false; // recoverable statement rollback? + $recoverableCL = false; // recoverable connection loss? + if ( $ret === false && $this->wasConnectionLoss() ) { - # Check if any meaningful session state was lost - $recoverable = $this->canRecoverFromDisconnect( $sql, $priorWritesPending ); + # Check if no meaningful session state was lost + $recoverableCL = $this->canRecoverFromDisconnect( $sql, $priorWritesPending ); # Update session state tracking and try to restore the connection $reconnected = $this->replaceLostConnection( __METHOD__ ); # Silently resend the query to the server if it is safe and possible - if ( $reconnected && $recoverable ) { - $ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname ); + if ( $recoverableCL && $reconnected ) { + $ret = $this->attemptQuery( $sql, $commentedSql, $isEffectiveWrite, $fname ); $lastError = $this->lastError(); $lastErrno = $this->lastErrno(); if ( $ret === false && $this->wasConnectionLoss() ) { # Query probably causes disconnects; reconnect and do not re-run it $this->replaceLostConnection( __METHOD__ ); + } else { + $recoverableCL = false; // connection does not need recovering + $recoverableSR = $this->wasKnownStatementRollbackError(); } } + } else { + $recoverableSR = $this->wasKnownStatementRollbackError(); } if ( $ret === false ) { - if ( $this->trxLevel ) { - if ( $this->wasKnownStatementRollbackError() ) { + if ( $priorTransaction ) { + if ( $recoverableSR ) { # We're ignoring an error that caused just the current query to be aborted. # But log the cause so we can log a deprecation notice if a caller actually # does ignore it. $this->trxStatusIgnoredCause = [ $lastError, $lastErrno, $fname ]; - } else { + } elseif ( !$recoverableCL ) { # Either the query was aborted or all queries after BEGIN where aborted. # In the first case, the only options going forward are (a) ROLLBACK, or # (b) ROLLBACK TO SAVEPOINT (if one was set). If the later case, the only @@ -1253,12 +1247,28 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * * @param string $sql Original SQL query * @param string $commentedSql SQL query with debugging/trace comment - * @param bool $isWrite Whether the query is a (non-temporary) write operation + * @param bool $isEffectiveWrite Whether the query is a (non-temporary table) write * @param string $fname Name of the calling function * @return bool|ResultWrapper True for a successful write query, ResultWrapper * object for a successful read query, or false on failure */ - private function doProfiledQuery( $sql, $commentedSql, $isWrite, $fname ) { + private function attemptQuery( $sql, $commentedSql, $isEffectiveWrite, $fname ) { + $this->beginIfImplied( $sql, $fname ); + + # Keep track of whether the transaction has write queries pending + if ( $isEffectiveWrite ) { + $this->lastWriteTime = microtime( true ); + if ( $this->trxLevel && !$this->trxDoneWrites ) { + $this->trxDoneWrites = true; + $this->trxProfiler->transactionWritingIn( + $this->server, $this->getDomainID(), $this->trxShortId ); + } + } + + if ( $this->getFlag( self::DBO_DEBUG ) ) { + $this->queryLogger->debug( "{$this->getDomainID()} {$commentedSql}" ); + } + $isMaster = !is_null( $this->getLBInfo( 'master' ) ); # generalizeSQL() will probably cut down the query to reasonable # logging size most of the time. The substr is really just a sanity check. @@ -1287,7 +1297,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $ret !== false ) { $this->lastPing = $startTime; - if ( $isWrite && $this->trxLevel ) { + if ( $isEffectiveWrite && $this->trxLevel ) { $this->updateTrxWriteQueryTime( $sql, $queryRuntime, $this->affectedRows() ); $this->trxWriteCallers[] = $fname; } @@ -1300,8 +1310,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->trxProfiler->recordQueryCompletion( $queryProf, $startTime, - $isWrite, - $isWrite ? $this->affectedRows() : $this->numRows( $ret ) + $isEffectiveWrite, + $isEffectiveWrite ? $this->affectedRows() : $this->numRows( $ret ) ); $this->queryLogger->debug( $sql, [ 'method' => $fname, @@ -1312,6 +1322,23 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return $ret; } + /** + * Start an implicit transaction if DBO_TRX is enabled and no transaction is active + * + * @param string $sql + * @param string $fname + */ + private function beginIfImplied( $sql, $fname ) { + if ( + !$this->trxLevel && + $this->getFlag( self::DBO_TRX ) && + $this->isTransactableQuery( $sql ) + ) { + $this->begin( __METHOD__ . " ($fname)", self::TRANSACTION_INTERNAL ); + $this->trxAutomatic = true; + } + } + /** * Update the estimated run-time of a query, not counting large row lock times * @@ -1391,8 +1418,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * Determine whether or not it is safe to retry queries after a database - * connection is lost + * Determine whether it is safe to retry queries after a database connection is lost * * @param string $sql SQL query * @param bool $priorWritesPending Whether there is a transaction open with @@ -1441,6 +1467,15 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * Clean things up after transaction loss */ private function handleTransactionLoss() { + if ( $this->trxDoneWrites ) { + $this->trxProfiler->transactionWritingOut( + $this->server, + $this->getDomainID(), + $this->trxShortId, + $this->pendingWriteQueryDuration( self::ESTIMATE_TOTAL ), + $this->trxWriteAffectedRows + ); + } $this->trxLevel = 0; $this->trxAtomicCounter = 0; $this->trxIdleCallbacks = []; // T67263; transaction already lost @@ -3289,7 +3324,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * @return bool Whether it is safe to assume the given error only caused statement rollback + * @return bool Whether it is known that the last query error only caused statement rollback * @note This is for backwards compatibility for callers catching DBError exceptions in * order to ignore problems like duplicate key errors or foriegn key violations * @since 1.31 @@ -3850,8 +3885,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware throw new DBUnexpectedError( $this, $msg ); } - // Avoid fatals if close() was called - $this->assertOpen(); + $this->assertHasConnectionHandle(); $this->doBegin( $fname ); $this->trxStatus = self::STATUS_TRX_OK; @@ -3927,8 +3961,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } } - // Avoid fatals if close() was called - $this->assertOpen(); + $this->assertHasConnectionHandle(); $this->runOnTransactionPreCommitCallbacks(); @@ -3980,8 +4013,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } if ( $trxActive ) { - // Avoid fatals if close() was called - $this->assertOpen(); + $this->assertHasConnectionHandle(); $this->doRollback( $fname ); $this->trxStatus = self::STATUS_TRX_NONE; diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php index 65b82abf0f..3d86fc8e53 100644 --- a/tests/phpunit/includes/db/DatabaseTestHelper.php +++ b/tests/phpunit/includes/db/DatabaseTestHelper.php @@ -108,7 +108,11 @@ class DatabaseTestHelper extends Database { // Handle some internal calls from the Database class $check = $fname; - if ( preg_match( '/^Wikimedia\\\\Rdbms\\\\Database::query \((.+)\)$/', $fname, $m ) ) { + if ( preg_match( + '/^Wikimedia\\\\Rdbms\\\\Database::(?:query|beginIfImplied) \((.+)\)$/', + $fname, + $m + ) ) { $check = $m[1]; }