Merge branch '42877-snippets-dashboard-slow' into 'master'

Improve query performance for Dashboard::SnippetsController#index

Closes #42877

See merge request gitlab-org/gitlab-ce!17088

5 files changed, 67 insertions, 14 deletions

diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb
index 33359fa1efb..ec61fe1892e 100644
--- a/app/finders/snippets_finder.rb
+++ b/app/finders/snippets_finder.rb
@@ -56,8 +56,10 @@ class SnippetsFinder < UnionFinder
   end
 
   def feature_available_projects
-    projects = Project.public_or_visible_to_user(current_user)
-      .with_feature_available_for_user(:snippets, current_user).select(:id)
+    projects = Project.public_or_visible_to_user(current_user, use_where_in: false) do |part|
+      part.with_feature_available_for_user(:snippets, current_user)
+    end.select(:id)
+
     arel_query = Arel::Nodes::SqlLiteral.new(projects.to_sql)
     table[:project_id].in(arel_query)
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index 2ba6a863500..79058d51af8 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -316,18 +316,42 @@ class Project < ActiveRecord::Base
 
   # Returns a collection of projects that is either public or visible to the
   # logged in user.
-  def self.public_or_visible_to_user(user = nil)
-    if user
-      authorized = user
-        .project_authorizations
-        .select(1)
-        .where('project_authorizations.project_id = projects.id')
-
-      levels = Gitlab::VisibilityLevel.levels_for_user(user)
-
-      where('EXISTS (?) OR projects.visibility_level IN (?)', authorized, levels)
+  #
+  # A caller may pass in a block to modify individual parts of
+  # the query, e.g. to apply .with_feature_available_for_user on top of it.
+  # This is useful for performance as we can stick those additional filters
+  # at the bottom of e.g. the UNION.
+  #
+  # Optionally, turning `use_where_in` off leads to returning a
+  # relation using #from instead of #where. This can perform much better
+  # but leads to trouble when used in conjunction with AR's #merge method.
+  def self.public_or_visible_to_user(user = nil, use_where_in: true, &block)
+    # If we don't get a block passed, use identity to avoid if/else repetitions
+    block = ->(part) { part } unless block_given?
+
+    return block.call(public_to_user) unless user
+
+    # If the user is allowed to see all projects,
+    # we can shortcut and just return.
+    return block.call(all) if user.full_private_access?
+
+    authorized = user
+      .project_authorizations
+      .select(1)
+      .where('project_authorizations.project_id = projects.id')
+    authorized_projects = block.call(where('EXISTS (?)', authorized))
+
+    levels = Gitlab::VisibilityLevel.levels_for_user(user)
+    visible_projects = block.call(where(visibility_level: levels))
+
+    # We use a UNION here instead of OR clauses since this results in better
+    # performance.
+    union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')])
+
+    if use_where_in
+      where("projects.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
     else
-      public_to_user
+      from("(#{union.to_sql}) AS #{table_name}")
     end
   end
 
diff --git a/changelogs/unreleased/42877-snippets-dashboard-slow.yml b/changelogs/unreleased/42877-snippets-dashboard-slow.yml
new file mode 100644
index 00000000000..839b44ad272
--- /dev/null
+++ b/changelogs/unreleased/42877-snippets-dashboard-slow.yml
@@ -0,0 +1,5 @@
+---
+title: Improve query performance for snippets dashboard.
+merge_request: 17088
+author:
+type: performance
diff --git a/db/migrate/20180213131630_add_partial_index_to_projects_for_index_only_scans.rb b/db/migrate/20180213131630_add_partial_index_to_projects_for_index_only_scans.rb
new file mode 100644
index 00000000000..cedf2510dda
--- /dev/null
+++ b/db/migrate/20180213131630_add_partial_index_to_projects_for_index_only_scans.rb
@@ -0,0 +1,21 @@
+class AddPartialIndexToProjectsForIndexOnlyScans < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_projects_on_id_partial_for_visibility'
+
+  disable_ddl_transaction!
+
+  # Adds a partial index to leverage index-only scans when looking up project ids
+  def up
+    unless index_exists?(:projects, :id, name: INDEX_NAME)
+      add_concurrent_index :projects, :id, name: INDEX_NAME, unique: true, where: 'visibility_level IN (10,20)'
+    end
+  end
+
+  def down
+    if index_exists?(:projects, :id, name: INDEX_NAME)
+      remove_concurrent_index_by_name :projects, INDEX_NAME
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 6b43fc8403c..409d1ac7644 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: 20180208183958) do
+ActiveRecord::Schema.define(version: 20180213131630) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1468,6 +1468,7 @@ ActiveRecord::Schema.define(version: 20180208183958) do
   add_index "projects", ["created_at"], name: "index_projects_on_created_at", using: :btree
   add_index "projects", ["creator_id"], name: "index_projects_on_creator_id", using: :btree
   add_index "projects", ["description"], name: "index_projects_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
+  add_index "projects", ["id"], name: "index_projects_on_id_partial_for_visibility", unique: true, where: "(visibility_level = ANY (ARRAY[10, 20]))", using: :btree
   add_index "projects", ["last_activity_at"], name: "index_projects_on_last_activity_at", using: :btree
   add_index "projects", ["last_repository_check_failed"], name: "index_projects_on_last_repository_check_failed", using: :btree
   add_index "projects", ["last_repository_updated_at"], name: "index_projects_on_last_repository_updated_at", using: :btree