Add toggle to disallow subtransactions

This allows us to toggle a check in WithLockRetries: Unless we allow
subtransactions, we'll raise an error if the logic executes within an
already open transaction.

lib/gitlab/database/migration_helpers.rb

@@ -411,7 +411,8 @@ module Gitlab
         raise_on_exhaustion = !!kwargs.delete(:raise_on_exhaustion)
         merged_args = {
           klass: self.class,
-          logger: Gitlab::BackgroundMigration::Logger
+          logger: Gitlab::BackgroundMigration::Logger,
+          allow_subtrans: true
         }.merge(kwargs)
 
         Gitlab::Database::WithLockRetries.new(**merged_args)

lib/gitlab/database/with_lock_retries.rb

@@ -61,9 +61,10 @@ module Gitlab
         [10.seconds, 10.minutes]
       ].freeze
 
-      def initialize(logger: NULL_LOGGER, timing_configuration: DEFAULT_TIMING_CONFIGURATION, klass: nil, env: ENV)
+      def initialize(logger: NULL_LOGGER, allow_subtrans: true, timing_configuration: DEFAULT_TIMING_CONFIGURATION, klass: nil, env: ENV)
         @logger = logger
         @klass = klass
+        @allow_subtrans = allow_subtrans
         @timing_configuration = timing_configuration
         @env = env
         @current_iteration = 1
@@ -122,6 +123,8 @@ module Gitlab
       end
 
       def run_block_with_lock_timeout
+        raise "WithLockRetries should not run inside already open transaction" if ActiveRecord::Base.connection.transaction_open? && @allow_subtrans.blank?
+
         ActiveRecord::Base.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
           execute("SET LOCAL lock_timeout TO '#{current_lock_timeout_in_ms}ms'")
 

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -2331,6 +2331,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
 
       model.with_lock_retries(env: env, logger: in_memory_logger) { }
     end
+
+    it 'defaults to allowing subtransactions' do
+      with_lock_retries = double
+
+      expect(Gitlab::Database::WithLockRetries).to receive(:new).with(hash_including(allow_subtrans: true)).and_return(with_lock_retries)
+      expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false)
+
+      model.with_lock_retries(env: env, logger: in_memory_logger) { }
+    end
   end
 
   describe '#backfill_iids' do

spec/lib/gitlab/database/with_lock_retries_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe Gitlab::Database::WithLockRetries do
   let(:env) { {} }
   let(:logger) { Gitlab::Database::WithLockRetries::NULL_LOGGER }
-  let(:subject) { described_class.new(env: env, logger: logger, timing_configuration: timing_configuration) }
+  let(:subject) { described_class.new(env: env, logger: logger, allow_subtrans: allow_subtrans, timing_configuration: timing_configuration) }
+  let(:allow_subtrans) { true }
 
   let(:timing_configuration) do
     [
@@ -256,4 +257,20 @@ RSpec.describe Gitlab::Database::WithLockRetries do
       subject.run { }
     end
   end
+
+  context 'Stop using subtransactions - allow_subtrans: false' do
+    let(:allow_subtrans) { false }
+
+    it 'prevents running inside already open transaction' do
+      allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(true)
+
+      expect { subject.run { } }.to raise_error(/should not run inside already open transaction/)
+    end
+
+    it 'does not raise the error if not inside open transaction' do
+      allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false)
+
+      expect { subject.run { } }.not_to raise_error
+    end
+  end
 end