Merge branch '54643-lower_issuable_finder_complexity' into 'master'

IssuableFinder - Always use CTE for group counts

Closes #54643

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

app/finders/issuable_finder.rb

@@ -78,13 +78,15 @@ class IssuableFinder
     items = init_collection
     items = filter_items(items)
 
-    # This has to be last as we may use a CTE as an optimization fence
-    # by passing the attempt_group_search_optimizations param and
-    # enabling the use_cte_for_group_issues_search feature flag
+    # This has to be last as we use a CTE as an optimization fence
+    # for counts by passing the force_cte param and enabling the
+    # attempt_group_search_optimizations feature flag
     # https://www.postgresql.org/docs/current/static/queries-with.html
     items = by_search(items)
 
-    sort(items)
+    items = sort(items) unless use_cte_for_count?
+
+    items
   end
 
   def filter_items(items)
@@ -117,8 +119,9 @@ class IssuableFinder
   #
   # rubocop: disable CodeReuse/ActiveRecord
   def count_by_state
-    count_params = params.merge(state: nil, sort: nil)
+    count_params = params.merge(state: nil, sort: nil, force_cte: true)
     finder = self.class.new(current_user, count_params)
+
     counts = Hash.new(0)
 
     # Searching by label includes a GROUP BY in the query, but ours will be last
@@ -128,8 +131,11 @@ 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.
     labels_count = label_names.any? ? label_names.count : 1
-    labels_count = 1 if use_cte_for_search?
+    labels_count = 1 if use_cte_for_count?
 
     finder.execute.reorder(nil).group(:state).count.each do |key, value|
       counts[count_key(key)] += value / labels_count
@@ -305,27 +311,31 @@ class IssuableFinder
 
   def use_subquery_for_search?
     strong_memoize(:use_subquery_for_search) do
-      attempt_group_search_optimizations? &&
-        Feature.enabled?(:use_subquery_for_group_issues_search, default_enabled: true)
+      !force_cte? && attempt_group_search_optimizations?
     end
   end
 
-  def use_cte_for_search?
-    strong_memoize(:use_cte_for_search) do
-      attempt_group_search_optimizations? &&
-        !use_subquery_for_search? &&
-        Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true)
+  def use_cte_for_count?
+    strong_memoize(:use_cte_for_count) do
+      force_cte? && attempt_group_search_optimizations?
     end
   end
 
   private
 
+  def force_cte?
+    !!params[:force_cte]
+  end
+
   def init_collection
     klass.all
   end
 
   def attempt_group_search_optimizations?
-    search && Gitlab::Database.postgresql? && params[:attempt_group_search_optimizations]
+    search &&
+      Gitlab::Database.postgresql? &&
+      params[:attempt_group_search_optimizations] &&
+      Feature.enabled?(:attempt_group_search_optimizations, default_enabled: true)
   end
 
   def count_key(value)
@@ -411,7 +421,7 @@ class IssuableFinder
   def by_search(items)
     return items unless search
 
-    if use_cte_for_search?
+    if use_cte_for_count?
       cte = Gitlab::SQL::RecursiveCTE.new(klass.table_name)
       cte << items
 

changelogs/unreleased/54643-lower_issuable_finder_complexity.yml

@@ -0,0 +1,5 @@
+---
+title: Speed up group issue search counts
+merge_request: 25411
+author:
+type: performance

spec/controllers/groups_controller_spec.rb

@@ -227,9 +227,7 @@ describe GroupsController do
 
     context 'searching' do
       before do
-        # Remove in https://gitlab.com/gitlab-org/gitlab-ce/issues/54643
-        stub_feature_flags(use_cte_for_group_issues_search: false)
-        stub_feature_flags(use_subquery_for_group_issues_search: true)
+        stub_feature_flags(attempt_group_search_optimizations: true)
       end
 
       it 'works with popularity sort' do

spec/finders/issues_finder_spec.rb

@@ -715,7 +715,7 @@ describe IssuesFinder do
 
     before do
       allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
-      stub_feature_flags(use_subquery_for_group_issues_search: true)
+      stub_feature_flags(attempt_group_search_optimizations: true)
     end
 
     context 'when there is no search param' do
@@ -746,11 +746,11 @@ describe IssuesFinder do
       end
     end
 
-    context 'when the use_subquery_for_group_issues_search flag is disabled' do
+    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(use_subquery_for_group_issues_search: false)
+        stub_feature_flags(attempt_group_search_optimizations: false)
       end
 
       it 'returns false' do
@@ -758,6 +758,14 @@ describe IssuesFinder do
       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 } }
 
@@ -767,72 +775,59 @@ describe IssuesFinder do
     end
   end
 
-  describe '#use_cte_for_search?' do
+  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(use_cte_for_group_issues_search: true)
-      stub_feature_flags(use_subquery_for_group_issues_search: false)
+      stub_feature_flags(attempt_group_search_optimizations: true)
     end
 
     context 'when there is no search param' do
-      let(:params) { { attempt_group_search_optimizations: true } }
+      let(:params) { { attempt_group_search_optimizations: true, force_cte: true } }
 
       it 'returns false' do
-        expect(finder.use_cte_for_search?).to be_falsey
+        expect(finder.use_cte_for_count?).to be_falsey
       end
     end
 
     context 'when the database is not Postgres' do
-      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+      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_search?).to be_falsey
+        expect(finder.use_cte_for_count?).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_cte_for_search?).to be_falsey
+        expect(finder.use_cte_for_count?).to be_falsey
       end
     end
 
-    context 'when the use_cte_for_group_issues_search flag is disabled' do
-      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+    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(use_cte_for_group_issues_search: false)
+        stub_feature_flags(attempt_group_search_optimizations: false)
       end
 
       it 'returns false' do
-        expect(finder.use_cte_for_search?).to be_falsey
-      end
-    end
-
-    context 'when use_subquery_for_search? is true' do
-      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
-
-      before do
-        stub_feature_flags(use_subquery_for_group_issues_search: true)
-      end
-
-      it 'returns false' do
-        expect(finder.use_cte_for_search?).to be_falsey
+        expect(finder.use_cte_for_count?).to be_falsey
       end
     end
 
     context 'when all conditions are met' do
-      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
+      let(:params) { { search: 'foo', force_cte: true, attempt_group_search_optimizations: true } }
 
       it 'returns true' do
-        expect(finder.use_cte_for_search?).to be_truthy
+        expect(finder.use_cte_for_count?).to be_truthy
       end
     end
   end