Commit 090ca9c3 authored by Jan Provaznik's avatar Jan Provaznik

Use limit for search count queries

Search query is especially slow if a user searches a generic string
which matches many records, in such case search can take tens of
seconds or time out. To speed up the search query, we search only for
first 1000 records, if there is >1000 matching records we just display
"1000+" instead of precise total count supposing that with such amount
the exact count is not so important for the user.

Because for issues even limited search was not fast enough, 2-phase
approach is used for issues: first we use simpler/faster query to get
all public issues, if this exceeds the limit, we just return the limit.
If the amount of matching results is lower than limit, we re-run more
complex search query (which includes also confidential issues).
Re-running the complex query should be fast enough in such case because the
amount of matching issues is lower than limit.

Because exact total_count is now limited, this patch also switches to
to "prev/next" pagination.

Related #40540
parent fbbd81ee
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
# label_name: string # label_name: string
# sort: string # sort: string
# my_reaction_emoji: string # my_reaction_emoji: string
# public_only: boolean
# #
class IssuesFinder < IssuableFinder class IssuesFinder < IssuableFinder
CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
...@@ -40,7 +41,15 @@ class IssuesFinder < IssuableFinder ...@@ -40,7 +41,15 @@ class IssuesFinder < IssuableFinder
private private
def init_collection def init_collection
with_confidentiality_access_check if public_only?
Issue.public_only
else
with_confidentiality_access_check
end
end
def public_only?
params.fetch(:public_only, false)
end end
def user_can_see_all_confidential_issues? def user_can_see_all_confidential_issues?
......
...@@ -170,4 +170,8 @@ module SearchHelper ...@@ -170,4 +170,8 @@ module SearchHelper
# Truncato's filtered_tags and filtered_attributes are not quite the same # Truncato's filtered_tags and filtered_attributes are not quite the same
sanitize(html, tags: %w(a p ol ul li pre code)) sanitize(html, tags: %w(a p ol ul li pre code))
end end
def limited_count(count, limit = 1000)
count > limit ? "#{limit}+" : count
end
end end
...@@ -57,25 +57,24 @@ ...@@ -57,25 +57,24 @@
Titles and Filenames Titles and Filenames
%span.badge %span.badge
= @search_results.snippet_titles_count = @search_results.snippet_titles_count
- else - else
%li{ class: active_when(@scope == 'projects') } %li{ class: active_when(@scope == 'projects') }
= link_to search_filter_path(scope: 'projects') do = link_to search_filter_path(scope: 'projects') do
Projects Projects
%span.badge %span.badge
= @search_results.projects_count = limited_count(@search_results.limited_projects_count)
%li{ class: active_when(@scope == 'issues') } %li{ class: active_when(@scope == 'issues') }
= link_to search_filter_path(scope: 'issues') do = link_to search_filter_path(scope: 'issues') do
Issues Issues
%span.badge %span.badge
= @search_results.issues_count = limited_count(@search_results.limited_issues_count)
%li{ class: active_when(@scope == 'merge_requests') } %li{ class: active_when(@scope == 'merge_requests') }
= link_to search_filter_path(scope: 'merge_requests') do = link_to search_filter_path(scope: 'merge_requests') do
Merge requests Merge requests
%span.badge %span.badge
= @search_results.merge_requests_count = limited_count(@search_results.limited_merge_requests_count)
%li{ class: active_when(@scope == 'milestones') } %li{ class: active_when(@scope == 'milestones') }
= link_to search_filter_path(scope: 'milestones') do = link_to search_filter_path(scope: 'milestones') do
Milestones Milestones
%span.badge %span.badge
= @search_results.milestones_count = limited_count(@search_results.limited_milestones_count)
...@@ -2,7 +2,8 @@ ...@@ -2,7 +2,8 @@
= render partial: "search/results/empty" = render partial: "search/results/empty"
- else - else
.row-content-block .row-content-block
= search_entries_info(@search_objects, @scope, @search_term) - unless @search_objects.is_a?(Kaminari::PaginatableWithoutCount)
= search_entries_info(@search_objects, @scope, @search_term)
- unless @show_snippets - unless @show_snippets
- if @project - if @project
in project #{link_to @project.name_with_namespace, [@project.namespace.becomes(Namespace), @project]} in project #{link_to @project.name_with_namespace, [@project.namespace.becomes(Namespace), @project]}
...@@ -22,4 +23,4 @@ ...@@ -22,4 +23,4 @@
= render partial: "search/results/#{@scope.singularize}", collection: @search_objects = render partial: "search/results/#{@scope.singularize}", collection: @search_objects
- if @scope != 'projects' - if @scope != 'projects'
= paginate(@search_objects, theme: 'gitlab') = paginate_collection(@search_objects)
---
title: Optimize search queries on the search page by setting a limit for matching records.
merge_request:
author:
type: performance
class AddIndexUpdatedAtToIssues < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :issues, :updated_at
end
def down
remove_concurrent_index :issues, :updated_at
end
end
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20180113220114) do ActiveRecord::Schema.define(version: 20180115201419) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -886,6 +886,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do ...@@ -886,6 +886,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do
add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree
add_index "issues", ["state"], name: "index_issues_on_state", using: :btree add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"} add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
add_index "issues", ["updated_at"], name: "index_issues_on_updated_at", using: :btree
add_index "issues", ["updated_by_id"], name: "index_issues_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree add_index "issues", ["updated_by_id"], name: "index_issues_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree
create_table "keys", force: :cascade do |t| create_table "keys", force: :cascade do |t|
......
...@@ -175,7 +175,7 @@ module API ...@@ -175,7 +175,7 @@ module API
end end
get "/search/:query", requirements: { query: /[^\/]+/ } do get "/search/:query", requirements: { query: /[^\/]+/ } do
search_service = Search::GlobalService.new(current_user, search: params[:query]).execute search_service = Search::GlobalService.new(current_user, search: params[:query]).execute
projects = search_service.objects('projects', params[:page]) projects = search_service.objects('projects', params[:page], false)
projects = projects.reorder(params[:order_by] => params[:sort]) projects = projects.reorder(params[:order_by] => params[:sort])
present paginate(projects), with: ::API::V3::Entities::Project present paginate(projects), with: ::API::V3::Entities::Project
......
...@@ -20,7 +20,7 @@ module Gitlab ...@@ -20,7 +20,7 @@ module Gitlab
when 'commits' when 'commits'
Kaminari.paginate_array(commits).page(page).per(per_page) Kaminari.paginate_array(commits).page(page).per(per_page)
else else
super super(scope, page, false)
end end
end end
......
...@@ -40,19 +40,21 @@ module Gitlab ...@@ -40,19 +40,21 @@ module Gitlab
@default_project_filter = default_project_filter @default_project_filter = default_project_filter
end end
def objects(scope, page = nil) def objects(scope, page = nil, without_count = true)
case scope collection = case scope
when 'projects' when 'projects'
projects.page(page).per(per_page) projects.page(page).per(per_page)
when 'issues' when 'issues'
issues.page(page).per(per_page) issues.page(page).per(per_page)
when 'merge_requests' when 'merge_requests'
merge_requests.page(page).per(per_page) merge_requests.page(page).per(per_page)
when 'milestones' when 'milestones'
milestones.page(page).per(per_page) milestones.page(page).per(per_page)
else else
Kaminari.paginate_array([]).page(page).per(per_page) Kaminari.paginate_array([]).page(page).per(per_page)
end end
without_count ? collection.without_count : collection
end end
def projects_count def projects_count
...@@ -71,18 +73,46 @@ module Gitlab ...@@ -71,18 +73,46 @@ module Gitlab
@milestones_count ||= milestones.count @milestones_count ||= milestones.count
end end
def limited_projects_count
@limited_projects_count ||= projects.limit(count_limit).count
end
def limited_issues_count
return @limited_issues_count if @limited_issues_count
# By default getting limited count (e.g. 1000+) is fast on issuable
# collections except for issues, where filtering both not confidential
# and confidential issues user has access to, is too complex.
# It's faster to try to fetch all public issues first, then only
# if necessary try to fetch all issues.
sum = issues(public_only: true).limit(count_limit).count
@limited_issues_count = sum < count_limit ? issues.limit(count_limit).count : sum
end
def limited_merge_requests_count
@limited_merge_requests_count ||= merge_requests.limit(count_limit).count
end
def limited_milestones_count
@limited_milestones_count ||= milestones.limit(count_limit).count
end
def single_commit_result? def single_commit_result?
false false
end end
def count_limit
1001
end
private private
def projects def projects
limit_projects.search(query) limit_projects.search(query)
end end
def issues def issues(finder_params = {})
issues = IssuesFinder.new(current_user).execute issues = IssuesFinder.new(current_user, finder_params).execute
unless default_project_filter unless default_project_filter
issues = issues.where(project_id: project_ids_relation) issues = issues.where(project_id: project_ids_relation)
end end
...@@ -94,13 +124,13 @@ module Gitlab ...@@ -94,13 +124,13 @@ module Gitlab
issues.full_search(query) issues.full_search(query)
end end
issues.order('updated_at DESC') issues.reorder('updated_at DESC')
end end
def milestones def milestones
milestones = Milestone.where(project_id: project_ids_relation) milestones = Milestone.where(project_id: project_ids_relation)
milestones = milestones.search(query) milestones = milestones.search(query)
milestones.order('updated_at DESC') milestones.reorder('updated_at DESC')
end end
def merge_requests def merge_requests
...@@ -116,7 +146,7 @@ module Gitlab ...@@ -116,7 +146,7 @@ module Gitlab
merge_requests.full_search(query) merge_requests.full_search(query)
end end
merge_requests.order('updated_at DESC') merge_requests.reorder('updated_at DESC')
end end
def default_scope def default_scope
......
...@@ -16,7 +16,7 @@ module Gitlab ...@@ -16,7 +16,7 @@ module Gitlab
when 'snippet_blobs' when 'snippet_blobs'
snippet_blobs.page(page).per(per_page) snippet_blobs.page(page).per(per_page)
else else
super super(scope, nil, false)
end end
end end
......
...@@ -22,7 +22,7 @@ feature 'Global search' do ...@@ -22,7 +22,7 @@ feature 'Global search' do
click_button "Go" click_button "Go"
select_filter("Issues") select_filter("Issues")
expect(page).to have_selector('.gl-pagination .page', count: 2) expect(page).to have_selector('.gl-pagination .next')
end end
end end
end end
...@@ -19,6 +19,12 @@ describe Gitlab::SearchResults do ...@@ -19,6 +19,12 @@ describe Gitlab::SearchResults do
project.add_developer(user) project.add_developer(user)
end end
describe '#objects' do
it 'returns without_page collection by default' do
expect(results.objects('projects')).to be_kind_of(Kaminari::PaginatableWithoutCount)
end
end
describe '#projects_count' do describe '#projects_count' do
it 'returns the total amount of projects' do it 'returns the total amount of projects' do
expect(results.projects_count).to eq(1) expect(results.projects_count).to eq(1)
...@@ -43,6 +49,58 @@ describe Gitlab::SearchResults do ...@@ -43,6 +49,58 @@ describe Gitlab::SearchResults do
end end
end end
context "when count_limit is lower than total amount" do
before do
allow(results).to receive(:count_limit).and_return(1)
end
describe '#limited_projects_count' do
it 'returns the limited amount of projects' do
create(:project, name: 'foo2')
expect(results.limited_projects_count).to eq(1)
end
end
describe '#limited_merge_requests_count' do
it 'returns the limited amount of merge requests' do
create(:merge_request, :simple, source_project: project, title: 'foo2')
expect(results.limited_merge_requests_count).to eq(1)
end
end
describe '#limited_milestones_count' do
it 'returns the limited amount of milestones' do
create(:milestone, project: project, title: 'foo2')
expect(results.limited_milestones_count).to eq(1)
end
end
describe '#limited_issues_count' do
it 'runs single SQL query to get the limited amount of issues' do
create(:milestone, project: project, title: 'foo2')
expect(results).to receive(:issues).with(public_only: true).and_call_original
expect(results).not_to receive(:issues).with(no_args).and_call_original
expect(results.limited_issues_count).to eq(1)
end
end
end
context "when count_limit is higher than total amount" do
describe '#limited_issues_count' do
it 'runs multiple queries to get the limited amount of issues' do
expect(results).to receive(:issues).with(public_only: true).and_call_original
expect(results).to receive(:issues).with(no_args).and_call_original
expect(results.limited_issues_count).to eq(1)
end
end
end
it 'includes merge requests from source and target projects' do it 'includes merge requests from source and target projects' do
forked_project = fork_project(project, user) forked_project = fork_project(project, user)
merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo') merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo')
......
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