Change migration table scopes to use id column

Change the scopes that are used to fetch batched_background_migrations
and batched_background_migration_jobs to use the `id` column rather than
`created_at`. This saves an index one of the tables, and ensures we
don't need a tiebreaker for the sort.

db/migrate/20210128172149_create_background_migration_tracking_tables.rb

@@ -47,7 +47,7 @@ class CreateBackgroundMigrationTrackingTables < ActiveRecord::Migration[6.0]
       t.integer :status, limit: 2, null: false, default: 0
       t.integer :attempts, limit: 2, null: false, default: 0
 
-      t.index [:batched_background_migration_id, :created_at], name: :index_batched_jobs_by_batched_migration_id_created_at
+      t.index [:batched_background_migration_id, :id], name: :index_batched_jobs_by_batched_migration_id_and_id
     end
   end
 

db/structure.sql

@@ -21705,7 +21705,7 @@ CREATE INDEX index_badges_on_group_id ON badges USING btree (group_id);
 
 CREATE INDEX index_badges_on_project_id ON badges USING btree (project_id);
 
-CREATE INDEX index_batched_jobs_by_batched_migration_id_created_at ON batched_background_migration_jobs USING btree (batched_background_migration_id, created_at);
+CREATE INDEX index_batched_jobs_by_batched_migration_id_and_id ON batched_background_migration_jobs USING btree (batched_background_migration_id, id);
 
 CREATE INDEX index_batched_migrations_on_job_table_and_column_name ON batched_background_migrations USING btree (job_class_name, table_name, column_name);
 

lib/gitlab/database/background_migration/batched_migration.rb

@@ -7,11 +7,11 @@ module Gitlab
         self.table_name = :batched_background_migrations
 
         has_many :batched_jobs, foreign_key: :batched_background_migration_id
-        has_one :last_job, -> { order(created_at: :desc) },
+        has_one :last_job, -> { order(id: :desc) },
           class_name: 'Gitlab::Database::BackgroundMigration::BatchedJob',
           foreign_key: :batched_background_migration_id
 
-        scope :queue_order, -> { order(created_at: :asc) }
+        scope :queue_order, -> { order(id: :asc) }
 
         enum status: {
           paused: 0,

lib/gitlab/database/background_migration/batched_migration_wrapper.rb

@@ -5,21 +5,17 @@ module Gitlab
     module BackgroundMigration
       class BatchedMigrationWrapper
         def perform(batch_tracking_record)
-          return if batch_tracking_record.migration_aborted?
+          start_tracking_execution(batch_tracking_record)
 
-          begin
-            start_tracking_execution(batch_tracking_record)
+          execute_batch(batch_tracking_record)
 
-            execute_batch(batch_tracking_record)
+          batch_tracking_record.status = :succeeded
+        rescue => e
+          batch_tracking_record.status = :failed
 
-            batch_tracking_record.status = :succeeded
-          rescue => e
-            batch_tracking_record.status = :failed
-
-            raise e
-          ensure
-            finish_tracking_execution(batch_tracking_record)
-          end
+          raise e
+        ensure
+          finish_tracking_execution(batch_tracking_record)
         end
 
         private

spec/lib/gitlab/database/background_migration/batched_job_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d
     let(:batched_job) { build(:batched_background_migration_job) }
     let(:batched_migration) { batched_job.batched_migration }
 
-    describe '#migration_job_class' do
+    describe '#migration_aborted?' do
       before do
         batched_migration.status = :aborted
       end

spec/lib/gitlab/database/background_migration/batched_migration_spec.rb

@@ -11,21 +11,21 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m
     describe '#last_job' do
       let!(:batched_migration) { create(:batched_background_migration) }
       let!(:batched_job1) { create(:batched_background_migration_job, batched_migration: batched_migration) }
-      let!(:batched_job2) { create(:batched_background_migration_job, batched_migration: batched_migration, created_at: 1.month.ago) }
+      let!(:batched_job2) { create(:batched_background_migration_job, batched_migration: batched_migration) }
 
-      it 'returns the most recently created batch_job' do
-        expect(batched_migration.last_job).to eq(batched_job1)
+      it 'returns the most recent (in order of id) batched job' do
+        expect(batched_migration.last_job).to eq(batched_job2)
       end
     end
   end
 
   describe '.queue_order' do
-    let!(:migration1) { create(:batched_background_migration, created_at: 1.month.ago) }
-    let!(:migration2) { create(:batched_background_migration, created_at: 2.months.ago) }
+    let!(:migration1) { create(:batched_background_migration) }
+    let!(:migration2) { create(:batched_background_migration) }
     let!(:migration3) { create(:batched_background_migration) }
 
-    it 'returns batched_migrations ordered by created_at' do
-      expect(described_class.queue_order.all).to eq([migration2, migration1, migration3])
+    it 'returns batched migrations ordered by their id' do
+      expect(described_class.queue_order.all).to eq([migration1, migration2, migration3])
     end
   end
 

spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb

@@ -8,85 +8,62 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, '
 
   let_it_be(:active_migration) { create(:batched_background_migration, :active, job_arguments: [:id, :other_id]) }
 
-  context 'when the migration is aborted' do
-    let!(:aborted_migration) { create(:batched_background_migration, :aborted) }
-    let!(:job_record) { create(:batched_background_migration_job, batched_migration: aborted_migration) }
+  let!(:job_record) { create(:batched_background_migration_job, batched_migration: active_migration) }
 
-    it 'does not run the migration job instance' do
-      expect(job_record).not_to receive(:migration_job_class)
-      expect(job_class).not_to receive(:new)
-
-      migration_wrapper.perform(job_record)
+  it 'runs the migration job' do
+    expect_next_instance_of(job_class) do |job_instance|
+      expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, 'id', 'other_id')
     end
 
-    it 'does not update the tracking record in the database' do
+    migration_wrapper.perform(job_record)
+  end
+
+  it 'updates the the tracking record in the database' do
+    expect(job_record).to receive(:update!).with(hash_including(attempts: 1, status: :running)).and_call_original
+
+    freeze_time do
       migration_wrapper.perform(job_record)
 
       reloaded_job_record = job_record.reload
 
-      expect(reloaded_job_record).to be_pending
-      expect(reloaded_job_record.attempts).to eq(0)
+      expect(reloaded_job_record).not_to be_pending
+      expect(reloaded_job_record.attempts).to eq(1)
+      expect(reloaded_job_record.started_at).to eq(Time.current)
     end
   end
 
-  context 'when the migration is not aborted' do
-    let!(:job_record) { create(:batched_background_migration_job, batched_migration: active_migration) }
-
-    it 'runs the migration job' do
+  context 'when the migration job does not raise an error' do
+    it 'marks the tracking record as succeeded' do
       expect_next_instance_of(job_class) do |job_instance|
         expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, 'id', 'other_id')
       end
 
-      migration_wrapper.perform(job_record)
-    end
-
-    it 'updates the the tracking record in the database' do
-      expect(job_record).to receive(:update!).with(hash_including(attempts: 1, status: :running)).and_call_original
-
       freeze_time do
         migration_wrapper.perform(job_record)
 
         reloaded_job_record = job_record.reload
 
-        expect(reloaded_job_record).not_to be_pending
-        expect(reloaded_job_record.attempts).to eq(1)
-        expect(reloaded_job_record.started_at).to eq(Time.current)
+        expect(reloaded_job_record).to be_succeeded
+        expect(reloaded_job_record.finished_at).to eq(Time.current)
       end
     end
+  end
 
-    context 'when the migration job does not raise an error' do
-      it 'marks the tracking record as succeeded' do
-        expect_next_instance_of(job_class) do |job_instance|
-          expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, 'id', 'other_id')
-        end
-
-        freeze_time do
-          migration_wrapper.perform(job_record)
-
-          reloaded_job_record = job_record.reload
-
-          expect(reloaded_job_record).to be_succeeded
-          expect(reloaded_job_record.finished_at).to eq(Time.current)
-        end
+  context 'when the migration job raises an error' do
+    it 'marks the tracking record as failed before raising the error' do
+      expect_next_instance_of(job_class) do |job_instance|
+        expect(job_instance).to receive(:perform)
+          .with(1, 10, 'events', 'id', 1, 'id', 'other_id')
+          .and_raise(RuntimeError, 'Something broke!')
       end
-    end
-
-    context 'when the migration job raises an error' do
-      it 'marks the tracking record as failed before raising the error' do
-        expect_next_instance_of(job_class) do |job_instance|
-          expect(job_instance).to receive(:perform)
-            .with(1, 10, 'events', 'id', 1, 'id', 'other_id')
-            .and_raise(RuntimeError, 'Something broke!')
-        end
 
-        freeze_time do
-          expect { migration_wrapper.perform(job_record) }.to raise_error(RuntimeError, 'Something broke!')
+      freeze_time do
+        expect { migration_wrapper.perform(job_record) }.to raise_error(RuntimeError, 'Something broke!')
 
-          reloaded_job_record = job_record.reload
+        reloaded_job_record = job_record.reload
 
-          expect(reloaded_job_record).to be_failed
-          expect(reloaded_job_record.finished_at).to eq(Time.current)
-        end
+        expect(reloaded_job_record).to be_failed
+        expect(reloaded_job_record.finished_at).to eq(Time.current)
       end
     end
   end

spec/lib/gitlab/database/background_migration/scheduler_spec.rb

@@ -32,8 +32,8 @@ RSpec.describe Gitlab::Database::BackgroundMigration::Scheduler, '#perform' do
   end
 
   context 'when there are active migrations' do
-    let!(:last_migration) { create(:batched_background_migration, :active, created_at: 1.month.ago) }
-    let!(:first_migration) { create(:batched_background_migration, :active, batch_size: 2, created_at: 2.months.ago) }
+    let!(:first_migration) { create(:batched_background_migration, :active, batch_size: 2) }
+    let!(:last_migration) { create(:batched_background_migration, :active) }
 
     let(:job_relation) do
       Gitlab::Database::BackgroundMigration::BatchedJob.where(batched_background_migration_id: first_migration.id)