Merge branch 'merge-request-notes-performance' into 'master'
Use a UNION ALL for getting merge request notes Closes #38508 See merge request gitlab-org/gitlab-ce!14620
This commit is contained in:
commit
b1050bfe39
4 changed files with 33 additions and 10 deletions
|
@ -560,14 +560,20 @@ class MergeRequest < ActiveRecord::Base
|
|||
commits_for_notes_limit = 100
|
||||
commit_ids = commit_shas.take(commits_for_notes_limit)
|
||||
|
||||
Note.where(
|
||||
"(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" +
|
||||
"((project_id = :source_project_id OR project_id = :target_project_id) AND noteable_type = 'Commit' AND commit_id IN (:commit_ids))",
|
||||
mr_id: id,
|
||||
commit_ids: commit_ids,
|
||||
target_project_id: target_project_id,
|
||||
source_project_id: source_project_id
|
||||
)
|
||||
commit_notes = Note
|
||||
.except(:order)
|
||||
.where(project_id: [source_project_id, target_project_id])
|
||||
.where(noteable_type: 'Commit', commit_id: commit_ids)
|
||||
|
||||
# We're using a UNION ALL here since this results in better performance
|
||||
# compared to using OR statements. We're using UNION ALL since the queries
|
||||
# used won't produce any duplicates (e.g. a note for a commit can't also be
|
||||
# a note for an MR).
|
||||
union = Gitlab::SQL::Union
|
||||
.new([notes, commit_notes], remove_duplicates: false)
|
||||
.to_sql
|
||||
|
||||
Note.from("(#{union}) #{Note.table_name}")
|
||||
end
|
||||
|
||||
alias_method :discussion_notes, :related_notes
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Use a UNION ALL for getting merge request notes
|
||||
merge_request:
|
||||
author:
|
||||
type: other
|
|
@ -12,8 +12,9 @@ module Gitlab
|
|||
#
|
||||
# Project.where("id IN (#{sql})")
|
||||
class Union
|
||||
def initialize(relations)
|
||||
def initialize(relations, remove_duplicates: true)
|
||||
@relations = relations
|
||||
@remove_duplicates = remove_duplicates
|
||||
end
|
||||
|
||||
def to_sql
|
||||
|
@ -25,7 +26,11 @@ module Gitlab
|
|||
@relations.map { |rel| rel.reorder(nil).to_sql }.reject(&:blank?)
|
||||
end
|
||||
|
||||
fragments.join("\nUNION\n")
|
||||
fragments.join("\n#{union_keyword}\n")
|
||||
end
|
||||
|
||||
def union_keyword
|
||||
@remove_duplicates ? 'UNION' : 'UNION ALL'
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -22,5 +22,12 @@ describe Gitlab::SQL::Union do
|
|||
expect {User.where("users.id IN (#{union.to_sql})").to_a}.not_to raise_error
|
||||
expect(union.to_sql).to eq("#{to_sql(relation_1)}\nUNION\n#{to_sql(relation_2)}")
|
||||
end
|
||||
|
||||
it 'uses UNION ALL when removing duplicates is disabled' do
|
||||
union = described_class
|
||||
.new([relation_1, relation_2], remove_duplicates: false)
|
||||
|
||||
expect(union.to_sql).to include('UNION ALL')
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue