summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2018-10-30 16:41:49 -0700
committerStan Hu <stanhu@gmail.com>2018-10-30 22:29:57 -0700
commitd4ef4ad752bf05758351bd0b8761566e40ab0e8e (patch)
tree8d8e3f67a217ad5b7d2dac4b0adb3f16d43f433e
parent571e651b21c7a618b8686a4b3f8a8c09c87a37f5 (diff)
downloadgitlab-ce-sh-optimize-merge-request-project-lookup.tar.gz
Reduce SQL queries needed to load open merge requestssh-optimize-merge-request-project-lookup
The SQL queries and memory allocation in MergeRequests::RefreshService is dominated by queries for Project and Route loads. On staging, the absence of an inverse relationship caused Rails to make over 1100 extraneous SQL queries for the www-gitlab-com repository. Relates to https://gitlab.com/gitlab-org/gitlab-ce/issues/49703
-rw-r--r--app/models/project.rb2
-rw-r--r--changelogs/unreleased/sh-optimize-merge-request-project-lookup.yml5
-rw-r--r--lib/gitlab/import/merge_request_helpers.rb2
-rw-r--r--spec/models/merge_request_spec.rb14
-rw-r--r--spec/models/project_spec.rb4
5 files changed, 25 insertions, 2 deletions
diff --git a/app/models/project.rb b/app/models/project.rb
index d593cbb223a..106cb92e0cc 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -181,7 +181,7 @@ class Project < ActiveRecord::Base
has_one :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
# Merge Requests for target project should be removed with it
- has_many :merge_requests, foreign_key: 'target_project_id'
+ has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project
has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest'
has_many :issues
has_many :labels, class_name: 'ProjectLabel'
diff --git a/changelogs/unreleased/sh-optimize-merge-request-project-lookup.yml b/changelogs/unreleased/sh-optimize-merge-request-project-lookup.yml
new file mode 100644
index 00000000000..241b89c4633
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-merge-request-project-lookup.yml
@@ -0,0 +1,5 @@
+---
+title: Reduce SQL queries needed to load open merge requests
+merge_request: 22709
+author:
+type: performance
diff --git a/lib/gitlab/import/merge_request_helpers.rb b/lib/gitlab/import/merge_request_helpers.rb
index 97dc1a987c4..9215067d973 100644
--- a/lib/gitlab/import/merge_request_helpers.rb
+++ b/lib/gitlab/import/merge_request_helpers.rb
@@ -22,7 +22,7 @@ module Gitlab
# additional work that is strictly necessary.
merge_request_id = insert_and_return_id(attributes, project.merge_requests)
- merge_request = project.merge_requests.find(merge_request_id)
+ merge_request = project.merge_requests.reload.find(merge_request_id)
# We use .insert_and_return_id which effectively disables all callbacks.
# Trigger iid logic here to make sure we track internal id values consistently.
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c8943f2d86f..85a4ebac66c 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -13,6 +13,20 @@ describe MergeRequest do
it { is_expected.to belong_to(:merge_user).class_name("User") }
it { is_expected.to belong_to(:assignee) }
it { is_expected.to have_many(:merge_request_diffs) }
+
+ context 'for forks' do
+ let!(:project) { create(:project) }
+ let!(:fork) { fork_project(project) }
+ let!(:merge_request) { create(:merge_request, target_project: project, source_project: fork) }
+
+ it 'does not load another project due to inverse relationship' do
+ expect(project.merge_requests.first.target_project.object_id).to eq(project.object_id)
+ end
+
+ it 'finds the associated merge request' do
+ expect(project.merge_requests.find(merge_request.id)).to eq(merge_request)
+ end
+ end
end
describe '#squash_in_progress?' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index e66838edd1a..83b3f308ec3 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -88,6 +88,10 @@ describe Project do
it { is_expected.to have_many(:project_deploy_tokens) }
it { is_expected.to have_many(:deploy_tokens).through(:project_deploy_tokens) }
+ it 'has an inverse relationship with merge requests' do
+ expect(described_class.reflect_on_association(:merge_requests).has_inverse?).to eq(:target_project)
+ end
+
context 'after initialized' do
it "has a project_feature" do
expect(described_class.new.project_feature).to be_present