From 10ceb33ba271f603fa09d4a4b5fdca03fd7ea333 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 3 Apr 2019 10:46:13 +0100 Subject: [PATCH] Extend CTE search optimisation to projects When we use the `search` param on an `IssuableFinder`, we can run into issues. We have trigram indexes to support these searches. On GitLab.com, we often see Postgres's optimiser prioritise the (global) trigram indexes over the index on `project_id`. For group and project searches, we know that it will be quicker to filter by `project_id` first, as it returns fewer rows in most cases. For group issues search, we ran into this issue previously, and went through the following iterations: 1. Use a CTE on the project IDs as an optimisation fence. This prevents the planner from disregarding the index on `project_id`. Unfortunately it breaks some types of sorting, like priority and popularity, as they sort on a joined table. 2. Use a subquery for listing issues, and a CTE for counts. The subquery - in the case of group lists - didn't help as much as the CTE, but was faster than not including it. We can safely use a CTE for counts as they don't have sorting. Now, however, we're seeing the same issue in a project context. The subquery doesn't help at all there (it would only return one row, after all). In an attempt to keep total code complexity under control, this commit removes the subquery optimisation and applies the CTE optimisation only for sorts we know that are safe. This means that for more complicated sorts (like priority and popularity), the search will continue to be very slow. If this is a high-priority issue, we can consider introducing further optimisations, but this finder is already very complicated and additional complexity has a cost. The group CTE optimisation is controlled by the same feature flag as before, `attempt_group_search_optimizations`, which is enabled by default. The new project CTE optimisation is controlled by a new feature flag, `attempt_project_search_optimizations`, which is disabled by default. --- .../concerns/issuable_collections.rb | 1 + app/finders/issuable_finder.rb | 46 ++++---- app/models/concerns/issuable.rb | 4 + app/models/concerns/sortable.rb | 28 ++--- .../extend-cte-optimisations-to-projects.yml | 5 + spec/finders/issues_finder_spec.rb | 105 ++++++------------ ...issuables_list_metadata_shared_examples.rb | 35 ++++-- 7 files changed, 105 insertions(+), 119 deletions(-) create mode 100644 changelogs/unreleased/extend-cte-optimisations-to-projects.yml 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,72 +743,7 @@ describe IssuesFinder do end it 'returns false' do - expect(finder.use_subquery_for_search?).to be_falsey - end - end - - context 'when the attempt_group_search_optimizations param is falsey' do - let(:params) { { search: 'foo' } } - - it 'returns false' do - expect(finder.use_subquery_for_search?).to be_falsey - end - end - - context 'when the attempt_group_search_optimizations flag is disabled' do - let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } - - before do - stub_feature_flags(attempt_group_search_optimizations: false) - end - - it 'returns false' do - expect(finder.use_subquery_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 - - 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 + expect(finder.use_cte_for_search?).to be_falsey end end @@ -816,27 +751,51 @@ describe IssuesFinder do let(:params) { { search: 'foo' } } it 'returns false' do - expect(finder.use_cte_for_count?).to be_falsey + expect(finder.use_cte_for_search?).to be_falsey 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 } } + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } before do stub_feature_flags(attempt_group_search_optimizations: false) end it 'returns false' do - expect(finder.use_cte_for_count?).to be_falsey + expect(finder.use_cte_for_search?).to be_falsey + 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 } } + + 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_search?).to be_falsey + end + end + + context 'and the corresponding feature flag is enabled' do + before do + stub_feature_flags(attempt_project_search_optimizations: true) + end + + 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')