From eb15e4dfc2953925cb4d029d007608a8d39bf97e Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 23 Nov 2018 13:06:04 +0000
Subject: [PATCH] Speed up setting of relative position

1. When every issue has a relative position set, we don't need to
   perform any updates, or calculate the maximum position in the parent.
2. If we do need to calculate the maximum position in the parent, many
   parents (specifically, groups with lots of projects) leads to a slow
   query where only the index on issues.relative_position is used, not
   the index on issues.project_id. Adding the GROUP BY forces Postgres
   to use both indices.
---
 app/models/concerns/relative_positioning.rb   | 46 +++++++++++++------
 app/models/issue.rb                           |  4 ++
 .../speed-up-relative-positioning.yml         |  5 ++
 .../concerns/relative_positioning_spec.rb     |  8 ++++
 4 files changed, 49 insertions(+), 14 deletions(-)
 create mode 100644 changelogs/unreleased/speed-up-relative-positioning.yml

diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index 045bf392ac8..46d2c345758 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -14,10 +14,12 @@ module RelativePositioning
 
   class_methods do
     def move_to_end(objects)
-      parent_ids = objects.map(&:parent_ids).flatten.uniq
-      max_relative_position = in_parents(parent_ids).maximum(:relative_position) || START_POSITION
       objects = objects.reject(&:relative_position)
 
+      return if objects.empty?
+
+      max_relative_position = objects.first.max_relative_position
+
       self.transaction do
         objects.each do |object|
           relative_position = position_between(max_relative_position, MAX_POSITION)
@@ -55,22 +57,21 @@ module RelativePositioning
     end
   end
 
-  def min_relative_position
-    self.class.in_parents(parent_ids).minimum(:relative_position)
+  def min_relative_position(&block)
+    calculate_relative_position('MIN', &block)
   end
 
-  def max_relative_position
-    self.class.in_parents(parent_ids).maximum(:relative_position)
+  def max_relative_position(&block)
+    calculate_relative_position('MAX', &block)
   end
 
   def prev_relative_position
     prev_pos = nil
 
     if self.relative_position
-      prev_pos = self.class
-        .in_parents(parent_ids)
-        .where('relative_position < ?', self.relative_position)
-        .maximum(:relative_position)
+      prev_pos = max_relative_position do |relation|
+        relation.where('relative_position < ?', self.relative_position)
+      end
     end
 
     prev_pos
@@ -80,10 +81,9 @@ module RelativePositioning
     next_pos = nil
 
     if self.relative_position
-      next_pos = self.class
-        .in_parents(parent_ids)
-        .where('relative_position > ?', self.relative_position)
-        .minimum(:relative_position)
+      next_pos = min_relative_position do |relation|
+        relation.where('relative_position > ?', self.relative_position)
+      end
     end
 
     next_pos
@@ -165,4 +165,22 @@ module RelativePositioning
     status
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
+
+  def calculate_relative_position(calculation)
+    # When calculating across projects, this is much more efficient than
+    # MAX(relative_position) without the GROUP BY, due to index usage:
+    # https://gitlab.com/gitlab-org/gitlab-ce/issues/54276#note_119340977
+    relation = self.class
+                 .in_parents(parent_ids)
+                 .order(Gitlab::Database.nulls_last_order('position', 'DESC'))
+                 .limit(1)
+                 .group(self.class.parent_column)
+
+    relation = yield relation if block_given?
+
+    relation
+      .pluck(self.class.parent_column, "#{calculation}(relative_position) AS position")
+      .first&.
+      last
+  end
 end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 0de5e434b02..780035c77e2 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -99,6 +99,10 @@ class Issue < ActiveRecord::Base
     alias_method :in_parents, :in_projects
   end
 
+  def self.parent_column
+    :project_id
+  end
+
   def self.reference_prefix
     '#'
   end
diff --git a/changelogs/unreleased/speed-up-relative-positioning.yml b/changelogs/unreleased/speed-up-relative-positioning.yml
new file mode 100644
index 00000000000..3bd865fb5de
--- /dev/null
+++ b/changelogs/unreleased/speed-up-relative-positioning.yml
@@ -0,0 +1,5 @@
+---
+title: Speed up issue board lists in groups with many projects
+merge_request:
+author:
+type: performance
diff --git a/spec/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb
index 66c1f47d12b..ac8da30b6c9 100644
--- a/spec/models/concerns/relative_positioning_spec.rb
+++ b/spec/models/concerns/relative_positioning_spec.rb
@@ -14,6 +14,14 @@ describe RelativePositioning do
       expect(issue.prev_relative_position).to eq nil
       expect(issue1.next_relative_position).to eq nil
     end
+
+    it 'does not perform any moves if all issues have their relative_position set' do
+      issue.update!(relative_position: 1)
+
+      expect(issue).not_to receive(:save)
+
+      Issue.move_to_end([issue])
+    end
   end
 
   describe '#max_relative_position' do