Always use CTE for IssuableFinder counts

Since the CTE is faster than a subquery and the only reason we're using
a subquery is that the CTE can't handle sorting by certain attributes,
let's use the CTE always (when the feature flag is enabled) when
counting, since we can ignore ordering if we just want a count of
results.

4 files changed, 56 insertions, 48 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 23af2e0521c..77ecf6fd2e2 100644
--- a/app/finders/issuable_finder.rb
+++ b/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)
@@ -116,8 +118,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
@@ -127,8 +130,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
@@ -304,27 +310,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)
@@ -403,7 +413,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
 
diff --git a/changelogs/unreleased/54643-lower_issuable_finder_complexity.yml b/changelogs/unreleased/54643-lower_issuable_finder_complexity.yml
new file mode 100644
index 00000000000..f7f8e4d0e1f
--- /dev/null
+++ b/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
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 7d87b33e503..21e5122c06b 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/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
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 34cb09942be..868caf748f4 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -659,7 +659,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
@@ -690,11 +690,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
@@ -702,6 +702,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 } }
 
@@ -711,72 +719,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
-      end
-    end
-
-    context 'when the use_cte_for_group_issues_search flag is disabled' do
-      let(:params) { { search: 'foo', attempt_group_search_optimizations: true } }
-
-      before do
-        stub_feature_flags(use_cte_for_group_issues_search: 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 use_subquery_for_search? is true' 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_subquery_for_group_issues_search: true)
+        stub_feature_flags(attempt_group_search_optimizations: 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 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