Merge branch 'jprovazn-fix-positioning' into 'master'
Optimize relative re-positioning See merge request gitlab-org/gitlab-ce!30938
This commit is contained in:
commit
ff76ca4494
5 changed files with 169 additions and 79 deletions
|
@ -29,10 +29,6 @@ module RelativePositioning
|
||||||
MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
|
MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
|
||||||
IDEAL_DISTANCE = 500
|
IDEAL_DISTANCE = 500
|
||||||
|
|
||||||
included do
|
|
||||||
after_save :save_positionable_neighbours
|
|
||||||
end
|
|
||||||
|
|
||||||
class_methods do
|
class_methods do
|
||||||
def move_nulls_to_end(objects)
|
def move_nulls_to_end(objects)
|
||||||
objects = objects.reject(&:relative_position)
|
objects = objects.reject(&:relative_position)
|
||||||
|
@ -114,11 +110,12 @@ module RelativePositioning
|
||||||
return move_after(before) unless after
|
return move_after(before) unless after
|
||||||
return move_before(after) unless before
|
return move_before(after) unless before
|
||||||
|
|
||||||
# If there is no place to insert an item we need to create one by moving the before item closer
|
# If there is no place to insert an item we need to create one by moving the item
|
||||||
# to its predecessor. This process will recursively move all the predecessors until we have a place
|
# before this and all preceding items until there is a gap
|
||||||
|
before, after = after, before if after.relative_position < before.relative_position
|
||||||
if (after.relative_position - before.relative_position) < 2
|
if (after.relative_position - before.relative_position) < 2
|
||||||
before.move_before
|
after.move_sequence_before
|
||||||
@positionable_neighbours = [before] # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
before.reset
|
||||||
end
|
end
|
||||||
|
|
||||||
self.relative_position = self.class.position_between(before.relative_position, after.relative_position)
|
self.relative_position = self.class.position_between(before.relative_position, after.relative_position)
|
||||||
|
@ -128,12 +125,8 @@ module RelativePositioning
|
||||||
pos_before = before.relative_position
|
pos_before = before.relative_position
|
||||||
pos_after = before.next_relative_position
|
pos_after = before.next_relative_position
|
||||||
|
|
||||||
if before.shift_after?
|
if pos_after && (pos_after - pos_before) < 2
|
||||||
item_to_move = self.class.relative_positioning_query_base(self).find_by!(relative_position: pos_after)
|
before.move_sequence_after
|
||||||
item_to_move.move_after
|
|
||||||
@positionable_neighbours = [item_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
|
||||||
|
|
||||||
pos_after = item_to_move.relative_position
|
|
||||||
end
|
end
|
||||||
|
|
||||||
self.relative_position = self.class.position_between(pos_before, pos_after)
|
self.relative_position = self.class.position_between(pos_before, pos_after)
|
||||||
|
@ -143,12 +136,8 @@ module RelativePositioning
|
||||||
pos_after = after.relative_position
|
pos_after = after.relative_position
|
||||||
pos_before = after.prev_relative_position
|
pos_before = after.prev_relative_position
|
||||||
|
|
||||||
if after.shift_before?
|
if pos_before && (pos_after - pos_before) < 2
|
||||||
item_to_move = self.class.relative_positioning_query_base(self).find_by!(relative_position: pos_before)
|
after.move_sequence_before
|
||||||
item_to_move.move_before
|
|
||||||
@positionable_neighbours = [item_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
|
|
||||||
|
|
||||||
pos_before = item_to_move.relative_position
|
|
||||||
end
|
end
|
||||||
|
|
||||||
self.relative_position = self.class.position_between(pos_before, pos_after)
|
self.relative_position = self.class.position_between(pos_before, pos_after)
|
||||||
|
@ -162,36 +151,82 @@ module RelativePositioning
|
||||||
self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION)
|
self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION)
|
||||||
end
|
end
|
||||||
|
|
||||||
# Indicates if there is an item that should be shifted to free the place
|
# Moves the sequence before the current item to the middle of the next gap
|
||||||
def shift_after?
|
# For example, we have 5 11 12 13 14 15 and the current item is 15
|
||||||
next_pos = next_relative_position
|
# This moves the sequence 11 12 13 14 to 8 9 10 11
|
||||||
next_pos && (next_pos - relative_position) == 1
|
def move_sequence_before
|
||||||
|
next_gap = find_next_gap_before
|
||||||
|
delta = optimum_delta_for_gap(next_gap)
|
||||||
|
|
||||||
|
move_sequence(next_gap[:start], relative_position, -delta)
|
||||||
end
|
end
|
||||||
|
|
||||||
# Indicates if there is an item that should be shifted to free the place
|
# Moves the sequence after the current item to the middle of the next gap
|
||||||
def shift_before?
|
# For example, we have 11 12 13 14 15 21 and the current item is 11
|
||||||
prev_pos = prev_relative_position
|
# This moves the sequence 12 13 14 15 to 15 16 17 18
|
||||||
prev_pos && (relative_position - prev_pos) == 1
|
def move_sequence_after
|
||||||
|
next_gap = find_next_gap_after
|
||||||
|
delta = optimum_delta_for_gap(next_gap)
|
||||||
|
|
||||||
|
move_sequence(relative_position, next_gap[:start], delta)
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
# rubocop:disable Gitlab/ModuleWithInstanceVariables
|
# Supposing that we have a sequence of items: 1 5 11 12 13 and the current item is 13
|
||||||
def save_positionable_neighbours
|
# This would return: `{ start: 11, end: 5 }`
|
||||||
return unless @positionable_neighbours
|
def find_next_gap_before
|
||||||
|
items_with_next_pos = scoped_items
|
||||||
|
.select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position DESC) AS next_pos')
|
||||||
|
.where('relative_position <= ?', relative_position)
|
||||||
|
.order(relative_position: :desc)
|
||||||
|
|
||||||
status = @positionable_neighbours.all? { |item| item.save(touch: false) }
|
find_next_gap(items_with_next_pos).tap do |gap|
|
||||||
@positionable_neighbours = nil
|
gap[:end] ||= MIN_POSITION
|
||||||
|
end
|
||||||
status
|
end
|
||||||
|
|
||||||
|
# Supposing that we have a sequence of items: 13 14 15 20 24 and the current item is 13
|
||||||
|
# This would return: `{ start: 15, end: 20 }`
|
||||||
|
def find_next_gap_after
|
||||||
|
items_with_next_pos = scoped_items
|
||||||
|
.select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position ASC) AS next_pos')
|
||||||
|
.where('relative_position >= ?', relative_position)
|
||||||
|
.order(:relative_position)
|
||||||
|
|
||||||
|
find_next_gap(items_with_next_pos).tap do |gap|
|
||||||
|
gap[:end] ||= MAX_POSITION
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
def find_next_gap(items_with_next_pos)
|
||||||
|
gap = self.class.from(items_with_next_pos, :items_with_next_pos)
|
||||||
|
.where('ABS(pos - next_pos) > 1 OR next_pos IS NULL')
|
||||||
|
.limit(1)
|
||||||
|
.pluck(:pos, :next_pos)
|
||||||
|
.first
|
||||||
|
|
||||||
|
{ start: gap[0], end: gap[1] }
|
||||||
|
end
|
||||||
|
|
||||||
|
def optimum_delta_for_gap(gap)
|
||||||
|
delta = ((gap[:start] - gap[:end]) / 2.0).abs.ceil
|
||||||
|
|
||||||
|
[delta, IDEAL_DISTANCE].min
|
||||||
|
end
|
||||||
|
|
||||||
|
def move_sequence(start_pos, end_pos, delta)
|
||||||
|
scoped_items
|
||||||
|
.where.not(id: self.id)
|
||||||
|
.where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
|
||||||
|
.update_all("relative_position = relative_position + #{delta}")
|
||||||
end
|
end
|
||||||
# rubocop:enable Gitlab/ModuleWithInstanceVariables
|
|
||||||
|
|
||||||
def calculate_relative_position(calculation)
|
def calculate_relative_position(calculation)
|
||||||
# When calculating across projects, this is much more efficient than
|
# When calculating across projects, this is much more efficient than
|
||||||
# MAX(relative_position) without the GROUP BY, due to index usage:
|
# MAX(relative_position) without the GROUP BY, due to index usage:
|
||||||
# https://gitlab.com/gitlab-org/gitlab-ce/issues/54276#note_119340977
|
# https://gitlab.com/gitlab-org/gitlab-ce/issues/54276#note_119340977
|
||||||
relation = self.class.relative_positioning_query_base(self)
|
relation = scoped_items
|
||||||
.order(Gitlab::Database.nulls_last_order('position', 'DESC'))
|
.order(Gitlab::Database.nulls_last_order('position', 'DESC'))
|
||||||
.group(self.class.relative_positioning_parent_column)
|
.group(self.class.relative_positioning_parent_column)
|
||||||
.limit(1)
|
.limit(1)
|
||||||
|
@ -203,4 +238,8 @@ module RelativePositioning
|
||||||
.first&.
|
.first&.
|
||||||
last
|
last
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def scoped_items
|
||||||
|
self.class.relative_positioning_query_base(self)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
5
changelogs/unreleased/jprovazn-fix-positioning.yml
Normal file
5
changelogs/unreleased/jprovazn-fix-positioning.yml
Normal file
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Optimize relative re-positioning when moving issues.
|
||||||
|
merge_request: 30938
|
||||||
|
author:
|
||||||
|
type: fixed
|
|
@ -0,0 +1,24 @@
|
||||||
|
# frozen_string_literal: true
|
||||||
|
|
||||||
|
class ReorderIssuesProjectIdRelativePositionIndex < ActiveRecord::Migration[5.2]
|
||||||
|
include Gitlab::Database::MigrationHelpers
|
||||||
|
|
||||||
|
DOWNTIME = false
|
||||||
|
|
||||||
|
disable_ddl_transaction!
|
||||||
|
|
||||||
|
OLD_INDEX_NAME = 'index_issues_on_project_id_and_state_and_rel_position_and_id'
|
||||||
|
NEW_INDEX_NAME = 'index_issues_on_project_id_and_rel_position_and_state_and_id'
|
||||||
|
|
||||||
|
def up
|
||||||
|
add_concurrent_index :issues, [:project_id, :relative_position, :state, :id], order: { id: :desc }, name: NEW_INDEX_NAME
|
||||||
|
|
||||||
|
remove_concurrent_index_by_name :issues, OLD_INDEX_NAME
|
||||||
|
end
|
||||||
|
|
||||||
|
def down
|
||||||
|
add_concurrent_index :issues, [:project_id, :state, :relative_position, :id], order: { id: :desc }, name: OLD_INDEX_NAME
|
||||||
|
|
||||||
|
remove_concurrent_index_by_name :issues, NEW_INDEX_NAME
|
||||||
|
end
|
||||||
|
end
|
|
@ -10,7 +10,7 @@
|
||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# It's strongly recommended that you check this file into your version control system.
|
||||||
|
|
||||||
ActiveRecord::Schema.define(version: 2019_08_01_114109) do
|
ActiveRecord::Schema.define(version: 2019_08_02_012622) do
|
||||||
|
|
||||||
# These are extensions that must be enabled in order to support this database
|
# These are extensions that must be enabled in order to support this database
|
||||||
enable_extension "pg_trgm"
|
enable_extension "pg_trgm"
|
||||||
|
@ -1716,7 +1716,7 @@ ActiveRecord::Schema.define(version: 2019_08_01_114109) do
|
||||||
t.index ["project_id", "created_at", "id", "state"], name: "index_issues_on_project_id_and_created_at_and_id_and_state"
|
t.index ["project_id", "created_at", "id", "state"], name: "index_issues_on_project_id_and_created_at_and_id_and_state"
|
||||||
t.index ["project_id", "due_date", "id", "state"], name: "idx_issues_on_project_id_and_due_date_and_id_and_state_partial", where: "(due_date IS NOT NULL)"
|
t.index ["project_id", "due_date", "id", "state"], name: "idx_issues_on_project_id_and_due_date_and_id_and_state_partial", where: "(due_date IS NOT NULL)"
|
||||||
t.index ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true
|
t.index ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true
|
||||||
t.index ["project_id", "state", "relative_position", "id"], name: "index_issues_on_project_id_and_state_and_rel_position_and_id", order: { id: :desc }
|
t.index ["project_id", "relative_position", "state", "id"], name: "index_issues_on_project_id_and_rel_position_and_state_and_id", order: { id: :desc }
|
||||||
t.index ["project_id", "updated_at", "id", "state"], name: "index_issues_on_project_id_and_updated_at_and_id_and_state"
|
t.index ["project_id", "updated_at", "id", "state"], name: "index_issues_on_project_id_and_updated_at_and_id_and_state"
|
||||||
t.index ["relative_position"], name: "index_issues_on_relative_position"
|
t.index ["relative_position"], name: "index_issues_on_relative_position"
|
||||||
t.index ["state"], name: "index_issues_on_state"
|
t.index ["state"], name: "index_issues_on_state"
|
||||||
|
|
|
@ -9,6 +9,12 @@ RSpec.shared_examples 'a class that supports relative positioning' do
|
||||||
create(factory, params.merge(default_params))
|
create(factory, params.merge(default_params))
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def create_items_with_positions(positions)
|
||||||
|
positions.map do |position|
|
||||||
|
create_item(relative_position: position)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '.move_nulls_to_end' do
|
describe '.move_nulls_to_end' do
|
||||||
it 'moves items with null relative_position to the end' do
|
it 'moves items with null relative_position to the end' do
|
||||||
skip("#{item1} has a default relative position") if item1.relative_position
|
skip("#{item1} has a default relative position") if item1.relative_position
|
||||||
|
@ -104,46 +110,6 @@ RSpec.shared_examples 'a class that supports relative positioning' do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '#shift_after?' do
|
|
||||||
before do
|
|
||||||
[item1, item2].each do |item1|
|
|
||||||
item1.move_to_end && item1.save
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
it 'returns true' do
|
|
||||||
item1.update(relative_position: item2.relative_position - 1)
|
|
||||||
|
|
||||||
expect(item1.shift_after?).to be_truthy
|
|
||||||
end
|
|
||||||
|
|
||||||
it 'returns false' do
|
|
||||||
item1.update(relative_position: item2.relative_position - 2)
|
|
||||||
|
|
||||||
expect(item1.shift_after?).to be_falsey
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
describe '#shift_before?' do
|
|
||||||
before do
|
|
||||||
[item1, item2].each do |item1|
|
|
||||||
item1.move_to_end && item1.save
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
it 'returns true' do
|
|
||||||
item1.update(relative_position: item2.relative_position + 1)
|
|
||||||
|
|
||||||
expect(item1.shift_before?).to be_truthy
|
|
||||||
end
|
|
||||||
|
|
||||||
it 'returns false' do
|
|
||||||
item1.update(relative_position: item2.relative_position + 2)
|
|
||||||
|
|
||||||
expect(item1.shift_before?).to be_falsey
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
describe '#move_between' do
|
describe '#move_between' do
|
||||||
before do
|
before do
|
||||||
[item1, item2].each do |item1|
|
[item1, item2].each do |item1|
|
||||||
|
@ -257,5 +223,61 @@ RSpec.shared_examples 'a class that supports relative positioning' do
|
||||||
|
|
||||||
expect(new_item.relative_position).to be(100)
|
expect(new_item.relative_position).to be(100)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'avoids N+1 queries when rebalancing other items' do
|
||||||
|
items = create_items_with_positions([100, 101, 102])
|
||||||
|
|
||||||
|
count = ActiveRecord::QueryRecorder.new do
|
||||||
|
new_item.move_between(items[-2], items[-1])
|
||||||
|
end
|
||||||
|
|
||||||
|
items = create_items_with_positions([150, 151, 152, 153, 154])
|
||||||
|
|
||||||
|
expect { new_item.move_between(items[-2], items[-1]) }.not_to exceed_query_limit(count)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe '#move_sequence_before' do
|
||||||
|
it 'moves the whole sequence of items to the middle of the nearest gap' do
|
||||||
|
items = create_items_with_positions([90, 100, 101, 102])
|
||||||
|
|
||||||
|
items.last.move_sequence_before
|
||||||
|
items.last.save!
|
||||||
|
|
||||||
|
positions = items.map { |item| item.reload.relative_position }
|
||||||
|
expect(positions).to eq([90, 95, 96, 102])
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'finds a gap if there are unused positions' do
|
||||||
|
items = create_items_with_positions([100, 101, 102])
|
||||||
|
|
||||||
|
items.last.move_sequence_before
|
||||||
|
items.last.save!
|
||||||
|
|
||||||
|
positions = items.map { |item| item.reload.relative_position }
|
||||||
|
expect(positions).to eq([50, 51, 102])
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe '#move_sequence_after' do
|
||||||
|
it 'moves the whole sequence of items to the middle of the nearest gap' do
|
||||||
|
items = create_items_with_positions([100, 101, 102, 110])
|
||||||
|
|
||||||
|
items.first.move_sequence_after
|
||||||
|
items.first.save!
|
||||||
|
|
||||||
|
positions = items.map { |item| item.reload.relative_position }
|
||||||
|
expect(positions).to eq([100, 105, 106, 110])
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'finds a gap if there are unused positions' do
|
||||||
|
items = create_items_with_positions([100, 101, 102])
|
||||||
|
|
||||||
|
items.first.move_sequence_after
|
||||||
|
items.first.save!
|
||||||
|
|
||||||
|
positions = items.map { |item| item.reload.relative_position }
|
||||||
|
expect(positions).to eq([100, 601, 602])
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in a new issue