summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/finders/issuable_finder.rb40
-rw-r--r--app/models/ci/pipeline.rb4
-rw-r--r--app/models/concerns/has_ref.rb2
-rw-r--r--changelogs/unreleased/54643-lower_issuable_finder_complexity.yml5
-rw-r--r--spec/controllers/groups_controller_spec.rb4
-rw-r--r--spec/factories/merge_requests.rb9
-rw-r--r--spec/finders/issues_finder_spec.rb55
-rw-r--r--spec/models/ci/build_spec.rb116
-rw-r--r--spec/models/concerns/has_ref_spec.rb20
9 files changed, 202 insertions, 53 deletions
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 5870f158690..072d07e0ed2 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)
@@ -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
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index d4586219333..ca941ca4331 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -417,10 +417,6 @@ module Ci
@commit ||= Commit.lazy(project, sha)
end
- def branch?
- super && !merge_request?
- end
-
def stuck?
pending_builds.any?(&:stuck?)
end
diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb
index d7089294efc..44b06b4890c 100644
--- a/app/models/concerns/has_ref.rb
+++ b/app/models/concerns/has_ref.rb
@@ -4,7 +4,7 @@ module HasRef
extend ActiveSupport::Concern
def branch?
- !tag?
+ !tag? && !merge_request?
end
def git_ref
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/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 2392bfc4a53..0c0c5fc8a4c 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -101,6 +101,15 @@ FactoryBot.define do
end
end
+ trait :with_merge_request_pipeline do
+ after(:build) do |merge_request|
+ merge_request.merge_request_pipelines << build(:ci_pipeline,
+ source: :merge_request,
+ merge_request: merge_request,
+ project: merge_request.source_project)
+ end
+ end
+
trait :deployed_review_app do
target_branch 'pages-deploy-target'
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 47e2548c3d6..55efab7dec3 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/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
- 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
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 81ff727b458..e5281f3a09e 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2734,6 +2734,122 @@ describe Ci::Build do
end
end
+ describe '#secret_group_variables' do
+ subject { build.secret_group_variables }
+
+ let!(:variable) { create(:ci_group_variable, protected: true, group: group) }
+
+ context 'when ref is branch' do
+ let(:build) { create(:ci_build, ref: 'master', tag: false, project: project) }
+
+ context 'when ref is protected' do
+ before do
+ create(:protected_branch, :developers_can_merge, name: 'master', project: project)
+ end
+
+ it { is_expected.to include(variable) }
+ end
+
+ context 'when ref is not protected' do
+ it { is_expected.not_to include(variable) }
+ end
+ end
+
+ context 'when ref is tag' do
+ let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, project: project) }
+
+ context 'when ref is protected' do
+ before do
+ create(:protected_tag, project: project, name: 'v*')
+ end
+
+ it { is_expected.to include(variable) }
+ end
+
+ context 'when ref is not protected' do
+ it { is_expected.not_to include(variable) }
+ end
+ end
+
+ context 'when ref is merge request' do
+ let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
+ let(:pipeline) { merge_request.merge_request_pipelines.first }
+ let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
+
+ context 'when ref is protected' do
+ before do
+ create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project)
+ end
+
+ it 'does not return protected variables as it is not supported for merge request pipelines' do
+ is_expected.not_to include(variable)
+ end
+ end
+
+ context 'when ref is not protected' do
+ it { is_expected.not_to include(variable) }
+ end
+ end
+ end
+
+ describe '#secret_project_variables' do
+ subject { build.secret_project_variables }
+
+ let!(:variable) { create(:ci_variable, protected: true, project: project) }
+
+ context 'when ref is branch' do
+ let(:build) { create(:ci_build, ref: 'master', tag: false, project: project) }
+
+ context 'when ref is protected' do
+ before do
+ create(:protected_branch, :developers_can_merge, name: 'master', project: project)
+ end
+
+ it { is_expected.to include(variable) }
+ end
+
+ context 'when ref is not protected' do
+ it { is_expected.not_to include(variable) }
+ end
+ end
+
+ context 'when ref is tag' do
+ let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, project: project) }
+
+ context 'when ref is protected' do
+ before do
+ create(:protected_tag, project: project, name: 'v*')
+ end
+
+ it { is_expected.to include(variable) }
+ end
+
+ context 'when ref is not protected' do
+ it { is_expected.not_to include(variable) }
+ end
+ end
+
+ context 'when ref is merge request' do
+ let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
+ let(:pipeline) { merge_request.merge_request_pipelines.first }
+ let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
+
+ context 'when ref is protected' do
+ before do
+ create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project)
+ end
+
+ it 'does not return protected variables as it is not supported for merge request pipelines' do
+ is_expected.not_to include(variable)
+ end
+ end
+
+ context 'when ref is not protected' do
+ it { is_expected.not_to include(variable) }
+ end
+ end
+ end
+
describe '#scoped_variables_hash' do
context 'when overriding CI variables' do
before do
diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb
index 8aed72d77a4..8aa2fecb18c 100644
--- a/spec/models/concerns/has_ref_spec.rb
+++ b/spec/models/concerns/has_ref_spec.rb
@@ -16,6 +16,16 @@ describe HasRef do
it 'return true when tag is set to false' do
is_expected.to be_truthy
end
+
+ context 'when it was triggered by merge request' do
+ let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
+ let(:pipeline) { merge_request.merge_request_pipelines.first }
+ let(:build) { create(:ci_build, pipeline: pipeline) }
+
+ it 'returns false' do
+ is_expected.to be_falsy
+ end
+ end
end
context 'is not a tag' do
@@ -55,5 +65,15 @@ describe HasRef do
is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
end
end
+
+ context 'when it is triggered by a merge request' do
+ let(:merge_request) { create(:merge_request, :with_merge_request_pipeline) }
+ let(:pipeline) { merge_request.merge_request_pipelines.first }
+ let(:build) { create(:ci_build, tag: false, pipeline: pipeline) }
+
+ it 'returns nil' do
+ is_expected.to be_nil
+ end
+ end
end
end
View Lorry Controller Status