Reduced LocalFileRestoreBatch::execute deadlocks when doing batch restores

* Often users restore a batch of many files with similar names.
  This fails due to gap locking on non-existing rows which tangles up
  unrelated actions. Avoid that locking for the common case.
* This changes the return value of LocalFile::lock() from a useless
  one (due to the exception) to one that conveys some useful information.
  The method is fairly internal in any case.

Change-Id: Idb86367cf45b731d4b2a67b1813a660bc0e7a84f

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index 6ca19fe..08c9afb 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -1850,7 +1850,7 @@ class LocalFile extends File {
         * Start a transaction and lock the image for update
         * Increments a reference counter if the lock is already held
         * @throws MWException Throws an error if the lock was not acquired
-        * @return bool Success
+        * @return bool Whether the file lock owns/spawned the DB transaction
         */
        function lock() {
                $dbw = $this->repo->getMasterDB();
@@ -1877,7 +1877,7 @@ class LocalFile extends File {
 
                $this->markVolatile(); // file may change soon
 
-               return true;
+               return $this->lockedOwnTrx;
        }
 
        /**
@@ -2419,13 +2419,19 @@ class LocalFileRestoreBatch {
                        return $this->file->repo->newGood();
                }
 
-               $this->file->lock();
+               $lockOwnsTrx = $this->file->lock();
 
                $dbw = $this->file->repo->getMasterDB();
                $status = $this->file->repo->newGood();
 
                $exists = (bool)$dbw->selectField( 'image', '1',
-                       array( 'img_name' => $this->file->getName() ), __METHOD__, array( 'FOR UPDATE' ) );
+                       array( 'img_name' => $this->file->getName() ),
+                       __METHOD__,
+                       // The lock() should already prevents changes, but this still may need
+                       // to bypass any transaction snapshot. However, if lock() started the
+                       // trx (which it probably did) then snapshot is post-lock and up-to-date.
+                       $lockOwnsTrx ? array() : array( 'LOCK IN SHARE MODE' )
+               );
 
                // Fetch all or selected archived revisions for the file,
                // sorted from the most recent to the oldest.