From fda4d46fc4f81061b15b760f0364e1f1b8669f6b Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 17 Oct 2016 18:21:46 -0700
Subject: [PATCH] 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
---
 includes/libs/rdbms/database/Database.php        | 12 ++++++++++--
 .../libs/rdbms/database/DatabasePostgres.php     | 16 +++++++---------
 .../libs/rdbms/loadbalancer/LoadBalancer.php     |  4 ++--
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index f33e244adb..4266912c9b 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -651,14 +651,22 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		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( '!\[<a.*</a>\]!', '', $this->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 84021a03e0..016b9cda8c 100644
--- a/includes/libs/rdbms/database/DatabasePostgres.php
+++ b/includes/libs/rdbms/database/DatabasePostgres.php
@@ -92,10 +92,6 @@ class DatabasePostgres extends Database {
 			);
 		}
 
-		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 @@ class DatabasePostgres extends Database {
 		$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 @@ class DatabasePostgres extends Database {
 		$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 @@ class DatabasePostgres extends Database {
 			} 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 682698d05d..8a51fe2fa3 100644
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -552,7 +552,7 @@ class LoadBalancer implements ILoadBalancer {
 		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 working server in getReaderIndex()?
@@ -886,7 +886,7 @@ class LoadBalancer implements ILoadBalancer {
 			// If all servers were busy, mLastError will contain something sensible
 			throw new DBConnectionError( null, $this->mLastError );
 		} else {
-			$context['db_server'] = $conn->getProperty( 'mServer' );
+			$context['db_server'] = $conn->getServer();
 			$this->connLogger->warning(
 				"Connection error: {last_error} ({db_server})",
 				$context
-- 
2.20.1