From 646a9490f74f3841803299a4e4d2d5677c5f1bba Mon Sep 17 00:00:00 2001 From: saper Date: Fri, 30 Mar 2012 02:10:52 +0200 Subject: [PATCH] (bug 27283) SqlBagOStuff breaks PostgreSQL txns * SqlBagOStuff should not turn off automatic transaction control just like that. Keep previous connection for PostgreSQL and SQLite * 94633ff448e9253c5503124df6748be1fc26b8e2 introduced rollback on error to restore sanity for the connection. Don't do this if you are in the middle of INSERT IGNORE and we have a savepoint open. Making sure query syntax errors don't get silenced by our "wonderful" implementation of INSERT IGNORE is bug 35572. Change-Id: I841b03895e1189c47307fefb1516c4c7c4102e25 --- RELEASE-NOTES-1.20 | 1 + includes/db/Database.php | 32 +++++- includes/db/DatabasePostgres.php | 156 ++++++++++++++++++-------- includes/objectcache/SqlBagOStuff.php | 21 +++- 4 files changed, 152 insertions(+), 58 deletions(-) diff --git a/RELEASE-NOTES-1.20 b/RELEASE-NOTES-1.20 index fefb6864d1..9522fc7a20 100644 --- a/RELEASE-NOTES-1.20 +++ b/RELEASE-NOTES-1.20 @@ -119,6 +119,7 @@ upgrade PHP if you have not done so prior to upgrading MediaWiki. * (bug 35264) Wrong type used for in export.xsd * (bug 24985) Use $wgTmpDirectory as the default temp directory so that people who don't have access to /tmp can specify an alternative. +* (bug 27283) SqlBagOStuff breaks PostgreSQL transactions === API changes in 1.20 === * (bug 34316) Add ability to retrieve maximum upload size from MediaWiki API. diff --git a/includes/db/Database.php b/includes/db/Database.php index b972f3b140..0a1f988cbf 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -530,7 +530,11 @@ abstract class DatabaseBase implements DatabaseType { * - DBO_PERSISTENT: use persistant database connection */ function setFlag( $flag ) { + global $wgDebugDBTransactions; $this->mFlags |= $flag; + if ( ( $flag & DBO_TRX) & $wgDebugDBTransactions ) { + wfDebug("Implicit transactions are now disabled.\n"); + } } /** @@ -539,7 +543,11 @@ abstract class DatabaseBase implements DatabaseType { * @param $flag: same as setFlag()'s $flag param */ function clearFlag( $flag ) { + global $wgDebugDBTransactions; $this->mFlags &= ~$flag; + if ( ( $flag & DBO_TRX ) && $wgDebugDBTransactions ) { + wfDebug("Implicit transactions are now disabled.\n"); + } } /** @@ -604,15 +612,21 @@ abstract class DatabaseBase implements DatabaseType { function __construct( $server = false, $user = false, $password = false, $dbName = false, $flags = 0, $tablePrefix = 'get from global' ) { - global $wgDBprefix, $wgCommandLineMode; + global $wgDBprefix, $wgCommandLineMode, $wgDebugDBTransactions; $this->mFlags = $flags; if ( $this->mFlags & DBO_DEFAULT ) { if ( $wgCommandLineMode ) { $this->mFlags &= ~DBO_TRX; + if ( $wgDebugDBTransactions ) { + wfDebug("Implicit transaction open disabled.\n"); + } } else { $this->mFlags |= DBO_TRX; + if ( $wgDebugDBTransactions ) { + wfDebug("Implicit transaction open enabled.\n"); + } } } @@ -868,8 +882,13 @@ abstract class DatabaseBase implements DatabaseType { # that would delay transaction initializations to once connection # is really used by application $sqlstart = substr( $sql, 0, 10 ); // very much worth it, benchmark certified(tm) - if ( strpos( $sqlstart, "SHOW " ) !== 0 && strpos( $sqlstart, "SET " ) !== 0 ) + if ( strpos( $sqlstart, "SHOW " ) !== 0 && strpos( $sqlstart, "SET " ) !== 0 ) { + global $wgDebugDBTransactions; + if ( $wgDebugDBTransactions ) { + wfDebug("Implicit transaction start.\n"); + } $this->begin( __METHOD__ . " ($fname)" ); + } } if ( $this->debug() ) { @@ -2885,7 +2904,7 @@ abstract class DatabaseBase implements DatabaseType { } /** - * Begin a transaction, committing any previously open transaction + * Begin a transaction * * @param $fname string */ @@ -3506,4 +3525,11 @@ abstract class DatabaseBase implements DatabaseType { public function setBigSelects( $value = true ) { // no-op } + + /** + * @since 1.19 + */ + public function __toString() { + return (string)$this->mConn; + } } diff --git a/includes/db/DatabasePostgres.php b/includes/db/DatabasePostgres.php index d058769527..f0838bb690 100644 --- a/includes/db/DatabasePostgres.php +++ b/includes/db/DatabasePostgres.php @@ -137,14 +137,14 @@ class PostgresTransactionState { static $WATCHED = array( array( - "desc" => "Connection state changed from %s -> %s\n", + "desc" => "%s: Connection state changed from %s -> %s\n", "states" => array( PGSQL_CONNECTION_OK => "OK", PGSQL_CONNECTION_BAD => "BAD" ) ), array( - "desc" => "Transaction state changed from %s -> %s\n", + "desc" => "%s: Transaction state changed from %s -> %s\n", "states" => array( PGSQL_TRANSACTION_IDLE => "IDLE", PGSQL_TRANSACTION_ACTIVE => "ACTIVE", @@ -198,12 +198,86 @@ class PostgresTransactionState { protected function log_changed( $old, $new, $watched ) { wfDebug(sprintf($watched["desc"], + $this->mConn, $this->describe_changed( $old, $watched["states"] ), $this->describe_changed( $new, $watched["states"] )) ); } } +/** + * Manage savepoints within a transaction + * @ingroup Database + * @since 1.19 + */ +class SavepointPostgres { + /** + * Establish a savepoint within a transaction + */ + protected $dbw; + protected $id; + protected $didbegin; + + public function __construct ($dbw, $id) { + $this->dbw = $dbw; + $this->id = $id; + $this->didbegin = false; + /* If we are not in a transaction, we need to be for savepoint trickery */ + if ( !$dbw->trxLevel() ) { + $dbw->begin( "FOR SAVEPOINT" ); + $this->didbegin = true; + } + } + + public function __destruct() { + if ( $this->didbegin ) { + $this->dbw->rollback(); + } + } + + public function commit() { + if ( $this->didbegin ) { + $this->dbw->commit(); + } + } + + protected function query( $keyword, $msg_ok, $msg_failed ) { + global $wgDebugDBTransactions; + if ( $this->dbw->doQuery( $keyword . " " . $this->id ) !== false ) { + if ( $wgDebugDBTransactions ) { + wfDebug( sprintf ($msg_ok, $this->id ) ); + } + } else { + wfDebug( sprintf ($msg_failed, $this->id ) ); + } + } + + public function savepoint() { + $this->query("SAVEPOINT", + "Transaction state: savepoint \"%s\" established.\n", + "Transaction state: establishment of savepoint \"%s\" FAILED.\n" + ); + } + + public function release() { + $this->query("RELEASE", + "Transaction state: savepoint \"%s\" released.\n", + "Transaction state: release of savepoint \"%s\" FAILED.\n" + ); + } + + public function rollback() { + $this->query("ROLLBACK TO", + "Transaction state: savepoint \"%s\" rolled back.\n", + "Transaction state: rollback of savepoint \"%s\" FAILED.\n" + ); + } + + public function __toString() { + return (string)$this->id; + } +} + /** * @ingroup Database */ @@ -345,7 +419,7 @@ class DatabasePostgres extends DatabaseBase { return pg_close( $this->mConn ); } - protected function doQuery( $sql ) { + public function doQuery( $sql ) { if ( function_exists( 'mb_convert_encoding' ) ) { $sql = mb_convert_encoding( $sql, 'UTF-8' ); } @@ -667,15 +741,9 @@ __INDEXATTR__; } // If IGNORE is set, we use savepoints to emulate mysql's behavior - $ignore = in_array( 'IGNORE', $options ) ? 'mw' : ''; - - // If we are not in a transaction, we need to be for savepoint trickery - $didbegin = 0; - if ( $ignore ) { - if ( !$this->mTrxLevel ) { - $this->begin( __METHOD__ ); - $didbegin = 1; - } + $savepoint = null; + if ( in_array( 'IGNORE', $options ) ) { + $savepoint = new SavepointPostgres( $this, 'mw' ); $olde = error_reporting( 0 ); // For future use, we may want to track the number of actual inserts // Right now, insert (all writes) simply return true/false @@ -685,7 +753,7 @@ __INDEXATTR__; $sql = "INSERT INTO $table (" . implode( ',', $keys ) . ') VALUES '; if ( $multi ) { - if ( $this->numeric_version >= 8.2 && !$ignore ) { + if ( $this->numeric_version >= 8.2 && !$savepoint ) { $first = true; foreach ( $args as $row ) { if ( $first ) { @@ -695,7 +763,7 @@ __INDEXATTR__; } $sql .= '(' . $this->makeList( $row ) . ')'; } - $res = (bool)$this->query( $sql, $fname, $ignore ); + $res = (bool)$this->query( $sql, $fname, $savepoint ); } else { $res = true; $origsql = $sql; @@ -703,18 +771,18 @@ __INDEXATTR__; $tempsql = $origsql; $tempsql .= '(' . $this->makeList( $row ) . ')'; - if ( $ignore ) { - $this->doQuery( "SAVEPOINT $ignore" ); + if ( $savepoint ) { + $savepoint->savepoint(); } - $tempres = (bool)$this->query( $tempsql, $fname, $ignore ); + $tempres = (bool)$this->query( $tempsql, $fname, $savepoint ); - if ( $ignore ) { + if ( $savepoint ) { $bar = pg_last_error(); if ( $bar != false ) { - $this->doQuery( $this->mConn, "ROLLBACK TO $ignore" ); + $savepoint->rollback(); } else { - $this->doQuery( $this->mConn, "RELEASE $ignore" ); + $savepoint->release(); $numrowsinserted++; } } @@ -728,27 +796,25 @@ __INDEXATTR__; } } else { // Not multi, just a lone insert - if ( $ignore ) { - $this->doQuery( "SAVEPOINT $ignore" ); + if ( $savepoint ) { + $savepoint->savepoint(); } $sql .= '(' . $this->makeList( $args ) . ')'; - $res = (bool)$this->query( $sql, $fname, $ignore ); - if ( $ignore ) { + $res = (bool)$this->query( $sql, $fname, $savepoint ); + if ( $savepoint ) { $bar = pg_last_error(); if ( $bar != false ) { - $this->doQuery( "ROLLBACK TO $ignore" ); + $savepoint->rollback(); } else { - $this->doQuery( "RELEASE $ignore" ); + $savepoint->release(); $numrowsinserted++; } } } - if ( $ignore ) { + if ( $savepoint ) { $olde = error_reporting( $olde ); - if ( $didbegin ) { - $this->commit( __METHOD__ ); - } + $savepoint->commit(); // Set the affected row count for the whole operation $this->mAffectedRows = $numrowsinserted; @@ -774,9 +840,6 @@ __INDEXATTR__; { $destTable = $this->tableName( $destTable ); - // If IGNORE is set, we use savepoints to emulate mysql's behavior - $ignore = in_array( 'IGNORE', $insertOptions ) ? 'mw' : ''; - if( is_array( $insertOptions ) ) { $insertOptions = implode( ' ', $insertOptions ); // FIXME: This is unused } @@ -790,16 +853,13 @@ __INDEXATTR__; $srcTable = $this->tableName( $srcTable ); } - // If we are not in a transaction, we need to be for savepoint trickery - $didbegin = 0; - if ( $ignore ) { - if( !$this->mTrxLevel ) { - $this->begin( __METHOD__ ); - $didbegin = 1; - } + // If IGNORE is set, we use savepoints to emulate mysql's behavior + $savepoint = null; + if ( in_array( 'IGNORE', $options ) ) { + $savepoint = new SavepointPostgres( $this, 'mw' ); $olde = error_reporting( 0 ); $numrowsinserted = 0; - $this->doQuery( "SAVEPOINT $ignore" ); + $savepoint->savepoint(); } $sql = "INSERT INTO $destTable (" . implode( ',', array_keys( $varMap ) ) . ')' . @@ -812,19 +872,17 @@ __INDEXATTR__; $sql .= " $tailOpts"; - $res = (bool)$this->query( $sql, $fname, $ignore ); - if( $ignore ) { + $res = (bool)$this->query( $sql, $fname, $savepoint ); + if( $savepoint ) { $bar = pg_last_error(); if( $bar != false ) { - $this->doQuery( "ROLLBACK TO $ignore" ); + $savepoint->rollback(); } else { - $this->doQuery( "RELEASE $ignore" ); + $savepoint->release(); $numrowsinserted++; } $olde = error_reporting( $olde ); - if( $didbegin ) { - $this->commit( __METHOD__ ); - } + $savepoint->commit(); // Set the affected row count for the whole operation $this->mAffectedRows = $numrowsinserted; @@ -1056,7 +1114,7 @@ __INDEXATTR__; * This will be also called by the installer after the schema is created * * @since 1.19 - * @param desired_schema string + * @param $desired_schema string */ function determineCoreSchema( $desired_schema ) { $this->begin( __METHOD__ ); diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index e504887090..209975b373 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -87,25 +87,34 @@ class SqlBagOStuff extends BagOStuff { * @return DatabaseBase */ protected function getDB() { + global $wgDebugDBTransactions; if ( !isset( $this->db ) ) { # If server connection info was given, use that if ( $this->serverInfo ) { + if ( $wgDebugDBTransactions ) { + wfDebug( sprintf( "Using provided serverInfo for SqlBagOStuff\n" ) ); + } $this->lb = new LoadBalancer( array( 'servers' => array( $this->serverInfo ) ) ); $this->db = $this->lb->getConnection( DB_MASTER ); $this->db->clearFlag( DBO_TRX ); } else { - # We must keep a separate connection to MySQL in order to avoid deadlocks - # However, SQLite has an opposite behaviour. - # @todo Investigate behaviour for other databases - if ( wfGetDB( DB_MASTER )->getType() == 'sqlite' ) { - $this->db = wfGetDB( DB_MASTER ); - } else { + /* + * We must keep a separate connection to MySQL in order to avoid deadlocks + * However, SQLite has an opposite behaviour. And PostgreSQL needs to know + * if we are in transaction or no + */ + if ( wfGetDB( DB_MASTER )->getType() == 'mysql' ) { $this->lb = wfGetLBFactory()->newMainLB(); $this->db = $this->lb->getConnection( DB_MASTER ); $this->db->clearFlag( DBO_TRX ); + } else { + $this->db = wfGetDB( DB_MASTER ); } } + if ( $wgDebugDBTransactions ) { + wfDebug( sprintf( "Connection %s will be used for SqlBagOStuff\n", $this->db ) ); + } } return $this->db; -- 2.20.1