Commit 8a022e2b authored by Jan Provaznik's avatar Jan Provaznik
Browse files

Denormalize commits count for merge request diffs

For each MR diff an extra 'SELECT COUNT()' is executed
to get number of commits for the diff. Overall time to get counts for
all MR diffs may be quite expensive. To speed up loading of MR info,
information about number of commits is stored in a MR diff's extra column.

Closes #38068
parent 1baa7fb95f46
......@@ -49,6 +49,7 @@ def save_git_content
ensure_commit_shas
save_commits
save_diffs
save
keep_around_commits
end
......@@ -56,7 +57,6 @@ def ensure_commit_shas
self.start_commit_sha ||= merge_request.target_branch_sha
self.head_commit_sha ||= merge_request.source_branch_sha
self.base_commit_sha ||= find_base_sha
save
end
# Override head_commit_sha to keep compatibility with merge request diff
......@@ -195,7 +195,7 @@ def compare_with(sha)
end
def commits_count
merge_request_diff_commits.size
super || merge_request_diff_commits.size
end
private
......@@ -264,13 +264,16 @@ def save_diffs
new_attributes[:state] = :overflow if diff_collection.overflow?
end
update(new_attributes)
assign_attributes(new_attributes)
end
def save_commits
MergeRequestDiffCommit.create_bulk(self.id, compare.commits.reverse)
merge_request_diff_commits.reload
# merge_request_diff_commits.reload is preferred way to reload associated
# objects but it returns cached result for some reason in this case
commits = merge_request_diff_commits(true)
self.commits_count = commits.size
end
def repository
......
---
title: Store number of commits in merge_request_diffs table.
merge_request:
author:
type: performance
class AddCommitsCountToMergeRequestDiff < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
MIGRATION = 'AddMergeRequestDiffCommitsCount'.freeze
BATCH_SIZE = 5000
DELAY_INTERVAL = 5.minutes.to_i
class MergeRequestDiff < ActiveRecord::Base
self.table_name = 'merge_request_diffs'
include ::EachBatch
end
disable_ddl_transaction!
def up
add_column :merge_request_diffs, :commits_count, :integer
say 'Populating the MergeRequestDiff `commits_count`'
queue_background_migration_jobs_by_range_at_intervals(MergeRequestDiff, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
end
def down
remove_column :merge_request_diffs, :commits_count
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171230123729) do
ActiveRecord::Schema.define(version: 20180105212544) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -1044,6 +1044,7 @@
t.string "real_size"
t.string "head_commit_sha"
t.string "start_commit_sha"
t.integer "commits_count"
end
add_index "merge_request_diffs", ["merge_request_id", "id"], name: "index_merge_request_diffs_on_merge_request_id_and_id", using: :btree
......
# frozen_string_literal: true
# rubocop:disable Style/Documentation
# rubocop:disable Metrics/LineLength
module Gitlab
module BackgroundMigration
class AddMergeRequestDiffCommitsCount
class MergeRequestDiff < ActiveRecord::Base
self.table_name = 'merge_request_diffs'
end
def perform(start_id, stop_id)
Rails.logger.info("Setting commits_count for merge request diffs: #{start_id} - #{stop_id}")
update = '
commits_count = (
SELECT count(*)
FROM merge_request_diff_commits
WHERE merge_request_diffs.id = merge_request_diff_commits.merge_request_diff_id
)'.squish
MergeRequestDiff.where(id: start_id..stop_id).update_all(update)
end
end
end
end
require 'spec_helper'
describe Gitlab::BackgroundMigration::AddMergeRequestDiffCommitsCount, :migration, schema: 20180105212544 do
let(:projects_table) { table(:projects) }
let(:merge_requests_table) { table(:merge_requests) }
let(:merge_request_diffs_table) { table(:merge_request_diffs) }
let(:merge_request_diff_commits_table) { table(:merge_request_diff_commits) }
let(:project) { projects_table.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce') }
let(:merge_request) do
merge_requests_table.create!(target_project_id: project.id,
target_branch: 'master',
source_project_id: project.id,
source_branch: 'mr name',
title: 'mr name')
end
def create_diff!(name, commits: 0)
mr_diff = merge_request_diffs_table.create!(
merge_request_id: merge_request.id)
commits.times do |i|
merge_request_diff_commits_table.create!(
merge_request_diff_id: mr_diff.id,
relative_order: i, sha: i)
end
mr_diff
end
describe '#perform' do
it 'migrates diffs that have no commits' do
diff = create_diff!('with_multiple_commits', commits: 0)
subject.perform(diff.id, diff.id)
expect(diff.reload.commits_count).to eq(0)
end
it 'migrates multiple diffs to the correct values' do
diffs = Array.new(3).map.with_index { |_, i| create_diff!(i, commits: 3) }
subject.perform(diffs.first.id, diffs.last.id)
diffs.each do |diff|
expect(diff.reload.commits_count).to eq(3)
end
end
end
end
......@@ -10,6 +10,11 @@
let(:merge_request_diff) { MergeRequest.find(merge_request.id).create_merge_request_diff }
let(:updated_merge_request_diff) { MergeRequestDiff.find(merge_request_diff.id) }
before do
allow_any_instance_of(MergeRequestDiff)
.to receive(:commits_count=).and_return(nil)
end
def diffs_to_hashes(diffs)
diffs.as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS).map(&:with_indifferent_access)
end
......
require 'rails_helper'
describe Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData, :migration, schema: 20171128214150 do
# commits_count attribute is added in a next migration
before do
allow_any_instance_of(MergeRequestDiff)
.to receive(:commits_count=).and_return(nil)
end
describe '#perform' do
let(:mr_with_event) { create(:merge_request) }
let!(:merged_event) { create(:event, :merged, target: mr_with_event) }
......
......@@ -180,6 +180,7 @@ MergeRequestDiff:
- real_size
- head_commit_sha
- start_commit_sha
- commits_count
MergeRequestDiffCommit:
- merge_request_diff_id
- relative_order
......
......@@ -2,6 +2,12 @@
require Rails.root.join('db', 'post_migrate', '20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb')
describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq do
# commits_count attribute is added in a next migration
before do
allow_any_instance_of(MergeRequestDiff)
.to receive(:commits_count=).and_return(nil)
end
let!(:mrs) { create_list(:merge_request, 3) }
it 'correctly schedules background migrations' do
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment