From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 28 Feb 2018 22:41:18 +0000 (-0800)
Subject: rdbms: make replace()/upsert() use atomic sections
X-Git-Tag: 1.31.0-rc.0~473
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=3193c0fb28256c9fa69f1bb24a3c01c87b9cab3c

rdbms: make replace()/upsert() use atomic sections

This makes them consistent with nonNativeInsertSelect()

Style aside, there is also the potential edge case of:
a) no transaction being active yet, with
b) the first query being replace() or upsert(), with
c) DBO_TRX being set (usually part of an implicit transaction round)

Previously, in that case, these methods would commit the transaction
they started, rather than leave it open. The correct semantics are
to leave it open. Since MySQL redefined upsert() to use ON DUPLICATE
KEY UPDATE, it already had the right behavior.

Also make sure that rollback() always sets the affectedRowCount
field to 0.

Change-Id: I15f923d3d4799cffc60e3aaea934f4ca1a9488e1
---

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 9e9b7ab4af..2992e7628e 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -2266,11 +2266,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			$rows = [ $rows ];
 		}
 
-		$useTrx = !$this->trxLevel;
-		if ( $useTrx ) {
-			$this->begin( $fname, self::TRANSACTION_INTERNAL );
-		}
 		try {
+			$this->startAtomic( $fname );
 			$affectedRowCount = 0;
 			foreach ( $rows as $row ) {
 				// Delete rows which collide with this one
@@ -2303,17 +2300,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 				$this->insert( $table, $row, $fname );
 				$affectedRowCount += $this->affectedRows();
 			}
+			$this->endAtomic( $fname );
+			$this->affectedRowCount = $affectedRowCount;
 		} catch ( Exception $e ) {
-			if ( $useTrx ) {
-				$this->rollback( $fname, self::FLUSHING_INTERNAL );
-			}
+			$this->rollback( $fname, self::FLUSHING_INTERNAL );
 			throw $e;
 		}
-		if ( $useTrx ) {
-			$this->commit( $fname, self::FLUSHING_INTERNAL );
-		}
-
-		$this->affectedRowCount = $affectedRowCount;
 	}
 
 	/**
@@ -2379,11 +2371,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		}
 
 		$affectedRowCount = 0;
-		$useTrx = !$this->trxLevel;
-		if ( $useTrx ) {
-			$this->begin( $fname, self::TRANSACTION_INTERNAL );
-		}
 		try {
+			$this->startAtomic( $fname );
 			# Update any existing conflicting row(s)
 			if ( $where !== false ) {
 				$ok = $this->update( $table, $set, $where, $fname );
@@ -2394,16 +2383,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			# Now insert any non-conflicting row(s)
 			$ok = $this->insert( $table, $rows, $fname, [ 'IGNORE' ] ) && $ok;
 			$affectedRowCount += $this->affectedRows();
+			$this->endAtomic( $fname );
+			$this->affectedRowCount = $affectedRowCount;
 		} catch ( Exception $e ) {
-			if ( $useTrx ) {
-				$this->rollback( $fname, self::FLUSHING_INTERNAL );
-			}
+			$this->rollback( $fname, self::FLUSHING_INTERNAL );
 			throw $e;
 		}
-		if ( $useTrx ) {
-			$this->commit( $fname, self::FLUSHING_INTERNAL );
-		}
-		$this->affectedRowCount = $affectedRowCount;
 
 		return $ok;
 	}
@@ -2557,12 +2542,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 				$this->affectedRowCount = $affectedRowCount;
 			} else {
 				$this->rollback( $fname, self::FLUSHING_INTERNAL );
-				$this->affectedRowCount = 0;
 			}
 			return $ok;
 		} catch ( Exception $e ) {
 			$this->rollback( $fname, self::FLUSHING_INTERNAL );
-			$this->affectedRowCount = 0;
 			throw $e;
 		}
 	}
@@ -3194,6 +3177,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		} catch ( Exception $e ) {
 			// already logged; let LoadBalancer move on during mass-rollback
 		}
+
+		$this->affectedRowCount = 0; // for the sake of consistency
 	}
 
 	/**