Migrating merge request diffs to (and from) external storage holds open a database transaction
Summary
As noted in gitlab-com/gl-infra/production#1851 (comment 334699787) by @andrewn , we hold open a database transaction for the duration of a merge request diff migration to external storage (or back). This uses up a shared resource (pgbouncer connections) on GitLab.com.
Steps to reproduce
- Migrate a very large diff to object storage
- Observe transaction timings
What is the current bug behavior?
Transaction is open for the duration of the copy to object storage
What is the expected correct behavior?
Transaction should only be opened once the file has been created in object storage
Possible fixes
The offending methods are MergeRequestDiff#migrate_files_to_external_storage!
and MergeRequestDiff#migrate_files_to_database!
We should be able to remove the transactions, but particular care needs to be taken when reasoning about the behaviour of carrierwave to minimise the chances of data loss.
For migrating to external storage, we should:
- Create the external file
- Transactionally update the diffs
- (Best-effort) remove the external file if the update failed
For migrating back, we should:
- Transactionally update the diffs
- Remove the external file