From e0a53cba1508fa4da67e53434ef012a3867da432 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 17 Mar 2022 17:55:52 -0700 Subject: [PATCH] rdbms: rename wasKnownStatementRollbackError() to isKnownStatementRollbackError() Pass the error number as an argument, similar to isConnectionError() Fix related mysql documentation links Change-Id: Id32ef2fd27de65376960de3f5138ffdf7654ff71 --- RELEASE-NOTES-1.39 | 2 ++ includes/libs/rdbms/database/Database.php | 7 ++++--- includes/libs/rdbms/database/DatabaseMysqlBase.php | 9 ++++----- includes/libs/rdbms/database/DatabasePostgres.php | 2 +- includes/libs/rdbms/database/DatabaseSqlite.php | 2 +- tests/phpunit/includes/db/DatabaseTestHelper.php | 8 +++++--- .../includes/libs/rdbms/database/DatabaseSQLTest.php | 2 +- 7 files changed, 18 insertions(+), 14 deletions(-) diff --git a/RELEASE-NOTES-1.39 b/RELEASE-NOTES-1.39 index 649a30951bf..b5f5b91eccd 100644 --- a/RELEASE-NOTES-1.39 +++ b/RELEASE-NOTES-1.39 @@ -77,6 +77,8 @@ because of Phabricator reports. - ::fetchRow() - ::numRows() - ::freeResult() +* Database::wasKnownStatementRollbackError() was removed. Subclasses should + override isKnownStatementRollbackError() instead. * … === Deprecations in 1.39 === diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 7b510f1a8bb..876235cff46 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -1330,7 +1330,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $reconnected = $this->replaceLostConnection( $lastErrno, __METHOD__ ); } else { # Check if only the last query was rolled back - $recoverableSR = $this->wasKnownStatementRollbackError(); + $recoverableSR = $this->isKnownStatementRollbackError( $lastErrno ); } if ( $sql === self::PING_QUERY ) { @@ -3904,12 +3904,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** * @stable to override + * @param int|string $errno * @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 foreign key violations - * @since 1.31 + * @since 1.39 */ - protected function wasKnownStatementRollbackError() { + protected function isKnownStatementRollbackError( $errno ) { return false; // don't know; it could have caused a transaction rollback } diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index d053a0774b9..c20ffdb27f3 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -1229,11 +1229,11 @@ abstract class DatabaseMysqlBase extends Database { return $errno == 2013 || $errno == 2006; } - protected function wasKnownStatementRollbackError() { - $errno = $this->lastErrno(); - + protected function isKnownStatementRollbackError( $errno ) { + // https://mariadb.com/kb/en/mariadb-error-codes/ + // https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html if ( $errno === 1205 ) { // lock wait timeout - // Note that this is uncached to avoid stale values of SET is used + // Note that this is uncached to avoid stale values if SET is used $res = $this->query( "SELECT @@innodb_rollback_on_timeout AS Value", __METHOD__, @@ -1245,7 +1245,6 @@ abstract class DatabaseMysqlBase extends Database { return ( $row && !$row->Value ); } - // See https://dev.mysql.com/doc/refman/5.5/en/error-messages-server.html return in_array( $errno, [ 1022, 1062, 1216, 1217, 1137, 1146, 1051, 1054 ], true ); } diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index b814f3134a0..aeb21c04059 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -661,7 +661,7 @@ __INDEXATTR__; return in_array( $errno, $codes, true ); } - protected function wasKnownStatementRollbackError() { + protected function isKnownStatementRollbackError( $errno ) { return false; // transaction has to be rolled-back from error state } diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index b0a8059965b..04c4029bc54 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -620,7 +620,7 @@ class DatabaseSqlite extends Database { return $errno == 17; // SQLITE_SCHEMA; } - protected function wasKnownStatementRollbackError() { + protected function isKnownStatementRollbackError( $errno ) { // ON CONFLICT ROLLBACK clauses make it so that SQLITE_CONSTRAINT error is // ambiguous with regard to whether it implies a ROLLBACK or an ABORT happened. // https://sqlite.org/lang_createtable.html#uniqueconst diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php index b13a9328fd9..0874f2a1946 100644 --- a/tests/phpunit/includes/db/DatabaseTestHelper.php +++ b/tests/phpunit/includes/db/DatabaseTestHelper.php @@ -110,7 +110,7 @@ class DatabaseTestHelper extends Database { * @param int $errno Error number * @param string $error Error text * @param array $options - * - wasKnownStatementRollbackError: Return value for wasKnownStatementRollbackError() + * - isKnownStatementRollbackError: Return value for isKnownStatementRollbackError() */ public function forceNextQueryError( $errno, $error, $options = [] ) { $this->nextError = [ 'errno' => $errno, 'error' => $error ] + $options; @@ -195,8 +195,10 @@ class DatabaseTestHelper extends Database { return $this->lastError ? $this->lastError['error'] : 'test'; } - protected function wasKnownStatementRollbackError() { - return $this->lastError['wasKnownStatementRollbackError'] ?? false; + protected function isKnownStatementRollbackError( $errno ) { + return ( $this->lastError['errno'] ?? 0 ) === $errno + ? ( $this->lastError['isKnownStatementRollbackError'] ?? false ) + : false; } public function fieldInfo( $table, $field ) { diff --git a/tests/phpunit/unit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/unit/includes/libs/rdbms/database/DatabaseSQLTest.php index 38bc9db2d14..188661983fb 100644 --- a/tests/phpunit/unit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/unit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -2375,7 +2375,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $doError = function () { $this->database->forceNextQueryError( 666, 'Evilness', [ - 'wasKnownStatementRollbackError' => true, + 'isKnownStatementRollbackError' => true, ] ); try { $this->database->delete( 'error', '1', __CLASS__ . '::SomeCaller' );