From c1aa9450ad49fd5f17d73c41077e96294c1b9eac Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 27 Feb 2018 16:00:05 -0800 Subject: [PATCH] rdbms: make LoadBalancer::getConnection() ignore CONN_TRX_AUTO when unusable Change-Id: I1fd13171c3cfbe071e8e398d561281188d998767 --- includes/libs/rdbms/database/Database.php | 26 ++++++++ .../libs/rdbms/database/DatabaseSqlite.php | 4 ++ .../libs/rdbms/loadbalancer/ILoadBalancer.php | 27 +++++++-- .../libs/rdbms/loadbalancer/LoadBalancer.php | 23 +++++++ .../includes/db/DatabaseSqliteTest.php | 8 +++ .../phpunit/includes/db/LoadBalancerTest.php | 60 +++++++++++++++++-- 6 files changed, 138 insertions(+), 10 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 9e9b7ab4af..c699374c7f 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -59,6 +59,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware const SLOW_WRITE_SEC = 0.500; const SMALL_WRITE_ROWS = 100; + /** @var string Whether lock granularity is on the level of the entire database */ + const ATTR_DB_LEVEL_LOCKING = 'db-level-locking'; + /** @var string SQL query */ protected $lastQuery = ''; /** @var float|bool UNIX timestamp of last write query */ @@ -385,6 +388,21 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return $conn; } + /** + * @param string $dbType A possible DB type (sqlite, mysql, postgres,...) + * @param string|null $driver Optional name of a specific DB client driver + * @return array Map of (Database::ATTRIBUTE_* constant => value) for all such constants + * @throws InvalidArgumentException + * @since 1.31 + */ + final public static function attributesFromType( $dbType, $driver = null ) { + static $defaults = [ self::ATTR_DB_LEVEL_LOCKING => false ]; + + $class = self::getClass( $dbType, $driver ); + + return call_user_func( [ $class, 'getAttributes' ] ) + $defaults; + } + /** * @param string $dbType A possible DB type (sqlite, mysql, postgres,...) * @param string|null $driver Optional name of a specific DB client driver @@ -441,6 +459,14 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return $class; } + /** + * @return array Map of (Database::ATTRIBUTE_* constant => value + * @since 1.31 + */ + protected static function getAttributes() { + return []; + } + /** * Set the PSR-3 logger interface to use for query logging. (The logger * interfaces for connection logging and error logging can be set with the diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 58b6ef934c..3d6cee350f 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -103,6 +103,10 @@ class DatabaseSqlite extends Database { ] ); } + protected static function getAttributes() { + return [ self::ATTR_DB_LEVEL_LOCKING => true ]; + } + /** * @param string $filename * @param array $p Options map; supports: diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 7bbb530730..8210507a14 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -172,7 +172,9 @@ interface ILoadBalancer { /** * Get a connection handle by server index * - * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first) + * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() + * can be used to check such flags beforehand. * * If the caller uses $domain or sets CONN_TRX_AUTO in $flags, then it must also * call ILoadBalancer::reuseConnection() on the handle when finished using it. @@ -206,7 +208,9 @@ interface ILoadBalancer { * * The handle's methods simply wrap those of a Database handle * - * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first) + * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() + * can be used to check such flags beforehand. * * @see ILoadBalancer::getConnection() for parameter information * @@ -223,7 +227,9 @@ interface ILoadBalancer { * * The handle's methods simply wrap those of a Database handle * - * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first) + * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() + * can be used to check such flags beforehand. * * @see ILoadBalancer::getConnection() for parameter information * @@ -240,7 +246,9 @@ interface ILoadBalancer { * * The handle's methods simply wrap those of a Database handle * - * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first) + * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() + * can be used to check such flags beforehand. * * @see ILoadBalancer::getConnection() for parameter information * @@ -258,7 +266,9 @@ interface ILoadBalancer { * The index must be an actual index into the array. If a connection to the server is * already open and not considered an "in use" foreign connection, this simply returns it. * - * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first) + * Avoid using CONN_TRX_AUTO for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) in + * order to avoid deadlocks. ILoadBalancer::getServerAttributes() can be used to check + * such flags beforehand. * * If the caller uses $domain or sets CONN_TRX_AUTO in $flags, then it must also * call ILoadBalancer::reuseConnection() on the handle when finished using it. @@ -319,6 +329,13 @@ interface ILoadBalancer { */ public function getServerType( $i ); + /** + * @param int $i Server index + * @return array (Database::ATTRIBUTE_* constant => value) for all such constants + * @since 1.31 + */ + public function getServerAttributes( $i ); + /** * Get the current master position for chronology control purposes * @return DBMasterPos|bool Returns false if not applicable diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 569ea0e06c..99a24c2b74 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -680,6 +680,22 @@ class LoadBalancer implements ILoadBalancer { $domain = false; // local connection requested } + if ( ( $flags & self::CONN_TRX_AUTO ) === self::CONN_TRX_AUTO ) { + // Assuming all servers are of the same type (or similar), which is overwhelmingly + // the case, use the master server information to get the attributes. The information + // for $i cannot be used since it might be DB_REPLICA, which might require connection + // attempts in order to be resolved into a real server index. + $attributes = $this->getServerAttributes( $this->getWriterIndex() ); + if ( $attributes[Database::ATTR_DB_LEVEL_LOCKING] ) { + // Callers sometimes want to (a) escape REPEATABLE-READ stateness without locking + // rows (e.g. FOR UPDATE) or (b) make small commits during a larger transactions + // to reduce lock contention. None of these apply for sqlite and using separate + // connections just causes self-deadlocks. + $flags &= ~self::CONN_TRX_AUTO; + $this->connLogger->info( __METHOD__ . ': ignoring CONN_TRX_AUTO to avoid deadlocks.' ); + } + } + $groups = ( $groups === false || $groups === [] ) ? [ false ] // check one "group": the generic pool : (array)$groups; @@ -981,6 +997,13 @@ class LoadBalancer implements ILoadBalancer { return $conn; } + public function getServerAttributes( $i ) { + return Database::attributesFromType( + $this->getServerType( $i ), + isset( $this->servers[$i]['driver'] ) ? $this->servers[$i]['driver'] : null + ); + } + /** * Test if the specified index represents an open connection * diff --git a/tests/phpunit/includes/db/DatabaseSqliteTest.php b/tests/phpunit/includes/db/DatabaseSqliteTest.php index 2de35a7c88..729b58c7ad 100644 --- a/tests/phpunit/includes/db/DatabaseSqliteTest.php +++ b/tests/phpunit/includes/db/DatabaseSqliteTest.php @@ -508,4 +508,12 @@ class DatabaseSqliteTest extends MediaWikiTestCase { $this->assertContains( 'SQLite ', $toString ); } + + /** + * @covers \Wikimedia\Rdbms\DatabaseSqlite::getAttributes() + */ + public function testsAttributes() { + $attributes = Database::attributesFromType( 'sqlite' ); + $this->assertTrue( $attributes[Database::ATTR_DB_LEVEL_LOCKING] ); + } } diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index fe7b710a1f..6cc41d1f41 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -1,10 +1,5 @@ 'my_unittest_wiki', + 'tablePrefix' => 'unittest_', + 'type' => 'sqlite', + 'dbDirectory' => "some_directory", + 'load' => 0 + ] + ]; + + $lb = new LoadBalancer( [ + 'servers' => $servers, + 'localDomain' => new DatabaseDomain( 'my_unittest_wiki', null, 'unittest_' ), + 'loadMonitorClass' => LoadMonitorNull::class + ] ); + + $this->assertTrue( $lb->getServerAttributes( 0 )[Database::ATTR_DB_LEVEL_LOCKING] ); + + $servers = [ + [ // master + 'host' => 'db1001', + 'user' => 'wikiuser', + 'password' => 'none', + 'dbname' => 'my_unittest_wiki', + 'tablePrefix' => 'unittest_', + 'type' => 'mysql', + 'load' => 100 + ], + [ // emulated replica + 'host' => 'db1002', + 'user' => 'wikiuser', + 'password' => 'none', + 'dbname' => 'my_unittest_wiki', + 'tablePrefix' => 'unittest_', + 'type' => 'mysql', + 'load' => 100 + ] + ]; + + $lb = new LoadBalancer( [ + 'servers' => $servers, + 'localDomain' => new DatabaseDomain( 'my_unittest_wiki', null, 'unittest_' ), + 'loadMonitorClass' => LoadMonitorNull::class + ] ); + + $this->assertFalse( $lb->getServerAttributes( 1 )[Database::ATTR_DB_LEVEL_LOCKING] ); + } } -- 2.20.1