[MediaWiki-commits] [Gerrit] mediawiki/core[master]: Clean up postgres connection handling
jenkins-bot has submitted this change and it was merged. Change subject: Clean up postgres connection handling .. Clean up postgres connection handling * Remove non-connection magic case when no DB $user is given. This was removed from the base class. * Use PGSQL_CONNECT_FORCE_NEW to let LoadBalancer handle connection reuse. This makes it work like the mysql classes. * Make postgres connection error messages actually be useful by using the PHP error when possible. This makes it clear if the problem is authentication or something else and so on. Change-Id: I3fd76c1e2db8d6008074f5347b201554579b549a --- M includes/libs/rdbms/database/Database.php M includes/libs/rdbms/database/DatabasePostgres.php M includes/libs/rdbms/loadbalancer/LoadBalancer.php 3 files changed, 19 insertions(+), 13 deletions(-) Approvals: Gergő Tisza: Looks good to me, approved jenkins-bot: Verified diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index f33e244..4266912 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -651,14 +651,22 @@ if ( $this->htmlErrors !== false ) { ini_set( 'html_errors', $this->htmlErrors ); } + + return $this->getLastPHPError(); + } + + /** +* @return string|bool Last PHP error for this DB (typically connection errors) +*/ + protected function getLastPHPError() { if ( $this->mPHPError ) { $error = preg_replace( '!\[mPHPError ); $error = preg_replace( '!^.*?:\s?(.*)$!', '$1', $error ); return $error; - } else { - return false; } + + return false; } /** diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index 84021a0..016b9cd 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -92,10 +92,6 @@ ); } - if ( !strlen( $user ) ) { # e.g. the class is being loaded - return null; - } - $this->mServer = $server; $this->mUser = $user; $this->mPassword = $password; @@ -121,7 +117,8 @@ $this->installErrorHandler(); try { - $this->mConn = pg_connect( $this->connectString ); + // Use new connections to let LoadBalancer/LBFactory handle reuse + $this->mConn = pg_connect( $this->connectString, PGSQL_CONNECT_FORCE_NEW ); } catch ( Exception $ex ) { $this->restoreErrorHandler(); throw $ex; @@ -130,10 +127,11 @@ $phpError = $this->restoreErrorHandler(); if ( !$this->mConn ) { - $this->queryLogger->debug( "DB connection error\n" ); $this->queryLogger->debug( + "DB connection error\n" . "Server: $server, Database: $dbName, User: $user, Password: " . - substr( $password, 0, 3 ) . "...\n" ); + substr( $password, 0, 3 ) . "...\n" + ); $this->queryLogger->debug( $this->lastError() . "\n" ); throw new DBConnectionError( $this, str_replace( "\n", ' ', $phpError ) ); } @@ -380,9 +378,9 @@ } else { return pg_last_error(); } - } else { - return 'No database connection'; } + + return $this->getLastPHPError() ?: 'No database connection'; } function lastErrno() { diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 682698d..8a51fe2 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -552,7 +552,7 @@ if ( $i == self::DB_REPLICA ) { $this->mLastError = 'Unknown error'; // reset error string # Try the general server pool if $groups are unavailable. - $i = in_array( false, $groups, true ) + $i = ( $groups === [ false ] ) ? false // don't bother with this if that is what was tried above : $this->getReaderIndex( false, $domain ); # Couldn't find a
[MediaWiki-commits] [Gerrit] mediawiki/core[master]: Clean up postgres connection handling
Aaron Schulz has uploaded a new change for review. https://gerrit.wikimedia.org/r/316509 Change subject: Clean up postgres connection handling .. Clean up postgres connection handling * Remove non-connection magic case when no DB $user is given. This was removed from the base class. * Use PGSQL_CONNECT_FORCE_NEW to let LoadBalancer handle connection reuse. This makes it work like the mysql classes. * Make postgres connection error messages actually be useful by using the PHP error when possible. This makes it clear if the problem is authentication or something else and so on. Change-Id: I3fd76c1e2db8d6008074f5347b201554579b549a --- M includes/libs/rdbms/database/Database.php M includes/libs/rdbms/database/DatabasePostgres.php M includes/libs/rdbms/loadbalancer/LoadBalancer.php 3 files changed, 19 insertions(+), 13 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/09/316509/1 diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index f33e244..4266912 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -651,14 +651,22 @@ if ( $this->htmlErrors !== false ) { ini_set( 'html_errors', $this->htmlErrors ); } + + return $this->getLastPHPError(); + } + + /** +* @return string|bool Last PHP error for this DB (typically connection errors) +*/ + protected function getLastPHPError() { if ( $this->mPHPError ) { $error = preg_replace( '!\[mPHPError ); $error = preg_replace( '!^.*?:\s?(.*)$!', '$1', $error ); return $error; - } else { - return false; } + + return false; } /** diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index 84021a0..016b9cd 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -92,10 +92,6 @@ ); } - if ( !strlen( $user ) ) { # e.g. the class is being loaded - return null; - } - $this->mServer = $server; $this->mUser = $user; $this->mPassword = $password; @@ -121,7 +117,8 @@ $this->installErrorHandler(); try { - $this->mConn = pg_connect( $this->connectString ); + // Use new connections to let LoadBalancer/LBFactory handle reuse + $this->mConn = pg_connect( $this->connectString, PGSQL_CONNECT_FORCE_NEW ); } catch ( Exception $ex ) { $this->restoreErrorHandler(); throw $ex; @@ -130,10 +127,11 @@ $phpError = $this->restoreErrorHandler(); if ( !$this->mConn ) { - $this->queryLogger->debug( "DB connection error\n" ); $this->queryLogger->debug( + "DB connection error\n" . "Server: $server, Database: $dbName, User: $user, Password: " . - substr( $password, 0, 3 ) . "...\n" ); + substr( $password, 0, 3 ) . "...\n" + ); $this->queryLogger->debug( $this->lastError() . "\n" ); throw new DBConnectionError( $this, str_replace( "\n", ' ', $phpError ) ); } @@ -380,9 +378,9 @@ } else { return pg_last_error(); } - } else { - return 'No database connection'; } + + return $this->getLastPHPError() ?: 'No database connection'; } function lastErrno() { diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 682698d..a7df9c8 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -552,7 +552,7 @@ if ( $i == self::DB_REPLICA ) { $this->mLastError = 'Unknown error'; // reset error string # Try the general server pool if $groups are unavailable. - $i = in_array( false, $groups, true ) + $i = ( $groups == [ false ] ) ? false // don't bother with this if that is what was tried above : $this->getReaderIndex( false, $domain );