Commit c16b99a4 authored by Yorick Peterse's avatar Yorick Peterse

Use a UNION ALL for getting merge request notes

In this particular case the use of UNION ALL leads to a better query
plan compared to using 1 big query that uses an OR statement to combine
different data sources.

See https://gitlab.com/gitlab-org/gitlab-ce/issues/38508 for more
information.
parent 147c46cc
...@@ -552,14 +552,20 @@ class MergeRequest < ActiveRecord::Base ...@@ -552,14 +552,20 @@ class MergeRequest < ActiveRecord::Base
commits_for_notes_limit = 100 commits_for_notes_limit = 100
commit_ids = commit_shas.take(commits_for_notes_limit) commit_ids = commit_shas.take(commits_for_notes_limit)
Note.where( commit_notes = Note
"(project_id = :target_project_id AND noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR" + .except(:order)
"((project_id = :source_project_id OR project_id = :target_project_id) AND noteable_type = 'Commit' AND commit_id IN (:commit_ids))", .where(project_id: [source_project_id, target_project_id])
mr_id: id, .where(noteable_type: 'Commit', commit_id: commit_ids)
commit_ids: commit_ids,
target_project_id: target_project_id, # We're using a UNION ALL here since this results in better performance
source_project_id: source_project_id # 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 end
alias_method :discussion_notes, :related_notes alias_method :discussion_notes, :related_notes
......
---
title: Use a UNION ALL for getting merge request notes
merge_request:
author:
type: other
...@@ -12,8 +12,9 @@ module Gitlab ...@@ -12,8 +12,9 @@ module Gitlab
# #
# Project.where("id IN (#{sql})") # Project.where("id IN (#{sql})")
class Union class Union
def initialize(relations) def initialize(relations, remove_duplicates: true)
@relations = relations @relations = relations
@remove_duplicates = remove_duplicates
end end
def to_sql def to_sql
...@@ -25,7 +26,11 @@ module Gitlab ...@@ -25,7 +26,11 @@ module Gitlab
@relations.map { |rel| rel.reorder(nil).to_sql }.reject(&:blank?) @relations.map { |rel| rel.reorder(nil).to_sql }.reject(&:blank?)
end end
fragments.join("\nUNION\n") fragments.join("\n#{union_keyword}\n")
end
def union_keyword
@remove_duplicates ? 'UNION' : 'UNION ALL'
end end
end end
end end
......
...@@ -22,5 +22,12 @@ describe Gitlab::SQL::Union do ...@@ -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 {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)}") expect(union.to_sql).to eq("#{to_sql(relation_1)}\nUNION\n#{to_sql(relation_2)}")
end 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
end end
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