Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

rdbms: make Database::query() more readable and consistent

Browse source 
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
This commit is contained in:
Aaron Schulz 2019-03-13 18:56:26 -07:00
parent fa0fe8d294
commit 0390811263
2 changed files with 104 additions and 68 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

166
includes/libs/rdbms/database/Database.php
View file

@ -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 * @throws DBUnexpectedError
*/ */
protected function assertOpen() { protected function assertHasConnectionHandle() {
if ( !$this->isOpen() ) { if ( !$this->isOpen() ) {
throw new DBUnexpectedError( $this, "DB connection was already closed." ); 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 * Closes underlying database connection
* @since 1.20 * @since 1.20
@ -1144,92 +1165,65 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) {
$this->assertTransactionStatus( $sql, $fname ); $this->assertTransactionStatus( $sql, $fname );
$this->assertHasConnectionHandle();
# Avoid fatals if close() was called $priorTransaction = $this->trxLevel;
$this->assertOpen();
$priorWritesPending = $this->writesOrCallbacksPending(); $priorWritesPending = $this->writesOrCallbacksPending();
$this->lastQuery = $sql; $this->lastQuery = $sql;
$isWrite = $this->isWriteQuery( $sql ); if ( $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.'
);
}
# In theory, non-persistent writes are allowed in read-only mode, but due to things # 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... # like https://bugs.mysql.com/bug.php?id=33669 that might not work anyway...
$reason = $this->getReadOnlyReason(); $this->assertIsWritableMaster();
if ( $reason !== false ) { # Avoid treating temporary table operations as meaningful "writes"
throw new DBReadOnlyError( $this, "Database is read-only: $reason" ); $isEffectiveWrite = !$this->registerTempTableOperation( $sql );
} } else {
# Set a flag indicating that writes have been done $isEffectiveWrite = false;
$this->lastWriteTime = microtime( true );
} }
# Add trace comment to the begin of the sql string, right after the operator. # 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) # Or, for one-word queries (like "BEGIN" or COMMIT") add it to the end (T44598)
$commentedSql = preg_replace( '/\s|$/', " /* $fname {$this->agent} */ ", $sql, 1 ); $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 # 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(); $lastError = $this->lastError();
$lastErrno = $this->lastErrno(); $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() ) { if ( $ret === false && $this->wasConnectionLoss() ) {
# Check if any meaningful session state was lost # Check if no meaningful session state was lost
$recoverable = $this->canRecoverFromDisconnect( $sql, $priorWritesPending ); $recoverableCL = $this->canRecoverFromDisconnect( $sql, $priorWritesPending );
# Update session state tracking and try to restore the connection # Update session state tracking and try to restore the connection
$reconnected = $this->replaceLostConnection( __METHOD__ ); $reconnected = $this->replaceLostConnection( __METHOD__ );
# Silently resend the query to the server if it is safe and possible # Silently resend the query to the server if it is safe and possible
if ( $reconnected && $recoverable ) { if ( $recoverableCL && $reconnected ) {
$ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname ); $ret = $this->attemptQuery( $sql, $commentedSql, $isEffectiveWrite, $fname );
$lastError = $this->lastError(); $lastError = $this->lastError();
$lastErrno = $this->lastErrno(); $lastErrno = $this->lastErrno();
if ( $ret === false && $this->wasConnectionLoss() ) { if ( $ret === false && $this->wasConnectionLoss() ) {
# Query probably causes disconnects; reconnect and do not re-run it # Query probably causes disconnects; reconnect and do not re-run it
$this->replaceLostConnection( __METHOD__ ); $this->replaceLostConnection( __METHOD__ );
} else {
$recoverableCL = false; // connection does not need recovering
$recoverableSR = $this->wasKnownStatementRollbackError();
} }
} }
} else {
$recoverableSR = $this->wasKnownStatementRollbackError();
} }
if ( $ret === false ) { if ( $ret === false ) {
if ( $this->trxLevel ) { if ( $priorTransaction ) {
if ( $this->wasKnownStatementRollbackError() ) { if ( $recoverableSR ) {
# We're ignoring an error that caused just the current query to be aborted. # 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 # But log the cause so we can log a deprecation notice if a caller actually
# does ignore it. # does ignore it.
$this->trxStatusIgnoredCause = [ $lastError, $lastErrno, $fname ]; $this->trxStatusIgnoredCause = [ $lastError, $lastErrno, $fname ];
} else { } elseif ( !$recoverableCL ) {
# Either the query was aborted or all queries after BEGIN where aborted. # 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 # 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 # (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 $sql Original SQL query
* @param string $commentedSql SQL query with debugging/trace comment * @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 * @param string $fname Name of the calling function
* @return bool|ResultWrapper True for a successful write query, ResultWrapper * @return bool|ResultWrapper True for a successful write query, ResultWrapper
* object for a successful read query, or false on failure * 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' ) ); $isMaster = !is_null( $this->getLBInfo( 'master' ) );
# generalizeSQL() will probably cut down the query to reasonable # generalizeSQL() will probably cut down the query to reasonable
# logging size most of the time. The substr is really just a sanity check. # 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 ) { if ( $ret !== false ) {
$this->lastPing = $startTime; $this->lastPing = $startTime;
if ( $isWrite && $this->trxLevel ) { if ( $isEffectiveWrite && $this->trxLevel ) {
$this->updateTrxWriteQueryTime( $sql, $queryRuntime, $this->affectedRows() ); $this->updateTrxWriteQueryTime( $sql, $queryRuntime, $this->affectedRows() );
$this->trxWriteCallers[] = $fname; $this->trxWriteCallers[] = $fname;
} }
@ -1300,8 +1310,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
$this->trxProfiler->recordQueryCompletion( $this->trxProfiler->recordQueryCompletion(
$queryProf, $queryProf,
$startTime, $startTime,
$isWrite, $isEffectiveWrite,
$isWrite ? $this->affectedRows() : $this->numRows( $ret ) $isEffectiveWrite ? $this->affectedRows() : $this->numRows( $ret )
); );
$this->queryLogger->debug( $sql, [ $this->queryLogger->debug( $sql, [
'method' => $fname, 'method' => $fname,
@ -1312,6 +1322,23 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
return $ret; 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 * 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 * Determine whether it is safe to retry queries after a database connection is lost
* connection is lost
* *
* @param string $sql SQL query * @param string $sql SQL query
* @param bool $priorWritesPending Whether there is a transaction open with * @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 * Clean things up after transaction loss
*/ */
private function handleTransactionLoss() { 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->trxLevel = 0;
$this->trxAtomicCounter = 0; $this->trxAtomicCounter = 0;
$this->trxIdleCallbacks = []; // T67263; transaction already lost $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 * @note This is for backwards compatibility for callers catching DBError exceptions in
* order to ignore problems like duplicate key errors or foriegn key violations * order to ignore problems like duplicate key errors or foriegn key violations
* @since 1.31 * @since 1.31
@ -3850,8 +3885,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
throw new DBUnexpectedError( $this, $msg ); throw new DBUnexpectedError( $this, $msg );
} }
// Avoid fatals if close() was called $this->assertHasConnectionHandle();
$this->assertOpen();
$this->doBegin( $fname ); $this->doBegin( $fname );
$this->trxStatus = self::STATUS_TRX_OK; $this->trxStatus = self::STATUS_TRX_OK;
@ -3927,8 +3961,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
} }
} }
// Avoid fatals if close() was called $this->assertHasConnectionHandle();
$this->assertOpen();
$this->runOnTransactionPreCommitCallbacks(); $this->runOnTransactionPreCommitCallbacks();
@ -3980,8 +4013,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
} }
if ( $trxActive ) { if ( $trxActive ) {
// Avoid fatals if close() was called $this->assertHasConnectionHandle();
$this->assertOpen();
$this->doRollback( $fname ); $this->doRollback( $fname );
$this->trxStatus = self::STATUS_TRX_NONE; $this->trxStatus = self::STATUS_TRX_NONE;

6
tests/phpunit/includes/db/DatabaseTestHelper.php
View file

@ -108,7 +108,11 @@ class DatabaseTestHelper extends Database {
// Handle some internal calls from the Database class // Handle some internal calls from the Database class
$check = $fname; $check = $fname;
if ( preg_match( '/^Wikimedia\\\\Rdbms\\\\Database::query \((.+)\)$/', $fname, $m ) ) { if ( preg_match(
'/^Wikimedia\\\\Rdbms\\\\Database::(?:query|beginIfImplied) \((.+)\)$/',
$fname,
$m
) ) {
$check = $m[1]; $check = $m[1];
} }