summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/concerns/issuable_collections.rb1
-rw-r--r--app/finders/issuable_finder.rb46
-rw-r--r--app/models/concerns/issuable.rb4
-rw-r--r--app/models/concerns/sortable.rb28
-rw-r--r--changelogs/unreleased/extend-cte-optimisations-to-projects.yml5
-rw-r--r--spec/finders/issues_finder_spec.rb89
-rw-r--r--spec/support/shared_examples/issuables_list_metadata_shared_examples.rb35
7 files changed, 97 insertions, 111 deletions
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index c529aabf797..6d6e0cc6c7f 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -100,6 +100,7 @@ module IssuableCollections
if @project
options[:project_id] = @project.id
+ options[:attempt_project_search_optimizations] = true
elsif @group
options[:group_id] = @group.id
options[:include_subgroups] = true
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 6eab8c5ee51..fa9dda2ab31 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -84,7 +84,7 @@ class IssuableFinder
# https://www.postgresql.org/docs/current/static/queries-with.html
items = by_search(items)
- items = sort(items) unless use_cte_for_count?
+ items = sort(items)
items
end
@@ -92,7 +92,6 @@ class IssuableFinder
def filter_items(items)
items = by_project(items)
items = by_group(items)
- items = by_subquery(items)
items = by_scope(items)
items = by_created_at(items)
items = by_updated_at(items)
@@ -132,10 +131,12 @@ class IssuableFinder
# This does not apply when we are using a CTE for the search, as the labels
# GROUP BY is inside the subquery in that case, so we set labels_count to 1.
#
- # We always use CTE when searching in Groups if the feature flag is enabled,
- # but never when searching in Projects.
+ # Groups and projects have separate feature flags to suggest the use
+ # of a CTE. The CTE will not be used if the sort doesn't support it,
+ # but will always be used for the counts here as we ignore sorting
+ # anyway.
labels_count = label_names.any? ? label_names.count : 1
- labels_count = 1 if use_cte_for_count?
+ labels_count = 1 if use_cte_for_search?
finder.execute.reorder(nil).group(:state).count.each do |key, value|
counts[count_key(key)] += value / labels_count
@@ -309,15 +310,14 @@ class IssuableFinder
end
# rubocop: enable CodeReuse/ActiveRecord
- def use_subquery_for_search?
- strong_memoize(:use_subquery_for_search) do
- !force_cte? && attempt_group_search_optimizations?
- end
- end
+ def use_cte_for_search?
+ strong_memoize(:use_cte_for_search) do
+ next false unless search
+ next false unless Gitlab::Database.postgresql?
+ # Only simple unsorted & simple sorts can use CTE
+ next false if params[:sort].present? && !params[:sort].in?(klass.simple_sorts.keys)
- def use_cte_for_count?
- strong_memoize(:use_cte_for_count) do
- force_cte? && attempt_group_search_optimizations?
+ attempt_group_search_optimizations? || attempt_project_search_optimizations?
end
end
@@ -332,12 +332,15 @@ class IssuableFinder
end
def attempt_group_search_optimizations?
- search &&
- Gitlab::Database.postgresql? &&
- params[:attempt_group_search_optimizations] &&
+ params[:attempt_group_search_optimizations] &&
Feature.enabled?(:attempt_group_search_optimizations, default_enabled: true)
end
+ def attempt_project_search_optimizations?
+ params[:attempt_project_search_optimizations] &&
+ Feature.enabled?(:attempt_project_search_optimizations)
+ end
+
def count_key(value)
Array(value).last.to_sym
end
@@ -408,20 +411,11 @@ class IssuableFinder
end
# rubocop: enable CodeReuse/ActiveRecord
- # Wrap projects and groups in a subquery if the conditions are met.
- def by_subquery(items)
- if use_subquery_for_search?
- klass.where(id: items.select(:id)) # rubocop: disable CodeReuse/ActiveRecord
- else
- items
- end
- end
-
# rubocop: disable CodeReuse/ActiveRecord
def by_search(items)
return items unless search
- if use_cte_for_count?
+ if use_cte_for_search?
cte = Gitlab::SQL::RecursiveCTE.new(klass.table_name)
cte << items
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 51a8395c013..17f94b4bd9b 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -172,6 +172,10 @@ module Issuable
fuzzy_search(query, matched_columns)
end
+ def simple_sorts
+ super.except('name_asc', 'name_desc')
+ end
+
def sort_by_attribute(method, excluded_labels: [])
sorted =
case method.to_s
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index 29e48f0c5f7..df1a9e3fe6e 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -21,19 +21,21 @@ module Sortable
class_methods do
def order_by(method)
- case method.to_s
- when 'created_asc' then order_created_asc
- when 'created_date' then order_created_desc
- when 'created_desc' then order_created_desc
- when 'id_asc' then order_id_asc
- when 'id_desc' then order_id_desc
- when 'name_asc' then order_name_asc
- when 'name_desc' then order_name_desc
- when 'updated_asc' then order_updated_asc
- when 'updated_desc' then order_updated_desc
- else
- all
- end
+ simple_sorts.fetch(method.to_s, -> { all }).call
+ end
+
+ def simple_sorts
+ {
+ 'created_asc' => -> { order_created_asc },
+ 'created_date' => -> { order_created_desc },
+ 'created_desc' => -> { order_created_desc },
+ 'id_asc' => -> { order_id_asc },
+ 'id_desc' => -> { order_id_desc },
+ 'name_asc' => -> { order_name_asc },
+ 'name_desc' => -> { order_name_desc },
+ 'updated_asc' => -> { order_updated_asc },
+ 'updated_desc' => -> { order_updated_desc }
+ }
end
private
diff --git a/changelogs/unreleased/extend-cte-optimisations-to-projects.yml b/changelogs/unreleased/extend-cte-optimisations-to-projects.yml
new file mode 100644
index 00000000000..e5407127b2f
--- /dev/null
+++ b/changelogs/unreleased/extend-cte-optimisations-to-projects.yml
@@ -0,0 +1,5 @@
+---
+title: Speed up filtering issues in a project when searching
+merge_request:
+author:
+type: performance
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 00b6cad1a66..fe53fabe54c 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -719,7 +719,7 @@ describe IssuesFinder do
end
end
- describe '#use_subquery_for_search?' do
+ describe '#use_cte_for_search?' do
let(:finder) { described_class.new(nil, params) }
before do
@@ -731,7 +731,7 @@ describe IssuesFinder do
let(:params) { { attempt_group_search_optimizations: true } }
it 'returns false' do
- expect(finder.use_subquery_for_search?).to be_falsey
+ expect(finder.use_cte_for_search?).to be_falsey
end
end
@@ -743,15 +743,15 @@ describe IssuesFinder do
end
it 'returns false' do
- expect(finder.use_subquery_for_search?).to be_falsey
+ expect(finder.use_cte_for_search?).to be_falsey
end
end
- context 'when the attempt_group_search_optimizations param is falsey' do
+ context 'when the force_cte param is falsey' do
let(:params) { { search: 'foo' } }
it 'returns false' do
- expect(finder.use_subquery_for_search?).to be_falsey
+ expect(finder.use_cte_for_search?).to be_falsey
end
end
@@ -763,80 +763,39 @@ describe IssuesFinder do
end
it 'returns false' do
- expect(finder.use_subquery_for_search?).to be_falsey
+ expect(finder.use_cte_for_search?).to be_falsey
end
end
- context 'when force_cte? is true' do
- let(:params) { { search: 'foo', attempt_group_search_optimizations: true, force_cte: true } }
-
- it 'returns false' do
- expect(finder.use_subquery_for_search?).to be_falsey
- end
- end
-
- context 'when all conditions are met' do
- let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
-
- it 'returns true' do
- expect(finder.use_subquery_for_search?).to be_truthy
- end
- end
- end
+ context 'when attempt_group_search_optimizations is unset and attempt_project_search_optimizations is set' do
+ let(:params) { { search: 'foo', attempt_project_search_optimizations: true } }
- describe '#use_cte_for_count?' do
- let(:finder) { described_class.new(nil, params) }
-
- before do
- allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
- stub_feature_flags(attempt_group_search_optimizations: true)
- end
-
- context 'when there is no search param' do
- let(:params) { { attempt_group_search_optimizations: true, force_cte: true } }
-
- it 'returns false' do
- expect(finder.use_cte_for_count?).to be_falsey
- end
- end
-
- context 'when the database is not Postgres' do
- let(:params) { { search: 'foo', force_cte: true, attempt_group_search_optimizations: true } }
-
- before do
- allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
- end
-
- it 'returns false' do
- expect(finder.use_cte_for_count?).to be_falsey
- end
- end
-
- context 'when the force_cte param is falsey' do
- let(:params) { { search: 'foo' } }
+ context 'and the corresponding feature flag is disabled' do
+ before do
+ stub_feature_flags(attempt_project_search_optimizations: false)
+ end
- it 'returns false' do
- expect(finder.use_cte_for_count?).to be_falsey
+ it 'returns false' do
+ expect(finder.use_cte_for_search?).to be_falsey
+ end
end
- end
- context 'when the attempt_group_search_optimizations flag is disabled' do
- let(:params) { { search: 'foo', force_cte: true, attempt_group_search_optimizations: true } }
-
- before do
- stub_feature_flags(attempt_group_search_optimizations: false)
- end
+ context 'and the corresponding feature flag is enabled' do
+ before do
+ stub_feature_flags(attempt_project_search_optimizations: true)
+ end
- it 'returns false' do
- expect(finder.use_cte_for_count?).to be_falsey
+ it 'returns true' do
+ expect(finder.use_cte_for_search?).to be_truthy
+ end
end
end
context 'when all conditions are met' do
- let(:params) { { search: 'foo', force_cte: true, attempt_group_search_optimizations: true } }
+ let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
it 'returns true' do
- expect(finder.use_cte_for_count?).to be_truthy
+ expect(finder.use_cte_for_search?).to be_truthy
end
end
end
diff --git a/spec/support/shared_examples/issuables_list_metadata_shared_examples.rb b/spec/support/shared_examples/issuables_list_metadata_shared_examples.rb
index 90d67fd00fc..244f4766a84 100644
--- a/spec/support/shared_examples/issuables_list_metadata_shared_examples.rb
+++ b/spec/support/shared_examples/issuables_list_metadata_shared_examples.rb
@@ -1,11 +1,11 @@
shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
include ProjectForksHelper
- def get_action(action, project)
+ def get_action(action, project, extra_params = {})
if action
- get action, params: { author_id: project.creator.id }
+ get action, params: { author_id: project.creator.id }.merge(extra_params)
else
- get :index, params: { namespace_id: project.namespace, project_id: project }
+ get :index, params: { namespace_id: project.namespace, project_id: project }.merge(extra_params)
end
end
@@ -17,23 +17,44 @@ shared_examples 'issuables list meta-data' do |issuable_type, action = nil|
end
end
- before do
- @issuable_ids = %w[fix improve/awesome].map do |source_branch|
- create_issuable(issuable_type, project, source_branch: source_branch).id
+ let!(:issuables) do
+ %w[fix improve/awesome].map do |source_branch|
+ create_issuable(issuable_type, project, source_branch: source_branch)
end
end
+ let(:issuable_ids) { issuables.map(&:id) }
+
it "creates indexed meta-data object for issuable notes and votes count" do
get_action(action, project)
meta_data = assigns(:issuable_meta_data)
aggregate_failures do
- expect(meta_data.keys).to match_array(@issuable_ids)
+ expect(meta_data.keys).to match_array(issuables.map(&:id))
expect(meta_data.values).to all(be_kind_of(Issuable::IssuableMeta))
end
end
+ context 'searching' do
+ let(:result_issuable) { issuables.first }
+ let(:search) { result_issuable.title }
+
+ before do
+ stub_feature_flags(attempt_project_search_optimizations: true)
+ end
+
+ # .simple_sorts is the same across all Sortable classes
+ sorts = ::Issue.simple_sorts.keys + %w[popularity priority label_priority]
+ sorts.each do |sort|
+ it "works when sorting by #{sort}" do
+ get_action(action, project, search: search, sort: sort)
+
+ expect(assigns(:issuable_meta_data).keys).to include(result_issuable.id)
+ end
+ end
+ end
+
it "avoids N+1 queries" do
control = ActiveRecord::QueryRecorder.new { get_action(action, project) }
issuable = create_issuable(issuable_type, project, source_branch: 'csv')
View Lorry Controller Status