rdbms: make LoadBalancer::getConnection() ignore CONN_TRX_AUTO when unusable
authorAaron Schulz <aschulz@wikimedia.org>
Wed, 28 Feb 2018 00:00:05 +0000 (16:00 -0800)
committerAaron Schulz <aschulz@wikimedia.org>
Fri, 2 Mar 2018 18:40:39 +0000 (18:40 +0000)
Change-Id: I1fd13171c3cfbe071e8e398d561281188d998767

includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/DatabaseSqlite.php
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
tests/phpunit/includes/db/DatabaseSqliteTest.php
tests/phpunit/includes/db/LoadBalancerTest.php

index 9e9b7ab..c699374 100644 (file)
@@ -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
index 58b6ef9..3d6cee3 100644 (file)
@@ -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:
index 7bbb530..8210507 100644 (file)
@@ -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
index 569ea0e..99a24c2 100644 (file)
@@ -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
         *
index 2de35a7..729b58c 100644 (file)
@@ -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] );
+       }
 }
index fe7b710..6cc41d1 100644 (file)
@@ -1,10 +1,5 @@
 <?php
 
-use Wikimedia\Rdbms\DBError;
-use Wikimedia\Rdbms\LoadBalancer;
-use Wikimedia\Rdbms\DatabaseDomain;
-use Wikimedia\Rdbms\Database;
-
 /**
  * Holds tests for LoadBalancer MediaWiki class.
  *
@@ -28,6 +23,13 @@ use Wikimedia\Rdbms\Database;
  *
  * @covers \Wikimedia\Rdbms\LoadBalancer
  */
+
+use Wikimedia\Rdbms\DBError;
+use Wikimedia\Rdbms\DatabaseDomain;
+use Wikimedia\Rdbms\Database;
+use Wikimedia\Rdbms\LoadBalancer;
+use Wikimedia\Rdbms\LoadMonitorNull;
+
 class LoadBalancerTest extends MediaWikiTestCase {
        public function testWithoutReplica() {
                global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir;
@@ -190,4 +192,52 @@ class LoadBalancerTest extends MediaWikiTestCase {
                }
        }
 
+       public function testServerAttributes() {
+               $servers = [
+                       [ // master
+                               'dbname'      => '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] );
+       }
 }