rdbms: avoid connections on more lazy DBConnRef methods

Also:
* Fixed LoadBalancer::getAnyOpenConnection for both
  DB_MASTER and DB_REPLICA, which are not real indexes.
* Lock down DBConnRef::close since it can only cause trouble.
* Relax DBConnRef restrictions on tablePrefix()/dbSchema()
  for the harmless "getter" mode case.
* Remove redundant DatabasePostgres::getServer definition.

Change-Id: Ia855d901cc3c28147e52284fdabb1645805d4466

diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php
index 5d80f83..70b0583 100644 (file)
--- a/includes/libs/rdbms/database/DBConnRef.php
+++ b/includes/libs/rdbms/database/DBConnRef.php
@@ -74,11 +74,27 @@ class DBConnRef implements IDatabase {
        }
 
        public function tablePrefix( $prefix = null ) {
+               if ( $this->conn === null && $prefix === null ) {
+                       $domain = DatabaseDomain::newFromId( $this->params[self::FLD_DOMAIN] );
+                       // Avoid triggering a database connection
+                       return $domain->getTablePrefix();
+               } elseif ( $this->conn !== null && $prefix === null ) {
+                       // This will just return the prefix
+                       return $this->__call( __FUNCTION__, func_get_args() );
+               }
                // Disallow things that might confuse the LoadBalancer tracking
                throw new DBUnexpectedError( $this, "Database selection is disallowed to enable reuse." );
        }
 
        public function dbSchema( $schema = null ) {
+               if ( $this->conn === null && $schema === null ) {
+                       $domain = DatabaseDomain::newFromId( $this->params[self::FLD_DOMAIN] );
+                       // Avoid triggering a database connection
+                       return $domain->getSchema();
+               } elseif ( $this->conn !== null && $schema === null ) {
+                       // This will just return the schema
+                       return $this->__call( __FUNCTION__, func_get_args() );
+               }
                // Disallow things that might confuse the LoadBalancer tracking
                throw new DBUnexpectedError( $this, "Database selection is disallowed to enable reuse." );
        }
@@ -235,7 +251,7 @@ class DBConnRef implements IDatabase {
        }
 
        public function close() {
-               return $this->__call( __FUNCTION__, func_get_args() );
+               throw new DBUnexpectedError( $this->conn, 'Cannot close shared connection.' );
        }
 
        public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) {
@@ -385,6 +401,12 @@ class DBConnRef implements IDatabase {
        }
 
        public function getDBname() {
+               if ( $this->conn === null ) {
+                       $domain = DatabaseDomain::newFromId( $this->params[self::FLD_DOMAIN] );
+                       // Avoid triggering a database connection
+                       return $domain->getDatabase();
+               }
+
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 

diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php
index 8aec1ac..481dd9a 100644 (file)
--- a/includes/libs/rdbms/database/DatabasePostgres.php
+++ b/includes/libs/rdbms/database/DatabasePostgres.php
@@ -1346,10 +1346,6 @@ SQL;
                return [ $startOpts, $useIndex, $preLimitTail, $postLimitTail, $ignoreIndex ];
        }
 
-       public function getServer() {
-               return $this->server;
-       }
-
        public function buildConcat( $stringList ) {
                return implode( ' || ', $stringList );
        }

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index 9e4b15f..21e4645 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -566,15 +566,24 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        public function getAnyOpenConnection( $i, $flags = 0 ) {
+               $i = ( $i === self::DB_MASTER ) ? $this->getWriterIndex() : $i;
                $autocommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT );
+
                foreach ( $this->conns as $connsByServer ) {
-                       if ( !isset( $connsByServer[$i] ) ) {
-                               continue;
+                       if ( $i === self::DB_REPLICA ) {
+                               $indexes = array_keys( $connsByServer );
+                       } else {
+                               $indexes = isset( $connsByServer[$i] ) ? [ $i ] : [];
                        }
 
-                       foreach ( $connsByServer[$i] as $conn ) {
-                               if ( !$autocommit || $conn->getLBInfo( 'autoCommitOnly' ) ) {
-                                       return $conn;
+                       foreach ( $indexes as $index ) {
+                               foreach ( $connsByServer[$index] as $conn ) {
+                                       if ( !$conn->isOpen() ) {
+                                               continue; // some sort of error occured?
+                                       }
+                                       if ( !$autocommit || $conn->getLBInfo( 'autoCommitOnly' ) ) {
+                                               return $conn;
+                                       }
                                }
                        }
                }

diff --git a/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php b/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php
index e130b23..9b72b95 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php
@@ -44,7 +44,27 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase {
                        ->disableOriginalConstructor()
                        ->getMock();
 
-               $db->method( 'select' )->willReturn( new FakeResultWrapper( [] ) );
+               $open = true;
+               $db->method( 'select' )->willReturnCallback( function () use ( &$open ) {
+                       if ( !$open ) {
+                               throw new LogicException( "Not open" );
+                       }
+
+                       return new FakeResultWrapper( [] );
+               } );
+               $db->method( 'close' )->willReturnCallback( function () use ( &$open ) {
+                       $open = false;
+
+                       return true;
+               } );
+               $db->method( 'isOpen' )->willReturnCallback( function () use ( &$open ) {
+                       return $open;
+               } );
+               $db->method( 'open' )->willReturnCallback( function () use ( &$open ) {
+                       $open = true;
+
+                       return $open;
+               } );
                $db->method( '__toString' )->willReturn( 'MOCK_DB' );
 
                return $db;
@@ -122,6 +142,9 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase {
                $this->assertSame( 'dummy', $ref->getDomainID() );
        }
 
+       /**
+        * @covers Wikimedia\Rdbms\DBConnRef::select
+        */
        public function testSelect() {
                // select should get passed through normally
                $ref = $this->getDBConnRef();
@@ -137,4 +160,13 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase {
                $this->assertInternalType( 'string', $ref->__toString() );
        }
 
+       /**
+        * @covers Wikimedia\Rdbms\DBConnRef::close
+        * @expectedException \Wikimedia\Rdbms\DBUnexpectedError
+        */
+       public function testClose() {
+               $lb = $this->getLoadBalancerMock();
+               $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ] );
+               $ref->close();
+       }
 }