From fba43f49596ac794e743d1bb4a51fe8ad2d3f746 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 21 Feb 2018 13:42:15 +0000 Subject: 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 --- app/finders/snippets_finder.rb | 6 ++- app/models/project.rb | 46 ++++++++++++++++------ .../unreleased/42877-snippets-dashboard-slow.yml | 5 +++ ...rtial_index_to_projects_for_index_only_scans.rb | 21 ++++++++++ db/schema.rb | 1 + 5 files changed, 66 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/42877-snippets-dashboard-slow.yml create mode 100644 db/migrate/20180213131630_add_partial_index_to_projects_for_index_only_scans.rb 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 8efe5512d91..5bb461169f1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1475,6 +1475,7 @@ ActiveRecord::Schema.define(version: 20180216121030) 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 -- cgit v1.2.1