Skip to content

Remove N+1 SQL query loading project feature in dashboard

Stan Hu requested to merge sh-project-feature-nplus-one into master

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.

Note that the views are cached, so it's possible that eager loads are unnecessary from a performance standpoint.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/66482

Edited by Stan Hu

Merge request reports