From 57e6a98ce41363150d24b96ee53c748189b936b4 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 5 Jun 2018 17:02:04 +0100 Subject: [PATCH 1/2] Simplify issuable finder queries We had `item_project_ids` to help make slow queries on the dashboard faster, but this isn't necessary any more - the queries are plenty fast, and we forbid searching the dashboard without filters. --- app/controllers/concerns/issuable_collections.rb | 6 ------ app/finders/issuable_finder.rb | 5 +---- app/finders/issues_finder.rb | 4 ---- app/finders/merge_requests_finder.rb | 4 ---- 4 files changed, 1 insertion(+), 18 deletions(-) diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index ca1b80a36a0..98a8939a4d0 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -95,12 +95,6 @@ module IssuableCollections elsif @group @filter_params[:group_id] = @group.id @filter_params[:include_subgroups] = true - else - # TODO: this filter ignore issues/mr created in public or - # internal repos where you are not a member. Enable this filter - # or improve current implementation to filter only issues you - # created or assigned or mentioned - # @filter_params[:authorized_only] = true end @filter_params.permit(finder_type.valid_params) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index c6ef79ce15e..3649e0fe560 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -159,10 +159,7 @@ class IssuableFinder finder_options = { include_subgroups: params[:include_subgroups], only_owned: true } GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute else - opts = { current_user: current_user } - opts[:project_ids_relation] = item_project_ids(items) if items - - ProjectsFinder.new(opts).execute + ProjectsFinder.new(current_user: current_user).execute end @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil) diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 3626670d141..24a6b9349a0 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -136,8 +136,4 @@ class IssuesFinder < IssuableFinder items end end - - def item_project_ids(items) - items&.reorder(nil)&.select(:project_id) - end end diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb index e2240e5e0d8..8d84ed4bdfb 100644 --- a/app/finders/merge_requests_finder.rb +++ b/app/finders/merge_requests_finder.rb @@ -56,8 +56,4 @@ class MergeRequestsFinder < IssuableFinder items.where(target_branch: target_branch) end - - def item_project_ids(items) - items&.reorder(nil)&.select(:target_project_id) - end end From c03386c3914feca56802e6f99bbd0fd08d269472 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 5 Jun 2018 18:31:32 +0100 Subject: [PATCH 2/2] Force Postgres to avoid trigram indexes when in a group When filtering issues with a search string in a group, we observed on GitLab.com that Postgres was using an inefficient query plan, preferring the (global) trigram indexes on description and title, rather than using a filter on the restricted set of issues within the group. Change the callers of the IssuableFinder to use a CTE in this case to fence the rest of the query from the LIKE filters, so that the optimiser is forced to perform the filter in the order we prefer. This will only force the use of a CTE when: 1. The use_cte_for_search params is truthy. 2. We are using Postgres. 3. We have passed the `search` param. The third item is important - searching issues using the search box does not use the finder in this way, but contructs a query and appends `full_search` to that. For some reason, this query does not suffer from the same issue. Currenly, we only pass this param when filtering issuables (issues or MRs) in a group context. --- .../concerns/issuable_collections.rb | 1 + app/finders/issuable_finder.rb | 34 ++++++++++++++++--- .../46648-timeout-searching-group-issues.yml | 5 +++ 3 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/46648-timeout-searching-group-issues.yml diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 98a8939a4d0..2ef2ee76855 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -95,6 +95,7 @@ module IssuableCollections elsif @group @filter_params[:group_id] = @group.id @filter_params[:include_subgroups] = true + @filter_params[:use_cte_for_search] = true end @filter_params.permit(finder_type.valid_params) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 3649e0fe560..5d5f72c4d86 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -23,6 +23,7 @@ # created_before: datetime # updated_after: datetime # updated_before: datetime +# use_cte_for_search: boolean # class IssuableFinder prepend FinderWithCrossProjectAccess @@ -54,6 +55,7 @@ class IssuableFinder sort state include_subgroups + use_cte_for_search ] end @@ -74,19 +76,21 @@ class IssuableFinder items = init_collection items = filter_items(items) - # Filtering by project HAS TO be the last because we use the project IDs yielded by the issuable query thus far - items = by_project(items) + # This has to be last as we may use a CTE as an optimization fence by + # passing the use_cte_for_search param + # https://www.postgresql.org/docs/current/static/queries-with.html + items = by_search(items) sort(items) end def filter_items(items) + items = by_project(items) items = by_scope(items) items = by_created_at(items) items = by_updated_at(items) items = by_state(items) items = by_group(items) - items = by_search(items) items = by_assignee(items) items = by_author(items) items = by_non_archived(items) @@ -107,7 +111,6 @@ class IssuableFinder # def count_by_state count_params = params.merge(state: nil, sort: nil) - labels_count = label_names.any? ? label_names.count : 1 finder = self.class.new(current_user, count_params) counts = Hash.new(0) @@ -116,6 +119,11 @@ class IssuableFinder # per issuable, so we have to count those in Ruby - which is bad, but still # better than performing multiple queries. # + # 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. + labels_count = label_names.any? ? label_names.count : 1 + labels_count = 1 if use_cte_for_search? + finder.execute.reorder(nil).group(:state).count.each do |key, value| counts[Array(key).last.to_sym] += value / labels_count end @@ -326,8 +334,24 @@ class IssuableFinder items end + def use_cte_for_search? + return false unless search + return false unless Gitlab::Database.postgresql? + + params[:use_cte_for_search] + end + def by_search(items) - search ? items.full_search(search) : items + return items unless search + + if use_cte_for_search? + cte = Gitlab::SQL::RecursiveCTE.new(klass.table_name) + cte << items + + items = klass.with(cte.to_arel).from(klass.table_name) + end + + items.full_search(search) end def by_iids(items) diff --git a/changelogs/unreleased/46648-timeout-searching-group-issues.yml b/changelogs/unreleased/46648-timeout-searching-group-issues.yml new file mode 100644 index 00000000000..54401edf5cc --- /dev/null +++ b/changelogs/unreleased/46648-timeout-searching-group-issues.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance of group issues filtering on GitLab.com +merge_request: 19429 +author: +type: performance