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.
parent b570f53d
......@@ -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
......
---
title: Speed up group issue search counts
merge_request: 25411
author:
type: performance
......@@ -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
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment