Merge branch 'extend-cte-optimisations-to-projects' into 'master'

Extend CTE search optimisation to projects

Closes #55170

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

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

app/finders/issuable_finder.rb

@@ -83,7 +83,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
@@ -91,7 +91,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)
@@ -131,10 +130,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
@@ -308,15 +309,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
 
@@ -331,12 +331,15 @@ class IssuableFinder
   end
 
   def attempt_group_search_optimizations?
-    search &&
-      Gitlab::Database.postgresql? &&
     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
@@ -407,20 +410,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
 

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

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
+      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

changelogs/unreleased/extend-cte-optimisations-to-projects.yml

+---
+title: Speed up filtering issues in a project when searching
+merge_request:
+author:
+type: performance

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
-
-  describe '#use_cte_for_count?' do
-    let(:finder) { described_class.new(nil, params) }
+    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
-      allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
-      stub_feature_flags(attempt_group_search_optimizations: true)
+          stub_feature_flags(attempt_project_search_optimizations: false)
         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
+          expect(finder.use_cte_for_search?).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 } }
-
+      context 'and the corresponding feature flag is enabled' do
         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' } }
-
-      it 'returns false' do
-        expect(finder.use_cte_for_count?).to be_falsey
-      end
+          stub_feature_flags(attempt_project_search_optimizations: true)
         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)
+        it 'returns true' do
+          expect(finder.use_cte_for_search?).to be_truthy
         end
-
-      it 'returns false' do
-        expect(finder.use_cte_for_count?).to be_falsey
       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

spec/support/shared_examples/issuables_list_metadata_shared_examples.rb

 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')