From: Aaron Schulz <aschulz@wikimedia.org>
Date: Tue, 23 Aug 2016 17:10:48 +0000 (-0700)
Subject: Avoid INSERT..SELECT in LocalFileDeleteBatch
X-Git-Tag: 1.31.0-rc.0~5913^2
X-Git-Url: http://git.cyclocoop.org/%7B%24admin_url%7Dmes_infos.php?a=commitdiff_plain;h=e2c03a8afd4fff0b364c8d64f1aca8c97dde9cfa;p=lhc%2Fweb%2Fwiklou.git

Avoid INSERT..SELECT in LocalFileDeleteBatch

That construct has poor locking characteristics in terms of
auto-inc columns as well as not allowing such inserts concurrently
for statement-based replication. Also, the INSERT..SELECT did not
have an ORDER BY, which could lead to fa_id drift with statement
based replication.

Change-Id: Iaacb75d9931b4cd24b70bdcaadd0e3979c7e9c90
---

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index 7e6e651746..40141c9cd0 100644
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -2216,8 +2216,9 @@ class LocalFileDeleteBatch {
 	}
 
 	protected function doDBInserts() {
+		$now = time();
 		$dbw = $this->file->repo->getMasterDB();
-		$encTimestamp = $dbw->addQuotes( $dbw->timestamp() );
+		$encTimestamp = $dbw->addQuotes( $dbw->timestamp( $now ) );
 		$encUserId = $dbw->addQuotes( $this->user->getId() );
 		$encReason = $dbw->addQuotes( $this->reason );
 		$encGroup = $dbw->addQuotes( 'deleted' );
@@ -2239,15 +2240,15 @@ class LocalFileDeleteBatch {
 		}
 
 		if ( $deleteCurrent ) {
-			$concat = $dbw->buildConcat( [ "img_sha1", $encExt ] );
-			$where = [ 'img_name' => $this->file->getName() ];
-			$dbw->insertSelect( 'filearchive', 'image',
+			$dbw->insertSelect(
+				'filearchive',
+				'image',
 				[
 					'fa_storage_group' => $encGroup,
 					'fa_storage_key' => $dbw->conditional(
 						[ 'img_sha1' => '' ],
 						$dbw->addQuotes( '' ),
-						$concat
+						$dbw->buildConcat( [ "img_sha1", $encExt ] )
 					),
 					'fa_deleted_user' => $encUserId,
 					'fa_deleted_timestamp' => $encTimestamp,
@@ -2268,44 +2269,56 @@ class LocalFileDeleteBatch {
 					'fa_user' => 'img_user',
 					'fa_user_text' => 'img_user_text',
 					'fa_timestamp' => 'img_timestamp',
-					'fa_sha1' => 'img_sha1',
-				], $where, __METHOD__ );
+					'fa_sha1' => 'img_sha1'
+				],
+				[ 'img_name' => $this->file->getName() ],
+				__METHOD__
+			);
 		}
 
 		if ( count( $oldRels ) ) {
-			$concat = $dbw->buildConcat( [ "oi_sha1", $encExt ] );
-			$where = [
-				'oi_name' => $this->file->getName(),
-				'oi_archive_name' => array_keys( $oldRels ) ];
-			$dbw->insertSelect( 'filearchive', 'oldimage',
+			$res = $dbw->select(
+				'oldimage',
+				OldLocalFile::selectFields(),
 				[
-					'fa_storage_group' => $encGroup,
-					'fa_storage_key' => $dbw->conditional(
-						[ 'oi_sha1' => '' ],
-						$dbw->addQuotes( '' ),
-						$concat
-					),
-					'fa_deleted_user' => $encUserId,
-					'fa_deleted_timestamp' => $encTimestamp,
-					'fa_deleted_reason' => $encReason,
-					'fa_deleted' => $this->suppress ? $bitfield : 'oi_deleted',
-
-					'fa_name' => 'oi_name',
-					'fa_archive_name' => 'oi_archive_name',
-					'fa_size' => 'oi_size',
-					'fa_width' => 'oi_width',
-					'fa_height' => 'oi_height',
-					'fa_metadata' => 'oi_metadata',
-					'fa_bits' => 'oi_bits',
-					'fa_media_type' => 'oi_media_type',
-					'fa_major_mime' => 'oi_major_mime',
-					'fa_minor_mime' => 'oi_minor_mime',
-					'fa_description' => 'oi_description',
-					'fa_user' => 'oi_user',
-					'fa_user_text' => 'oi_user_text',
-					'fa_timestamp' => 'oi_timestamp',
-					'fa_sha1' => 'oi_sha1',
-				], $where, __METHOD__ );
+					'oi_name' => $this->file->getName(),
+					'oi_archive_name' => array_keys( $oldRels )
+				],
+				__METHOD__,
+				[ 'FOR UPDATE' ]
+			);
+			$rowsInsert = [];
+			foreach ( $res as $row ) {
+				$rowsInsert[] = [
+					// Deletion-specific fields
+					'fa_storage_group' => 'deleted',
+					'fa_storage_key' => ( $row->oi_sha1 === '' )
+						? ''
+						: "{$row->oi_sha1}{$dotExt}",
+					'fa_deleted_user' => $this->user->getId(),
+					'fa_deleted_timestamp' => $dbw->timestamp( $now ),
+					'fa_deleted_reason' => $this->reason,
+					// Counterpart fields
+					'fa_deleted' => $this->suppress ? $bitfield : $row->oi_deleted,
+					'fa_name' => $row->oi_name,
+					'fa_archive_name' => $row->oi_archive_name,
+					'fa_size' => $row->oi_size,
+					'fa_width' => $row->oi_width,
+					'fa_height' => $row->oi_height,
+					'fa_metadata' => $row->oi_metadata,
+					'fa_bits' => $row->oi_bits,
+					'fa_media_type' => $row->oi_media_type,
+					'fa_major_mime' => $row->oi_major_mime,
+					'fa_minor_mime' => $row->oi_minor_mime,
+					'fa_description' => $row->oi_description,
+					'fa_user' => $row->oi_user,
+					'fa_user_text' => $row->oi_user_text,
+					'fa_timestamp' => $row->oi_timestamp,
+					'fa_sha1' => $row->oi_sha1
+				];
+			}
+
+			$dbw->insert( 'filearchive', $rowsInsert, __METHOD__ );
 		}
 	}