Commit ab126ec6 authored by Stan Hu's avatar Stan Hu Committed by Mayra Cabrera

Use CTE optimization fence for loading projects in dashboard

Certain users experienced Error 500s when loading projects from the
dashboard because the PostgreSQL query planner attempted to scan all
projects rather than just the user's authorized projects. This would
cause the query to hit a statement timeout.

To fix this, we add support for a CTE optimization fence to load
authorized projects first, which can be optionally used by
`ProjectsFinder` via the `use_cte` parameter. To be safe, we only enable
it for the finder call that loads the list of projects behind the
`use_cte_for_projects_finder` feature flag.

Closes https://gitlab.com/gitlab-org/gitlab/issues/198440
parent ec9a0616
...@@ -66,6 +66,8 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController ...@@ -66,6 +66,8 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
@total_user_projects_count = ProjectsFinder.new(params: { non_public: true }, current_user: current_user).execute @total_user_projects_count = ProjectsFinder.new(params: { non_public: true }, current_user: current_user).execute
@total_starred_projects_count = ProjectsFinder.new(params: { starred: true }, current_user: current_user).execute @total_starred_projects_count = ProjectsFinder.new(params: { starred: true }, current_user: current_user).execute
finder_params[:use_cte] = Feature.enabled?(:use_cte_for_projects_finder, default_enabled: true)
projects = ProjectsFinder projects = ProjectsFinder
.new(params: finder_params, current_user: current_user) .new(params: finder_params, current_user: current_user)
.execute .execute
......
...@@ -44,6 +44,8 @@ class ProjectsFinder < UnionFinder ...@@ -44,6 +44,8 @@ class ProjectsFinder < UnionFinder
init_collection init_collection
end end
use_cte = params.delete(:use_cte)
collection = Project.wrap_authorized_projects_with_cte(collection) if use_cte
collection = filter_projects(collection) collection = filter_projects(collection)
sort(collection) sort(collection)
end end
...@@ -177,7 +179,7 @@ class ProjectsFinder < UnionFinder ...@@ -177,7 +179,7 @@ class ProjectsFinder < UnionFinder
end end
def sort(items) def sort(items)
params[:sort].present? ? items.sort_by_attribute(params[:sort]) : items.order_id_desc params[:sort].present? ? items.sort_by_attribute(params[:sort]) : items.projects_order_id_desc
end end
def by_archived(projects) def by_archived(projects)
......
...@@ -397,6 +397,8 @@ class Project < ApplicationRecord ...@@ -397,6 +397,8 @@ class Project < ApplicationRecord
scope :sorted_by_stars_desc, -> { reorder(star_count: :desc) } scope :sorted_by_stars_desc, -> { reorder(star_count: :desc) }
scope :sorted_by_stars_asc, -> { reorder(star_count: :asc) } scope :sorted_by_stars_asc, -> { reorder(star_count: :asc) }
scope :sorted_by_name_asc_limited, ->(limit) { reorder(name: :asc).limit(limit) } scope :sorted_by_name_asc_limited, ->(limit) { reorder(name: :asc).limit(limit) }
# Sometimes queries (e.g. using CTEs) require explicit disambiguation with table name
scope :projects_order_id_desc, -> { reorder("#{table_name}.id DESC") }
scope :in_namespace, ->(namespace_ids) { where(namespace_id: namespace_ids) } scope :in_namespace, ->(namespace_ids) { where(namespace_id: namespace_ids) }
scope :personal, ->(user) { where(namespace_id: user.namespace_id) } scope :personal, ->(user) { where(namespace_id: user.namespace_id) }
...@@ -543,6 +545,11 @@ class Project < ApplicationRecord ...@@ -543,6 +545,11 @@ class Project < ApplicationRecord
) )
end end
def self.wrap_authorized_projects_with_cte(collection)
cte = Gitlab::SQL::CTE.new(:authorized_projects, collection)
Project.with(cte.to_arel).from(cte.alias_to(Project.arel_table))
end
scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') } scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }
scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) } scope :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }
......
---
title: Use CTE optimization fence for loading projects in dashboard
merge_request: 23754
author:
type: performance
...@@ -28,10 +28,12 @@ describe ProjectsFinder, :do_not_mock_admin_mode do ...@@ -28,10 +28,12 @@ describe ProjectsFinder, :do_not_mock_admin_mode do
let(:params) { {} } let(:params) { {} }
let(:current_user) { user } let(:current_user) { user }
let(:project_ids_relation) { nil } let(:project_ids_relation) { nil }
let(:finder) { described_class.new(params: params, current_user: current_user, project_ids_relation: project_ids_relation) } let(:use_cte) { true }
let(:finder) { described_class.new(params: params.merge(use_cte: use_cte), current_user: current_user, project_ids_relation: project_ids_relation) }
subject { finder.execute } subject { finder.execute }
shared_examples 'ProjectFinder#execute examples' do
describe 'without a user' do describe 'without a user' do
let(:current_user) { nil } let(:current_user) { nil }
...@@ -202,6 +204,8 @@ describe ProjectsFinder, :do_not_mock_admin_mode do ...@@ -202,6 +204,8 @@ describe ProjectsFinder, :do_not_mock_admin_mode do
current_user.toggle_star(private_project) current_user.toggle_star(private_project)
is_expected.to eq([public_project]) is_expected.to eq([public_project])
expect(subject.count).to eq(1)
expect(subject.limit(1000).count).to eq(1)
end end
end end
...@@ -234,4 +238,17 @@ describe ProjectsFinder, :do_not_mock_admin_mode do ...@@ -234,4 +238,17 @@ describe ProjectsFinder, :do_not_mock_admin_mode do
end end
end end
end end
describe 'without CTE flag enabled' do
let(:use_cte) { false }
it_behaves_like 'ProjectFinder#execute examples'
end
describe 'with CTE flag enabled' do
let(:use_cte) { true }
it_behaves_like 'ProjectFinder#execute examples'
end
end
end end
...@@ -3780,6 +3780,25 @@ describe Project do ...@@ -3780,6 +3780,25 @@ describe Project do
end end
end end
describe '.wrap_authorized_projects_with_cte' do
let!(:user) { create(:user) }
let!(:private_project) do
create(:project, :private, creator: user, namespace: user.namespace)
end
let!(:public_project) { create(:project, :public) }
let(:projects) { described_class.all.public_or_visible_to_user(user) }
subject { described_class.wrap_authorized_projects_with_cte(projects) }
it 'wrapped query matches original' do
expect(subject.to_sql).to match(/^WITH "authorized_projects" AS/)
expect(subject).to match_array(projects)
end
end
describe '#pages_available?' do describe '#pages_available?' do
let(:project) { create(:project, group: group) } let(:project) { create(:project, group: group) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment