From 29e60b0643ca5451919df233a9aeb4825779e846 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Aug 2019 13:53:02 -0700 Subject: Remove N+1 SQL query loading project feature in dashboard Projects that have a pipeline may need to check whether the user has permission to read the build (`can?(current_user, :read_build, project)`), which requires checking the `project_features` table. This would cause an N+1 SQL query for each project. This change also has a beneficial side effect that may avoid a race condition. When a user deletes a project, the project is queued for deletion and the user is redirected back to the dashboard page. However, the following may happen: 1. The dashboard page may load this deleted project in the list of 20 projects. 2. The view will load the project pipeline status from the cache and attempt to show each project. 3. When the view encounters the deleted project, it calls `can?(current_user, :read_build, project)` to determine whether to display the pipeline status. 4. Sidekiq deletes the project from the database. 5. However, since the deleted project is still loaded in memory, it will attempt to call `project.project_feature.access_level`. 6. Since `project_feature` was not eager loaded, a lazy `SELECT` call is made to the database. 7. This `SELECT` call returns nothing, and the user sees a 500 error. By eager loading `project_feature`, we can ensure that we have a consistent view and avoid records from being deleted later. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66482 --- app/controllers/dashboard/projects_controller.rb | 1 + .../unreleased/sh-project-feature-nplus-one.yml | 5 +++++ spec/features/dashboard/projects_spec.rb | 22 ++++++++++++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 changelogs/unreleased/sh-project-feature-nplus-one.yml diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 71f18694613..1dc89943f7f 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -70,6 +70,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController .new(params: finder_params, current_user: current_user) .execute .includes(:route, :creator, :group, namespace: [:route, :owner]) + .preload(:project_feature) .page(finder_params[:page]) prepare_projects_for_rendering(projects) diff --git a/changelogs/unreleased/sh-project-feature-nplus-one.yml b/changelogs/unreleased/sh-project-feature-nplus-one.yml new file mode 100644 index 00000000000..425ae7815c1 --- /dev/null +++ b/changelogs/unreleased/sh-project-feature-nplus-one.yml @@ -0,0 +1,5 @@ +--- +title: Remove N+1 SQL query loading project feature in dashboard +merge_request: 32169 +author: +type: performance diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb index e2100c8562b..f362172e712 100644 --- a/spec/features/dashboard/projects_spec.rb +++ b/spec/features/dashboard/projects_spec.rb @@ -220,4 +220,26 @@ describe 'Dashboard Projects' do expect(find('input#merge_request_target_branch', visible: false).value).to eq 'master' end end + + it 'avoids an N+1 query in dashboard index' do + create(:ci_pipeline, :with_job, status: :success, project: project, ref: project.default_branch, sha: project.commit.sha) + visit dashboard_projects_path + + control_count = ActiveRecord::QueryRecorder.new { visit dashboard_projects_path }.count + + new_project = create(:project, :repository, name: 'new project') + create(:ci_pipeline, :with_job, status: :success, project: new_project, ref: new_project.commit.sha) + new_project.add_developer(user) + + ActiveRecord::QueryRecorder.new { visit dashboard_projects_path }.count + + # There are three known N+1 queries: + # 1. Project#open_issues_count + # 2. Project#open_merge_requests_count + # 3. Project#forks_count + # + # In addition, ProjectsHelper#load_pipeline_status also adds an + # additional query. + expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 4) + end end -- cgit v1.2.1