Merge branch '37631-add-a-merge_request_diff_id-column-to-merge_requests' into 'master'

Resolve "Add a `merge_request_diff_id` column to `merge_requests`"

See merge request gitlab-org/gitlab-ce!15035

app/models/merge_request_diff.rb

@@ -48,6 +48,10 @@ class MergeRequestDiff < ActiveRecord::Base
   # Collect information about commits and diff from repository
   # and save it to the database as serialized data
   def save_git_content
+    MergeRequest
+      .where('id = ? AND COALESCE(latest_merge_request_diff_id, 0) < ?', self.merge_request_id, self.id)
+      .update_all(latest_merge_request_diff_id: self.id)
+
     ensure_commit_shas
     save_commits
     save_diffs

changelogs/unreleased/37631-add-a-merge_request_diff_id-column-to-merge_requests.yml

+---
+title: Add a latest_merge_request_diff_id column to merge_requests
+merge_request: 15035
+author:
+type: performance

db/migrate/20171025110159_add_latest_merge_request_diff_id_to_merge_requests.rb

+class AddLatestMergeRequestDiffIdToMergeRequests < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column :merge_requests, :latest_merge_request_diff_id, :integer
+    add_concurrent_index :merge_requests, :latest_merge_request_diff_id
+
+    add_concurrent_foreign_key :merge_requests, :merge_request_diffs,
+                               column: :latest_merge_request_diff_id,
+                               on_delete: :nullify
+  end
+
+  def down
+    remove_foreign_key :merge_requests, column: :latest_merge_request_diff_id
+
+    if index_exists?(:merge_requests, :latest_merge_request_diff_id)
+      remove_concurrent_index :merge_requests, :latest_merge_request_diff_id
+    end
+
+    remove_column :merge_requests, :latest_merge_request_diff_id
+  end
+end

db/post_migrate/20171026082505_populate_merge_requests_latest_merge_request_diff_id.rb

+class PopulateMergeRequestsLatestMergeRequestDiffId < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 1_000
+
+  class MergeRequest < ActiveRecord::Base
+    self.table_name = 'merge_requests'
+
+    include ::EachBatch
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    update = '
+      latest_merge_request_diff_id = (
+        SELECT MAX(id)
+        FROM merge_request_diffs
+        WHERE merge_requests.id = merge_request_diffs.merge_request_id
+      )'.squish
+
+    MergeRequest.where(latest_merge_request_diff_id: nil).each_batch(of: BATCH_SIZE) do |relation|
+      relation.update_all(update)
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20171017145932) do
+ActiveRecord::Schema.define(version: 20171026082505) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -973,6 +973,7 @@ ActiveRecord::Schema.define(version: 20171017145932) do
     t.boolean "ref_fetched"
     t.string "merge_jid"
     t.boolean "discussion_locked"
+    t.integer "latest_merge_request_diff_id"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
@@ -981,6 +982,7 @@ ActiveRecord::Schema.define(version: 20171017145932) do
   add_index "merge_requests", ["deleted_at"], name: "index_merge_requests_on_deleted_at", using: :btree
   add_index "merge_requests", ["description"], name: "index_merge_requests_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "merge_requests", ["head_pipeline_id"], name: "index_merge_requests_on_head_pipeline_id", using: :btree
+  add_index "merge_requests", ["latest_merge_request_diff_id"], name: "index_merge_requests_on_latest_merge_request_diff_id", using: :btree
   add_index "merge_requests", ["milestone_id"], name: "index_merge_requests_on_milestone_id", using: :btree
   add_index "merge_requests", ["source_branch"], name: "index_merge_requests_on_source_branch", using: :btree
   add_index "merge_requests", ["source_project_id", "source_branch"], name: "index_merge_requests_on_source_project_id_and_source_branch", using: :btree
@@ -1844,6 +1846,7 @@ ActiveRecord::Schema.define(version: 20171017145932) do
   add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
   add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify
+  add_foreign_key "merge_requests", "merge_request_diffs", column: "latest_merge_request_diff_id", name: "fk_06067f5644", on_delete: :nullify
   add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade
   add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade

lib/gitlab/import_export/import_export.yml

@@ -114,6 +114,7 @@ excluded_attributes:
     - :milestone_id
     - :ref_fetched
     - :merge_jid
+    - :latest_merge_request_diff_id
   award_emoji:
     - :awardable_id
   statuses:

spec/migrations/populate_merge_requests_latest_merge_request_diff_id_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20171026082505_populate_merge_requests_latest_merge_request_diff_id')
+
+describe PopulateMergeRequestsLatestMergeRequestDiffId, :migration do
+  let(:projects_table) { table(:projects) }
+  let(:merge_requests_table) { table(:merge_requests) }
+  let(:merge_request_diffs_table) { table(:merge_request_diffs) }
+
+  let(:project) { projects_table.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce') }
+
+  def create_mr!(name, diffs: 0)
+    merge_request =
+      merge_requests_table.create!(target_project_id: project.id,
+                                   target_branch: 'master',
+                                   source_project_id: project.id,
+                                   source_branch: name,
+                                   title: name)
+
+    diffs.times do
+      merge_request_diffs_table.create!(merge_request_id: merge_request.id)
+    end
+
+    merge_request
+  end
+
+  def diffs_for(merge_request)
+    merge_request_diffs_table.where(merge_request_id: merge_request.id)
+  end
+
+  describe '#up' do
+    it 'ignores MRs without diffs' do
+      merge_request_without_diff = create_mr!('without_diff')
+
+      expect(merge_request_without_diff.latest_merge_request_diff_id).to be_nil
+
+      expect { migrate! }
+        .not_to change { merge_request_without_diff.reload.latest_merge_request_diff_id }
+    end
+
+    it 'ignores MRs that have a diff ID already set' do
+      merge_request_with_multiple_diffs = create_mr!('with_multiple_diffs', diffs: 3)
+      diff_id = diffs_for(merge_request_with_multiple_diffs).minimum(:id)
+
+      merge_request_with_multiple_diffs.update!(latest_merge_request_diff_id: diff_id)
+
+      expect { migrate! }
+        .not_to change { merge_request_with_multiple_diffs.reload.latest_merge_request_diff_id }
+    end
+
+    it 'migrates multiple MR diffs to the correct values' do
+      merge_requests = Array.new(3).map.with_index { |_, i| create_mr!(i, diffs: 3) }
+
+      migrate!
+
+      merge_requests.each do |merge_request|
+        expect(merge_request.reload.latest_merge_request_diff_id)
+          .to eq(diffs_for(merge_request).maximum(:id))
+      end
+    end
+  end
+end