Merge branch 'speed-up-relative-positioning' into 'master'

Speed up setting of relative position

See merge request gitlab-org/gitlab-ce!23324

4 files changed, 49 insertions, 14 deletions

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