Use limit for search count queriessearch-100

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

13 files changed, 153 insertions, 31 deletions

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index d2275139c42..98831f5be4a 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -15,6 +15,7 @@
 #     label_name: string
 #     sort: string
 #     my_reaction_emoji: string
+#     public_only: boolean
 #
 class IssuesFinder < IssuableFinder
   CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
@@ -40,7 +41,15 @@ class IssuesFinder < IssuableFinder
   private
 
   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
 
   def user_can_see_all_confidential_issues?
diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb
index 0f9ac958f95..e6a6496871a 100644
--- a/app/helpers/search_helper.rb
+++ b/app/helpers/search_helper.rb
@@ -170,4 +170,8 @@ module SearchHelper
     # Truncato's filtered_tags and filtered_attributes are not quite the same
     sanitize(html, tags: %w(a p ol ul li pre code))
   end
+
+  def limited_count(count, limit = 1000)
+    count > limit ? "#{limit}+" : count
+  end
 end
diff --git a/app/views/search/_category.html.haml b/app/views/search/_category.html.haml
index 314d8e9cb25..915e648a5d3 100644
--- a/app/views/search/_category.html.haml
+++ b/app/views/search/_category.html.haml
@@ -57,25 +57,24 @@
           Titles and Filenames
           %span.badge
             = @search_results.snippet_titles_count
-
     - else
       %li{ class: active_when(@scope == 'projects') }
         = link_to search_filter_path(scope: 'projects') do
           Projects
           %span.badge
-            = @search_results.projects_count
+            = limited_count(@search_results.limited_projects_count)
       %li{ class: active_when(@scope == 'issues') }
         = link_to search_filter_path(scope: 'issues') do
           Issues
           %span.badge
-            = @search_results.issues_count
+            = limited_count(@search_results.limited_issues_count)
       %li{ class: active_when(@scope == 'merge_requests') }
         = link_to search_filter_path(scope: 'merge_requests') do
           Merge requests
           %span.badge
-            = @search_results.merge_requests_count
+            = limited_count(@search_results.limited_merge_requests_count)
       %li{ class: active_when(@scope == 'milestones') }
         = link_to search_filter_path(scope: 'milestones') do
           Milestones
           %span.badge
-            = @search_results.milestones_count
+            = limited_count(@search_results.limited_milestones_count)
diff --git a/app/views/search/_results.html.haml b/app/views/search/_results.html.haml
index 02133d09cdf..60ef44482f0 100644
--- a/app/views/search/_results.html.haml
+++ b/app/views/search/_results.html.haml
@@ -2,7 +2,8 @@
   = render partial: "search/results/empty"
 - else
   .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
       - if @project
         in project #{link_to @project.name_with_namespace, [@project.namespace.becomes(Namespace), @project]}
@@ -22,4 +23,4 @@
           = render partial: "search/results/#{@scope.singularize}", collection: @search_objects
 
     - if @scope != 'projects'
-      = paginate(@search_objects, theme: 'gitlab')
+      = paginate_collection(@search_objects)
diff --git a/changelogs/unreleased/40540-use-limit-for-global-search.yml b/changelogs/unreleased/40540-use-limit-for-global-search.yml
new file mode 100644
index 00000000000..7d9612c16df
--- /dev/null
+++ b/changelogs/unreleased/40540-use-limit-for-global-search.yml
@@ -0,0 +1,5 @@
+---
+title: Optimize search queries on the search page by setting a limit for matching records.
+merge_request:
+author:
+type: performance
diff --git a/db/migrate/20180115201419_add_index_updated_at_to_issues.rb b/db/migrate/20180115201419_add_index_updated_at_to_issues.rb
new file mode 100644
index 00000000000..a5a48fc97be
--- /dev/null
+++ b/db/migrate/20180115201419_add_index_updated_at_to_issues.rb
@@ -0,0 +1,15 @@
+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
diff --git a/db/schema.rb b/db/schema.rb
index a0901833c3d..4e82a688725 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # 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
   enable_extension "plpgsql"
@@ -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", ["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", ["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
 
   create_table "keys", force: :cascade do |t|
diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb
index 446f804124b..a7f0813bf74 100644
--- a/lib/api/v3/projects.rb
+++ b/lib/api/v3/projects.rb
@@ -175,7 +175,7 @@ module API
         end
         get "/search/:query", requirements: { query: /[^\/]+/ } do
           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])
 
           present paginate(projects), with: ::API::V3::Entities::Project
diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb
index 7771b15069b..4823f703ba4 100644
--- a/lib/gitlab/project_search_results.rb
+++ b/lib/gitlab/project_search_results.rb
@@ -20,7 +20,7 @@ module Gitlab
       when 'commits'
         Kaminari.paginate_array(commits).page(page).per(per_page)
       else
-        super
+        super(scope, page, false)
       end
     end
 
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index 70b639501fd..7362514167f 100644
--- a/lib/gitlab/search_results.rb
+++ b/lib/gitlab/search_results.rb
@@ -40,19 +40,21 @@ module Gitlab
       @default_project_filter = default_project_filter
     end
 
-    def objects(scope, page = nil)
-      case scope
-      when 'projects'
-        projects.page(page).per(per_page)
-      when 'issues'
-        issues.page(page).per(per_page)
-      when 'merge_requests'
-        merge_requests.page(page).per(per_page)
-      when 'milestones'
-        milestones.page(page).per(per_page)
-      else
-        Kaminari.paginate_array([]).page(page).per(per_page)
-      end
+    def objects(scope, page = nil, without_count = true)
+      collection = case scope
+                   when 'projects'
+                     projects.page(page).per(per_page)
+                   when 'issues'
+                     issues.page(page).per(per_page)
+                   when 'merge_requests'
+                     merge_requests.page(page).per(per_page)
+                   when 'milestones'
+                     milestones.page(page).per(per_page)
+                   else
+                     Kaminari.paginate_array([]).page(page).per(per_page)
+                   end
+
+      without_count ? collection.without_count : collection
     end
 
     def projects_count
@@ -71,18 +73,46 @@ module Gitlab
       @milestones_count ||= milestones.count
     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?
       false
     end
 
+    def count_limit
+      1001
+    end
+
     private
 
     def projects
       limit_projects.search(query)
     end
 
-    def issues
-      issues = IssuesFinder.new(current_user).execute
+    def issues(finder_params = {})
+      issues = IssuesFinder.new(current_user, finder_params).execute
       unless default_project_filter
         issues = issues.where(project_id: project_ids_relation)
       end
@@ -94,13 +124,13 @@ module Gitlab
           issues.full_search(query)
         end
 
-      issues.order('updated_at DESC')
+      issues.reorder('updated_at DESC')
     end
 
     def milestones
       milestones = Milestone.where(project_id: project_ids_relation)
       milestones = milestones.search(query)
-      milestones.order('updated_at DESC')
+      milestones.reorder('updated_at DESC')
     end
 
     def merge_requests
@@ -116,7 +146,7 @@ module Gitlab
           merge_requests.full_search(query)
         end
 
-      merge_requests.order('updated_at DESC')
+      merge_requests.reorder('updated_at DESC')
     end
 
     def default_scope
diff --git a/lib/gitlab/snippet_search_results.rb b/lib/gitlab/snippet_search_results.rb
index b85f70e450e..4f86b3e8f73 100644
--- a/lib/gitlab/snippet_search_results.rb
+++ b/lib/gitlab/snippet_search_results.rb
@@ -16,7 +16,7 @@ module Gitlab
       when 'snippet_blobs'
         snippet_blobs.page(page).per(per_page)
       else
-        super
+        super(scope, nil, false)
       end
     end
 
diff --git a/spec/features/global_search_spec.rb b/spec/features/global_search_spec.rb
index 4f575613848..f8c4db1403c 100644
--- a/spec/features/global_search_spec.rb
+++ b/spec/features/global_search_spec.rb
@@ -22,7 +22,7 @@ feature 'Global search' do
       click_button "Go"
 
       select_filter("Issues")
-      expect(page).to have_selector('.gl-pagination .page', count: 2)
+      expect(page).to have_selector('.gl-pagination .next')
     end
   end
 end
diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb
index b5a9ac570e6..17b48b3d062 100644
--- a/spec/lib/gitlab/search_results_spec.rb
+++ b/spec/lib/gitlab/search_results_spec.rb
@@ -19,6 +19,12 @@ describe Gitlab::SearchResults do
       project.add_developer(user)
     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
       it 'returns the total amount of projects' do
         expect(results.projects_count).to eq(1)
@@ -43,6 +49,58 @@ describe Gitlab::SearchResults do
       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
       forked_project = fork_project(project, user)
       merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo')