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

4 files changed, 33 insertions, 10 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 3cb6913549d..0ba00d447e8 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -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
diff --git a/changelogs/unreleased/merge-request-notes-performance.yml b/changelogs/unreleased/merge-request-notes-performance.yml
new file mode 100644
index 00000000000..6cf7a5047df
--- /dev/null
+++ b/changelogs/unreleased/merge-request-notes-performance.yml
@@ -0,0 +1,5 @@
+---
+title: Use a UNION ALL for getting merge request notes
+merge_request:
+author:
+type: other
diff --git a/lib/gitlab/sql/union.rb b/lib/gitlab/sql/union.rb
index 222021e8802..f30c771837a 100644
--- a/lib/gitlab/sql/union.rb
+++ b/lib/gitlab/sql/union.rb
@@ -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
diff --git a/spec/lib/gitlab/sql/union_spec.rb b/spec/lib/gitlab/sql/union_spec.rb
index baf8f6644bf..8026fba9f0a 100644
--- a/spec/lib/gitlab/sql/union_spec.rb
+++ b/spec/lib/gitlab/sql/union_spec.rb
@@ -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