From 4118762bbe609356d042e8746c5abbfe64e4696e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 21 Mar 2019 05:51:28 -0700 Subject: [PATCH] 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 --- includes/libs/rdbms/database/DBConnRef.php | 24 ++++++++++++- .../libs/rdbms/database/DatabasePostgres.php | 4 --- .../libs/rdbms/loadbalancer/LoadBalancer.php | 19 ++++++++--- .../libs/rdbms/database/DBConnRefTest.php | 34 ++++++++++++++++++- 4 files changed, 70 insertions(+), 11 deletions(-) diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index 5d80f836fc..70b05835c1 100644 --- 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 8aec1acaea..481dd9a6be 100644 --- 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 9e4b15ff79..21e46457d0 100644 --- 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 e130b23cff..9b72b95dc3 100644 --- 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(); + } } -- 2.20.1