Address review comments

db/post_migrate/20190214112022_schedule_sync_issuables_state_id.rb

@@ -18,6 +18,8 @@ class ScheduleSyncIssuablesStateId < ActiveRecord::Migration[5.0]
   ISSUES_MIGRATION = 'SyncIssuesStateId'.freeze
   MERGE_REQUESTS_MIGRATION = 'SyncMergeRequestsStateId'.freeze
 
+  disable_ddl_transaction!
+
   class Issue < ActiveRecord::Base
     include EachBatch
 
@@ -31,19 +33,19 @@ class ScheduleSyncIssuablesStateId < ActiveRecord::Migration[5.0]
   end
 
   def up
-    Sidekiq::Worker.skipping_transaction_check do
-      queue_background_migration_jobs_by_range_at_intervals(Issue.where(state_id: nil),
-                                                            ISSUES_MIGRATION,
-                                                            DELAY_INTERVAL,
-                                                            batch_size: BATCH_SIZE
-                                                           )
-
-      queue_background_migration_jobs_by_range_at_intervals(MergeRequest.where(state_id: nil),
-                                                            MERGE_REQUESTS_MIGRATION,
-                                                            DELAY_INTERVAL,
-                                                            batch_size: BATCH_SIZE
-                                                           )
-    end
+    queue_background_migration_jobs_by_range_at_intervals(
+      Issue.where(state_id: nil),
+      ISSUES_MIGRATION,
+      DELAY_INTERVAL,
+      batch_size: BATCH_SIZE
+    )
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      MergeRequest.where(state_id: nil),
+      MERGE_REQUESTS_MIGRATION,
+      DELAY_INTERVAL,
+      batch_size: BATCH_SIZE
+    )
   end
 
   def down

lib/gitlab/background_migration.rb

 # frozen_string_literal: true
 
-Dir[Rails.root.join("lib/gitlab/background_migration/helpers/*.rb")].each { |f| require f }
-
 module Gitlab
   module BackgroundMigration
     def self.queue

lib/gitlab/background_migration/delete_diff_files.rb

@@ -4,8 +4,6 @@
 module Gitlab
   module BackgroundMigration
     class DeleteDiffFiles
-      include Helpers::Reschedulable
-
       class MergeRequestDiff < ActiveRecord::Base
         self.table_name = 'merge_request_diffs'
 
@@ -17,24 +15,47 @@ module Gitlab
         self.table_name = 'merge_request_diff_files'
       end
 
+      DEAD_TUPLES_THRESHOLD = 50_000
+      VACUUM_WAIT_TIME = 5.minutes
+
       def perform(ids)
         @ids = ids
 
-        reschedule_if_needed(ids) do
-          prune_diff_files
+        # We should reschedule until deadtuples get in a desirable
+        # state (e.g. < 50_000). That may take more than one reschedule.
+        #
+        if should_wait_deadtuple_vacuum?
+          reschedule
+          return
         end
+
+        prune_diff_files
+      end
+
+      def should_wait_deadtuple_vacuum?
+        return false unless Gitlab::Database.postgresql?
+
+        diff_files_dead_tuples_count >= DEAD_TUPLES_THRESHOLD
       end
 
       private
 
-      def should_reschedule?
-        wait_for_deadtuple_vacuum?(MergeRequestDiffFile.table_name)
+      def reschedule
+        BackgroundMigrationWorker.perform_in(VACUUM_WAIT_TIME, self.class.name.demodulize, [@ids])
       end
 
       def diffs_collection
         MergeRequestDiff.where(id: @ids)
       end
 
+      def diff_files_dead_tuples_count
+        dead_tuple =
+          execute_statement("SELECT n_dead_tup FROM pg_stat_all_tables "\
+                            "WHERE relname = 'merge_request_diff_files'")[0]
+
+        dead_tuple&.fetch('n_dead_tup', 0).to_i
+      end
+
       def prune_diff_files
         removed = 0
         updated = 0
@@ -48,6 +69,10 @@ module Gitlab
                  "updated #{updated} merge_request_diffs rows")
       end
 
+      def execute_statement(sql)
+        ActiveRecord::Base.connection.execute(sql)
+      end
+
       def log_info(message)
         Rails.logger.info("BackgroundMigration::DeleteDiffFiles - #{message}")
       end

lib/gitlab/background_migration/helpers/reschedulable.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module BackgroundMigration
-    module Helpers
-      # == Reschedulable helper
-      #
-      # Allows background migrations to be rescheduled if a condition is met,
-      # the condition should be overridden in classes in #should_reschedule? method.
-      #
-      # For example, check DeleteDiffFiles migration which is rescheduled if dead tuple count
-      # on DB is not acceptable.
-      #
-      module Reschedulable
-        extend ActiveSupport::Concern
-
-        # Use this method to perform the background migration and it will be rescheduled
-        # if #should_reschedule? returns true.
-        def reschedule_if_needed(*args, &block)
-          if should_reschedule?
-            BackgroundMigrationWorker.perform_in(wait_time, self.class.name.demodulize, args)
-          else
-            yield
-          end
-        end
-
-        # Override this on base class if you need a different reschedule condition
-        def should_reschedule?
-          raise NotImplementedError, "#{self.class} does not implement #{__method__}"
-        end
-
-        # Override in subclass if a different dead tuple threshold
-        def dead_tuples_threshold
-          @dead_tuples_threshold ||= 50_000
-        end
-
-        # Override in subclass if a different wait time
-        def wait_time
-          @wait_time ||= 5.minutes
-        end
-
-        def execute_statement(sql)
-          ActiveRecord::Base.connection.execute(sql)
-        end
-
-        def wait_for_deadtuple_vacuum?(table_name)
-          return false unless Gitlab::Database.postgresql?
-
-          dead_tuples_count_for(table_name) >= dead_tuples_threshold
-        end
-
-        def dead_tuples_count_for(table_name)
-          dead_tuple =
-            execute_statement("SELECT n_dead_tup FROM pg_stat_all_tables "\
-                              "WHERE relname = '#{table_name}'")[0]
-
-          dead_tuple&.fetch('n_dead_tup', 0).to_i
-        end
-      end
-    end
-  end
-end

lib/gitlab/background_migration/reschedulable.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # == Reschedulable helper
+    #
+    # Allows background migrations to be rescheduled if a condition is met,
+    # the condition should be overridden in classes in #should_reschedule? method.
+    #
+    # For example, check DeleteDiffFiles migration which is rescheduled if dead tuple count
+    # on DB is not acceptable.
+    #
+    module Reschedulable
+      extend ActiveSupport::Concern
+
+      # Use this method to perform the background migration and it will be rescheduled
+      # if #should_reschedule? returns true.
+      def reschedule_if_needed(*args, &block)
+        if should_reschedule?
+          BackgroundMigrationWorker.perform_in(wait_time, self.class.name.demodulize, args)
+        else
+          yield
+        end
+      end
+
+      # Override this on base class if you need a different reschedule condition
+      def should_reschedule?
+        raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+      end
+
+      # Override in subclass if a different dead tuple threshold
+      def dead_tuples_threshold
+        @dead_tuples_threshold ||= 50_000
+      end
+
+      # Override in subclass if a different wait time
+      def wait_time
+        @wait_time ||= 5.minutes
+      end
+
+      def execute_statement(sql)
+        ActiveRecord::Base.connection.execute(sql)
+      end
+
+      def wait_for_deadtuple_vacuum?(table_name)
+        return false unless Gitlab::Database.postgresql?
+
+        dead_tuples_count_for(table_name) >= dead_tuples_threshold
+      end
+
+      def dead_tuples_count_for(table_name)
+        dead_tuple =
+          execute_statement("SELECT n_dead_tup FROM pg_stat_all_tables "\
+                            "WHERE relname = '#{table_name}'")[0]
+
+        dead_tuple&.fetch('n_dead_tup', 0).to_i
+      end
+    end
+  end
+end

lib/gitlab/background_migration/sync_issues_state_id.rb

@@ -4,7 +4,7 @@
 module Gitlab
   module BackgroundMigration
     class SyncIssuesStateId
-      include Helpers::Reschedulable
+      include Reschedulable
 
       def perform(start_id, end_id)
         Rails.logger.info("Issues - Populating state_id: #{start_id} - #{end_id}")

lib/gitlab/background_migration/sync_merge_requests_state_id.rb

@@ -4,7 +4,7 @@
 module Gitlab
   module BackgroundMigration
     class SyncMergeRequestsStateId
-      include Helpers::Reschedulable
+      include Reschedulable
 
       def perform(start_id, end_id)
         Rails.logger.info("Merge Requests - Populating state_id: #{start_id} - #{end_id}")

spec/lib/gitlab/background_migration/delete_diff_files_spec.rb

@@ -40,14 +40,14 @@ describe Gitlab::BackgroundMigration::DeleteDiffFiles, :migration, :sidekiq, sch
       end
     end
 
-    it 'reschedules itself when should wait for dead tuple vacuum' do
+    it 'reschedules itself when should_wait_deadtuple_vacuum' do
       merge_request = create(:merge_request, :merged)
       first_diff = merge_request.merge_request_diff
       second_diff = merge_request.create_merge_request_diff
 
       Sidekiq::Testing.fake! do
         worker = described_class.new
-        allow(worker).to receive(:wait_for_deadtuple_vacuum?) { true }
+        allow(worker).to receive(:should_wait_deadtuple_vacuum?) { true }
 
         worker.perform([first_diff.id, second_diff.id])
 
@@ -57,10 +57,10 @@ describe Gitlab::BackgroundMigration::DeleteDiffFiles, :migration, :sidekiq, sch
     end
   end
 
-  describe '#wait_for_deadtuple_vacuum?' do
+  describe '#should_wait_deadtuple_vacuum?' do
     it 'returns true when hitting merge_request_diff_files hits DEAD_TUPLES_THRESHOLD', :postgresql do
       worker = described_class.new
-      threshold_query_result = [{ "n_dead_tup" => 50_000 }]
+      threshold_query_result = [{ "n_dead_tup" => described_class::DEAD_TUPLES_THRESHOLD.to_s }]
       normal_query_result = [{ "n_dead_tup" => '3' }]
 
       allow(worker)
@@ -68,7 +68,7 @@ describe Gitlab::BackgroundMigration::DeleteDiffFiles, :migration, :sidekiq, sch
         .with(/SELECT n_dead_tup */)
         .and_return(threshold_query_result, normal_query_result)
 
-      expect(worker.wait_for_deadtuple_vacuum?('merge_request_diff_files')).to be(true)
+      expect(worker.should_wait_deadtuple_vacuum?).to be(true)
     end
   end
 end