diff options
author | Sean McGivern <sean@gitlab.com> | 2018-11-29 12:52:48 +0000 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2018-11-30 11:57:56 +0000 |
commit | 7fd5dbf9db4e513faaabbe14aff3c10fca603415 (patch) | |
tree | de44c17a53b286b8230260cab6c499e85cc4cbb8 | |
parent | 938b891f89403ced0268699a6295531db508be17 (diff) | |
download | gitlab-ce-7fd5dbf9db4e513faaabbe14aff3c10fca603415.tar.gz |
Add a flag to use a subquery for group issues search
We already had a flag to use a CTE, but this broke searching in some
cases where we need to sort by a joined table. Disabling the CTE flag
makes searches much slower.
The new flag, to use a subquery, makes them slightly slower than the
CTE, while maintaining correctness. If both it and the CTE flag are
enabled, the subquery takes precedence.
-rw-r--r-- | app/controllers/concerns/issuable_collections.rb | 2 | ||||
-rw-r--r-- | app/finders/issuable_finder.rb | 47 | ||||
-rw-r--r-- | changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml | 5 | ||||
-rw-r--r-- | spec/controllers/groups_controller_spec.rb | 3 | ||||
-rw-r--r-- | spec/finders/issues_finder_spec.rb | 127 |
5 files changed, 173 insertions, 11 deletions
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 34a8c50fcbd..4aeec961f46 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -102,7 +102,7 @@ module IssuableCollections elsif @group options[:group_id] = @group.id options[:include_subgroups] = true - options[:use_cte_for_search] = true + options[:attempt_group_search_optimizations] = true end params.permit(finder_type.valid_params).merge(options) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index e04e3a2a7e0..247f2d373f1 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -27,12 +27,13 @@ # created_before: datetime # updated_after: datetime # updated_before: datetime -# use_cte_for_search: boolean +# attempt_group_search_optimizations: boolean # class IssuableFinder prepend FinderWithCrossProjectAccess include FinderMethods include CreatedAtFilter + include Gitlab::Utils::StrongMemoize requires_cross_project_access unless: -> { project? } @@ -75,8 +76,9 @@ 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 use_cte_for_search param + # 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 # https://www.postgresql.org/docs/current/static/queries-with.html items = by_search(items) @@ -85,6 +87,8 @@ 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) @@ -282,12 +286,36 @@ class IssuableFinder end # rubocop: enable CodeReuse/ActiveRecord + def use_subquery_for_search? + strong_memoize(:use_subquery_for_search) do + if attempt_group_search_optimizations? + Feature.enabled?(:use_subquery_for_group_issues_search, default_enabled: false) + else + false + end + end + end + + def use_cte_for_search? + strong_memoize(:use_cte_for_search) do + if attempt_group_search_optimizations? && !use_subquery_for_search? + Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true) + else + false + end + end + end + private def init_collection klass.all end + def attempt_group_search_optimizations? + search && Gitlab::Database.postgresql? && params[:attempt_group_search_optimizations] + end + def count_key(value) Array(value).last.to_sym end @@ -351,12 +379,13 @@ class IssuableFinder end # rubocop: enable CodeReuse/ActiveRecord - def use_cte_for_search? - return false unless search - return false unless Gitlab::Database.postgresql? - return false unless Feature.enabled?(:use_cte_for_group_issues_search, default_enabled: true) - - params[:use_cte_for_search] + # 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 diff --git a/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml b/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml new file mode 100644 index 00000000000..0269e7b6196 --- /dev/null +++ b/changelogs/unreleased/move-group-issues-search-cte-up-the-chain.yml @@ -0,0 +1,5 @@ +--- +title: Fix error when searching for group issues with priority or popularity sort +merge_request: 23445 +author: +type: fixed diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index f6c85102830..4b0dc4c9b69 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -226,9 +226,10 @@ describe GroupsController do end context 'searching' do - # Remove as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/52271 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) end it 'works with popularity sort' do diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 515f6f70b99..80f7232f282 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -640,4 +640,131 @@ describe IssuesFinder do end end end + + describe '#use_subquery_for_search?' do + let(:finder) { described_class.new(nil, params) } + + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + stub_feature_flags(use_subquery_for_group_issues_search: true) + end + + context 'when there is no search param' do + let(:params) { { attempt_group_search_optimizations: true } } + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when the database is not Postgres' do + let(:params) { { search: 'foo', attempt_group_search_optimizations: true } } + + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when the attempt_group_search_optimizations param is falsey' do + let(:params) { { search: 'foo' } } + + it 'returns false' do + expect(finder.use_subquery_for_search?).to be_falsey + end + end + + context 'when the use_subquery_for_group_issues_search 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) + end + + 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_search?' 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) + end + + context 'when there is no search param' do + let(:params) { { attempt_group_search_optimizations: true } } + + it 'returns false' do + expect(finder.use_cte_for_search?).to be_falsey + end + end + + context 'when the database is not Postgres' do + let(:params) { { search: 'foo', 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 + end + end + + context 'when the attempt_group_search_optimizations 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 + 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 + 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_cte_for_search?).to be_truthy + end + end + end end |