From 66b0ad56df146c74e531c0f89524398ee0ad3e5b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 17 Oct 2016 11:21:40 -0700 Subject: [PATCH] Postgres installation fixes * Make isTransactableQuery() exclude CREATE/ALTER. Starting transactions for schema changes like this can cause errors as it is not supported for MySQL and some Postgres operations. Note that temporary tables are session-level, so they are not effected by this change. * Clean up the transaction logic in determineCoreSchema() so a transaction is not left dangling. * Fix broken getSchemaPath() call in PostgresInstaller. * Avoid warnings in DatabasePostgres::closeConnection() if mConn is already unset. * Commit master changes in doMaintenance.php before running deferred updates, just as MediaWiki.php does. * Change E_WARNING to E_USER_WARNING to avoid notices in the default /rdbms error handlers. * Also avoid trying to rollback in MWExceptionHandler if the LBFactory service is disabled, which just results in an error. Bug: T147599 Change-Id: I64ccab7f9b74f60309ba0c9a8ce68337c42ffb0f --- includes/exception/MWExceptionHandler.php | 7 ++- includes/installer/PostgresInstaller.php | 6 +-- includes/libs/rdbms/database/Database.php | 42 +++++++++++++--- .../libs/rdbms/database/DatabaseMysqlBase.php | 22 --------- .../libs/rdbms/database/DatabaseMysqli.php | 3 +- .../libs/rdbms/database/DatabasePostgres.php | 48 ++++++++++++------- .../libs/rdbms/database/DatabaseSqlite.php | 3 +- includes/libs/rdbms/lbfactory/LBFactory.php | 2 +- .../libs/rdbms/loadbalancer/LoadBalancer.php | 2 +- maintenance/doMaintenance.php | 8 ++-- 10 files changed, 82 insertions(+), 61 deletions(-) diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php index 4a1f190c75..736cb06a21 100644 --- a/includes/exception/MWExceptionHandler.php +++ b/includes/exception/MWExceptionHandler.php @@ -87,7 +87,12 @@ class MWExceptionHandler { * @param Exception|Throwable $e */ public static function rollbackMasterChangesAndLog( $e ) { - $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); + $services = MediaWikiServices::getInstance(); + if ( $services->isServiceDisabled( 'DBLoadBalancerFactory' ) ) { + return; // T147599 + } + + $lbFactory = $services->getDBLoadBalancerFactory(); if ( $lbFactory->hasMasterChanges() ) { $logger = LoggerFactory::getInstance( 'Bug56269' ); $logger->warning( diff --git a/includes/installer/PostgresInstaller.php b/includes/installer/PostgresInstaller.php index 33e1a1f89d..6dfa28b21c 100644 --- a/includes/installer/PostgresInstaller.php +++ b/includes/installer/PostgresInstaller.php @@ -587,9 +587,7 @@ class PostgresInstaller extends DatabaseInstaller { return $status; } - /** - * @var $conn Database - */ + /** @var $conn DatabasePostgres */ $conn = $status->value; if ( $conn->tableExists( 'archive' ) ) { @@ -606,7 +604,7 @@ class PostgresInstaller extends DatabaseInstaller { return $status; } - $error = $conn->sourceFile( $conn->getSchemaPath() ); + $error = $conn->sourceFile( $this->getSchemaPath( $conn ) ); if ( $error !== true ) { $conn->reportQueryError( $error, 0, '', __METHOD__ ); $conn->rollback( __METHOD__ ); diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index a5a170ba06..f33e244adb 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -78,7 +78,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** @var callback Error logging callback */ protected $errorLogger; - /** @var resource Database connection */ + /** @var resource|null Database connection */ protected $mConn = null; /** @var bool */ protected $mOpened = false; @@ -382,7 +382,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } if ( !isset( $p['errorLogger'] ) ) { $p['errorLogger'] = function ( Exception $e ) { - trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_WARNING ); + trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_USER_WARNING ); }; } @@ -773,8 +773,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * @return bool */ protected function isTransactableQuery( $sql ) { - $verb = $this->getQueryVerb( $sql ); - return !in_array( $verb, [ 'BEGIN', 'COMMIT', 'ROLLBACK', 'SHOW', 'SET' ], true ); + return !in_array( + $this->getQueryVerb( $sql ), + [ 'BEGIN', 'COMMIT', 'ROLLBACK', 'SHOW', 'SET', 'CREATE', 'ALTER' ], + true + ); } /** @@ -2828,7 +2831,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $fnames = implode( ', ', $this->pendingWriteAndCallbackCallers() ); throw new DBUnexpectedError( $this, - "$fname: Cannot COMMIT to clear snapshot because writes are pending ($fnames)." + "$fname: Cannot flush snapshot because writes are pending ($fnames)." ); } @@ -3249,7 +3252,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $fnames = implode( ', ', $this->pendingWriteAndCallbackCallers() ); throw new DBUnexpectedError( $this, - "$fname: Cannot COMMIT to clear snapshot because writes are pending ($fnames)." + "$fname: Cannot flush pre-lock snapshot because writes are pending ($fnames)." ); } @@ -3368,6 +3371,28 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return true; } + /** + * Get the underlying binding handle, mConn + * + * Makes sure that mConn is set (disconnects and ping() failure can unset it). + * This catches broken callers than catch and ignore disconnection exceptions. + * Unlike checking isOpen(), this is safe to call inside of open(). + * + * @return resource|object + * @throws DBUnexpectedError + * @since 1.26 + */ + protected function getBindingHandle() { + if ( !$this->mConn ) { + throw new DBUnexpectedError( + $this, + 'DB connection was already closed or the connection dropped.' + ); + } + + return $this->mConn; + } + /** * @since 1.19 * @return string @@ -3422,8 +3447,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } if ( $this->mConn ) { - // Avoid connection leaks for sanity + // Avoid connection leaks for sanity. Normally, resources close at script completion. + // The connection might already be closed in zend/hhvm by now, so suppress warnings. + \MediaWiki\suppressWarnings(); $this->closeConnection(); + \MediaWiki\restoreWarnings(); $this->mConn = false; $this->mOpened = false; } diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index 9662a5b9bb..3b7681e3bd 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -1204,28 +1204,6 @@ abstract class DatabaseMysqlBase extends Database { return $errno == 2013 || $errno == 2006; } - /** - * Get the underlying binding handle, mConn - * - * Makes sure that mConn is set (disconnects and ping() failure can unset it). - * This catches broken callers than catch and ignore disconnection exceptions. - * Unlike checking isOpen(), this is safe to call inside of open(). - * - * @return resource|object - * @throws DBUnexpectedError - * @since 1.26 - */ - protected function getBindingHandle() { - if ( !$this->mConn ) { - throw new DBUnexpectedError( - $this, - 'DB connection was already closed or the connection dropped.' - ); - } - - return $this->mConn; - } - /** * @param string $oldName * @param string $newName diff --git a/includes/libs/rdbms/database/DatabaseMysqli.php b/includes/libs/rdbms/database/DatabaseMysqli.php index c34f9018ec..2f27ff9736 100644 --- a/includes/libs/rdbms/database/DatabaseMysqli.php +++ b/includes/libs/rdbms/database/DatabaseMysqli.php @@ -29,8 +29,7 @@ * @see Database */ class DatabaseMysqli extends DatabaseMysqlBase { - /** @var mysqli */ - protected $mConn; + /** @var $mConn mysqli */ /** * @param string $sql diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index f82d76d4d8..84021a03e0 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -61,10 +61,12 @@ class DatabasePostgres extends Database { } function hasConstraint( $name ) { + $conn = $this->getBindingHandle(); + $sql = "SELECT 1 FROM pg_catalog.pg_constraint c, pg_catalog.pg_namespace n " . "WHERE c.connamespace = n.oid AND conname = '" . - pg_escape_string( $this->mConn, $name ) . "' AND n.nspname = '" . - pg_escape_string( $this->mConn, $this->getCoreSchema() ) . "'"; + pg_escape_string( $conn, $name ) . "' AND n.nspname = '" . + pg_escape_string( $conn, $this->getCoreSchema() ) . "'"; $res = $this->doQuery( $sql ); return $this->numRows( $res ); @@ -185,19 +187,21 @@ class DatabasePostgres extends Database { * @return bool */ protected function closeConnection() { - return pg_close( $this->mConn ); + return $this->mConn ? pg_close( $this->mConn ) : true; } public function doQuery( $sql ) { + $conn = $this->getBindingHandle(); + $sql = mb_convert_encoding( $sql, 'UTF-8' ); // Clear previously left over PQresult - while ( $res = pg_get_result( $this->mConn ) ) { + while ( $res = pg_get_result( $conn ) ) { pg_free_result( $res ); } - if ( pg_send_query( $this->mConn, $sql ) === false ) { + if ( pg_send_query( $conn, $sql ) === false ) { throw new DBUnexpectedError( $this, "Unable to post new query to PostgreSQL\n" ); } - $this->mLastResult = pg_get_result( $this->mConn ); + $this->mLastResult = pg_get_result( $conn ); $this->mAffectedRows = null; if ( pg_result_error( $this->mLastResult ) ) { return false; @@ -281,10 +285,11 @@ class DatabasePostgres extends Database { # @todo hashar: not sure if the following test really trigger if the object # fetching failed. - if ( pg_last_error( $this->mConn ) ) { + $conn = $this->getBindingHandle(); + if ( pg_last_error( $conn ) ) { throw new DBUnexpectedError( $this, - 'SQL error: ' . htmlspecialchars( pg_last_error( $this->mConn ) ) + 'SQL error: ' . htmlspecialchars( pg_last_error( $conn ) ) ); } @@ -298,10 +303,12 @@ class DatabasePostgres extends Database { MediaWiki\suppressWarnings(); $row = pg_fetch_array( $res ); MediaWiki\restoreWarnings(); - if ( pg_last_error( $this->mConn ) ) { + + $conn = $this->getBindingHandle(); + if ( pg_last_error( $conn ) ) { throw new DBUnexpectedError( $this, - 'SQL error: ' . htmlspecialchars( pg_last_error( $this->mConn ) ) + 'SQL error: ' . htmlspecialchars( pg_last_error( $conn ) ) ); } @@ -315,10 +322,12 @@ class DatabasePostgres extends Database { MediaWiki\suppressWarnings(); $n = pg_num_rows( $res ); MediaWiki\restoreWarnings(); - if ( pg_last_error( $this->mConn ) ) { + + $conn = $this->getBindingHandle(); + if ( pg_last_error( $conn ) ) { throw new DBUnexpectedError( $this, - 'SQL error: ' . htmlspecialchars( pg_last_error( $this->mConn ) ) + 'SQL error: ' . htmlspecialchars( pg_last_error( $conn ) ) ); } @@ -1033,7 +1042,7 @@ __INDEXATTR__; $this->mCoreSchema . "\"\n" ); } /* Commit SET otherwise it will be rollbacked on error or IGNORE SELECT */ - $this->commit( __METHOD__ ); + $this->commit( __METHOD__, self::FLUSHING_INTERNAL ); } /** @@ -1051,7 +1060,8 @@ __INDEXATTR__; */ function getServerVersion() { if ( !isset( $this->numericVersion ) ) { - $versionInfo = pg_version( $this->mConn ); + $conn = $this->getBindingHandle(); + $versionInfo = pg_version( $conn ); if ( version_compare( $versionInfo['client'], '7.4.0', 'lt' ) ) { // Old client, abort install $this->numericVersion = '7.3 or earlier'; @@ -1060,7 +1070,7 @@ __INDEXATTR__; $this->numericVersion = $versionInfo['server']; } else { // Bug 16937: broken pgsql extension from PHP<5.3 - $this->numericVersion = pg_parameter_status( $this->mConn, 'server_version' ); + $this->numericVersion = pg_parameter_status( $conn, 'server_version' ); } } @@ -1229,7 +1239,7 @@ SQL; function strencode( $s ) { // Should not be called by us - return pg_escape_string( $this->mConn, $s ); + return pg_escape_string( $this->getBindingHandle(), $s ); } /** @@ -1237,6 +1247,8 @@ SQL; * @return string|int */ function addQuotes( $s ) { + $conn = $this->getBindingHandle(); + if ( is_null( $s ) ) { return 'NULL'; } elseif ( is_bool( $s ) ) { @@ -1245,12 +1257,12 @@ SQL; if ( $s instanceof PostgresBlob ) { $s = $s->fetch(); } else { - $s = pg_escape_bytea( $this->mConn, $s->fetch() ); + $s = pg_escape_bytea( $conn, $s->fetch() ); } return "'$s'"; } - return "'" . pg_escape_string( $this->mConn, $s ) . "'"; + return "'" . pg_escape_string( $conn, $s ) . "'"; } /** diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 3ccf3f002a..31bb26b75c 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -44,8 +44,7 @@ class DatabaseSqlite extends Database { /** @var resource */ protected $mLastResult; - /** @var PDO */ - protected $mConn; + /** @var $mConn PDO */ /** @var FSLockManager (hopefully on the same server as the DB) */ protected $lockMgr; diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 929bd4d4f1..d107e104e4 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -96,7 +96,7 @@ abstract class LBFactory implements ILBFactory { $this->errorLogger = isset( $conf['errorLogger'] ) ? $conf['errorLogger'] : function ( Exception $e ) { - trigger_error( E_WARNING, get_class( $e ) . ': ' . $e->getMessage() ); + trigger_error( E_USER_WARNING, get_class( $e ) . ': ' . $e->getMessage() ); }; $this->profiler = isset( $params['profiler'] ) ? $params['profiler'] : null; diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 32df19dcd7..682698d05d 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -189,7 +189,7 @@ class LoadBalancer implements ILoadBalancer { $this->errorLogger = isset( $params['errorLogger'] ) ? $params['errorLogger'] : function ( Exception $e ) { - trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_WARNING ); + trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_USER_WARNING ); }; foreach ( [ 'replLogger', 'connLogger', 'queryLogger', 'perfLogger' ] as $key ) { diff --git a/maintenance/doMaintenance.php b/maintenance/doMaintenance.php index 60b24a2db0..f3561b5538 100644 --- a/maintenance/doMaintenance.php +++ b/maintenance/doMaintenance.php @@ -25,6 +25,7 @@ * @file * @ingroup Maintenance */ +use MediaWiki\MediaWikiServices; if ( !defined( 'RUN_MAINTENANCE_IF_MAIN' ) ) { echo "This file must be included after Maintenance.php\n"; @@ -113,12 +114,13 @@ $maintenance->execute(); $maintenance->globals(); // Perform deferred updates. +$lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); +$lbFactory->commitMasterChanges( $maintClass ); DeferredUpdates::doUpdates(); // log profiling info wfLogProfilingData(); // Commit and close up! -$factory = wfGetLBFactory(); -$factory->commitMasterChanges( 'doMaintenance' ); -$factory->shutdown( $factory::SHUTDOWN_NO_CHRONPROT ); +$lbFactory->commitMasterChanges( 'doMaintenance' ); +$lbFactory->shutdown( $lbFactory::SHUTDOWN_NO_CHRONPROT ); -- 2.20.1