Commit 213ec7d1 authored by Nick Thomas's avatar Nick Thomas

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

Performing this I/O inside the transaction holds it open for longer
than necessary, which can lead to database connection saturation (since
a connection with an open transaction cannot be shared).
parent 6568763c
...@@ -414,10 +414,16 @@ class MergeRequestDiff < ApplicationRecord ...@@ -414,10 +414,16 @@ class MergeRequestDiff < ApplicationRecord
return if stored_externally? || !use_external_diff? || merge_request_diff_files.count == 0 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_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 transaction do
MergeRequestDiffFile.where(merge_request_diff_id: id).delete_all 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! save!
end end
......
---
title: 'MR diff migration: perform I/O outside of database transaction'
merge_request: 35734
author:
type: performance
...@@ -1213,7 +1213,7 @@ test: ...@@ -1213,7 +1213,7 @@ test:
# has been pushed). # has been pushed).
# when: always # when: always
# The location where external diffs are stored (default: shared/external-diffs). # The location where external diffs are stored (default: shared/external-diffs).
# storage_path: shared/external-diffs storage_path: tmp/tests/external-diffs
object_store: object_store:
enabled: false enabled: false
remote_directory: external-diffs # The bucket name remote_directory: external-diffs # The bucket name
......
...@@ -177,6 +177,30 @@ RSpec.describe MergeRequestDiff do ...@@ -177,6 +177,30 @@ RSpec.describe MergeRequestDiff do
expect(diff.external_diff_store).to eq(file_store) expect(diff.external_diff_store).to eq(file_store)
end 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 it 'converts from in-database to external object storage' do
expect(diff).not_to be_stored_externally expect(diff).not_to be_stored_externally
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment