Merge branch '216458-migrate-mr-diffs-without-transaction' into 'master'

MR diff migration: perform I/O outside of database transaction

See merge request gitlab-org/gitlab!35734

app/models/merge_request_diff.rb

@@ -414,10 +414,16 @@ class MergeRequestDiff < ApplicationRecord
     return if stored_externally? || !use_external_diff? || merge_request_diff_files.count == 0
 
     rows = build_merge_request_diff_files(merge_request_diff_files)
+    rows = build_external_merge_request_diff_files(rows)
+
+    # Perform carrierwave activity before entering the database transaction.
+    # This is safe as until the `external_diff_store` column is changed, we will
+    # continue to consult the in-database content.
+    self.external_diff.store!
 
     transaction do
       MergeRequestDiffFile.where(merge_request_diff_id: id).delete_all
-      create_merge_request_diff_files(rows)
+      Gitlab::Database.bulk_insert('merge_request_diff_files', rows) # rubocop:disable Gitlab/BulkInsert
       save!
     end
 

changelogs/unreleased/216458-migrate-mr-diffs-without-transaction.yml

+---
+title: 'MR diff migration: perform I/O outside of database transaction'
+merge_request: 35734
+author:
+type: performance

config/gitlab.yml.example

@@ -1213,7 +1213,7 @@ test:
     # has been pushed).
     # when: always
     # The location where external diffs are stored (default: shared/external-diffs).
-    # storage_path: shared/external-diffs
+    storage_path: tmp/tests/external-diffs
     object_store:
       enabled: false
       remote_directory: external-diffs # The bucket name

spec/models/merge_request_diff_spec.rb

@@ -177,6 +177,30 @@ RSpec.describe MergeRequestDiff do
       expect(diff.external_diff_store).to eq(file_store)
     end
 
+    it 'safely handles a transaction error when migrating to external storage' do
+      expect(diff).not_to be_stored_externally
+      expect(diff.external_diff).not_to be_exists
+
+      stub_external_diffs_setting(enabled: true)
+
+      expect(diff).not_to receive(:save!)
+      expect(Gitlab::Database)
+        .to receive(:bulk_insert)
+        .with('merge_request_diff_files', anything)
+        .and_raise(ActiveRecord::Rollback)
+
+      expect { diff.migrate_files_to_external_storage! }.not_to change(diff, :merge_request_diff_files)
+
+      diff.reload
+
+      expect(diff).not_to be_stored_externally
+
+      # The diff is written outside of the transaction, which is desirable to
+      # avoid long transaction times when migrating, but it does mean we can
+      # leave the file dangling on failure
+      expect(diff.external_diff).to be_exists
+    end
+
     it 'converts from in-database to external object storage' do
       expect(diff).not_to be_stored_externally