Merge branch 'sh-use-cte-for-projects-finder' into 'master'

Use CTE optimization fence for loading projects in dashboard

Closes #198440

See merge request gitlab-org/gitlab!23754

app/controllers/dashboard/projects_controller.rb

@@ -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_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
                 .new(params: finder_params, current_user: current_user)
                 .execute

app/finders/projects_finder.rb

@@ -44,6 +44,8 @@ class ProjectsFinder < UnionFinder
         init_collection
       end
 
+    use_cte = params.delete(:use_cte)
+    collection = Project.wrap_authorized_projects_with_cte(collection) if use_cte
     collection = filter_projects(collection)
     sort(collection)
   end
@@ -177,7 +179,7 @@ class ProjectsFinder < UnionFinder
   end
 
   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
 
   def by_archived(projects)

app/models/project.rb

@@ -397,6 +397,8 @@ class Project < ApplicationRecord
   scope :sorted_by_stars_desc, -> { reorder(star_count: :desc) }
   scope :sorted_by_stars_asc, -> { reorder(star_count: :asc) }
   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 :personal, ->(user) { where(namespace_id: user.namespace_id) }
@@ -543,6 +545,11 @@ class Project < ApplicationRecord
       )
   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 :abandoned, -> { where('projects.last_activity_at < ?', 6.months.ago) }
 

changelogs/unreleased/sh-use-cte-for-projects-finder.yml

+---
+title: Use CTE optimization fence for loading projects in dashboard
+merge_request: 23754
+author:
+type: performance

spec/finders/projects_finder_spec.rb

@@ -28,210 +28,227 @@ describe ProjectsFinder, :do_not_mock_admin_mode do
     let(:params) { {} }
     let(:current_user) { user }
     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 }
 
-    describe 'without a user' do
-      let(:current_user) { nil }
+    shared_examples 'ProjectFinder#execute examples' do
+      describe 'without a user' do
+        let(:current_user) { nil }
 
-      it { is_expected.to eq([public_project]) }
-    end
-
-    describe 'with a user' do
-      describe 'without private projects' do
-        it { is_expected.to match_array([public_project, internal_project]) }
+        it { is_expected.to eq([public_project]) }
       end
 
-      describe 'with private projects' do
-        before do
-          private_project.add_maintainer(user)
+      describe 'with a user' do
+        describe 'without private projects' do
+          it { is_expected.to match_array([public_project, internal_project]) }
         end
 
-        it { is_expected.to match_array([public_project, internal_project, private_project]) }
+        describe 'with private projects' do
+          before do
+            private_project.add_maintainer(user)
+          end
+
+          it { is_expected.to match_array([public_project, internal_project, private_project]) }
+        end
       end
-    end
 
-    describe 'with project_ids_relation' do
-      let(:project_ids_relation) { Project.where(id: internal_project.id) }
+      describe 'with project_ids_relation' do
+        let(:project_ids_relation) { Project.where(id: internal_project.id) }
 
-      it { is_expected.to eq([internal_project]) }
-    end
+        it { is_expected.to eq([internal_project]) }
+      end
 
-    describe 'with id_after' do
-      context 'only returns projects with a project id greater than given' do
-        let(:params) { { id_after: internal_project.id }}
+      describe 'with id_after' do
+        context 'only returns projects with a project id greater than given' do
+          let(:params) { { id_after: internal_project.id }}
 
-        it { is_expected.to eq([public_project]) }
+          it { is_expected.to eq([public_project]) }
+        end
       end
-    end
 
-    describe 'with id_before' do
-      context 'only returns projects with a project id less than given' do
-        let(:params) { { id_before: public_project.id }}
+      describe 'with id_before' do
+        context 'only returns projects with a project id less than given' do
+          let(:params) { { id_before: public_project.id }}
 
-        it { is_expected.to eq([internal_project]) }
+          it { is_expected.to eq([internal_project]) }
+        end
       end
-    end
 
-    describe 'with both id_before and id_after' do
-      context 'only returns projects with a project id less than given' do
-        let!(:projects) { create_list(:project, 5, :public) }
-        let(:params) { { id_after: projects.first.id, id_before: projects.last.id }}
+      describe 'with both id_before and id_after' do
+        context 'only returns projects with a project id less than given' do
+          let!(:projects) { create_list(:project, 5, :public) }
+          let(:params) { { id_after: projects.first.id, id_before: projects.last.id }}
 
-        it { is_expected.to contain_exactly(*projects[1..-2]) }
+          it { is_expected.to contain_exactly(*projects[1..-2]) }
+        end
       end
-    end
 
-    describe 'filter by visibility_level' do
-      before do
-        private_project.add_maintainer(user)
-      end
+      describe 'filter by visibility_level' do
+        before do
+          private_project.add_maintainer(user)
+        end
 
-      context 'private' do
-        let(:params) { { visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
+        context 'private' do
+          let(:params) { { visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
 
-        it { is_expected.to eq([private_project]) }
-      end
+          it { is_expected.to eq([private_project]) }
+        end
 
-      context 'internal' do
-        let(:params) { { visibility_level: Gitlab::VisibilityLevel::INTERNAL } }
+        context 'internal' do
+          let(:params) { { visibility_level: Gitlab::VisibilityLevel::INTERNAL } }
 
-        it { is_expected.to eq([internal_project]) }
+          it { is_expected.to eq([internal_project]) }
+        end
+
+        context 'public' do
+          let(:params) { { visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
+
+          it { is_expected.to eq([public_project]) }
+        end
       end
 
-      context 'public' do
-        let(:params) { { visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
+      describe 'filter by tags' do
+        before do
+          public_project.tag_list.add('foo')
+          public_project.save!
+        end
+
+        let(:params) { { tag: 'foo' } }
 
         it { is_expected.to eq([public_project]) }
       end
-    end
 
-    describe 'filter by tags' do
-      before do
-        public_project.tag_list.add('foo')
-        public_project.save!
+      describe 'filter by personal' do
+        let!(:personal_project) { create(:project, namespace: user.namespace) }
+        let(:params) { { personal: true } }
+
+        it { is_expected.to eq([personal_project]) }
       end
 
-      let(:params) { { tag: 'foo' } }
+      describe 'filter by search' do
+        let(:params) { { search: 'C' } }
 
-      it { is_expected.to eq([public_project]) }
-    end
+        it { is_expected.to eq([public_project]) }
+      end
 
-    describe 'filter by personal' do
-      let!(:personal_project) { create(:project, namespace: user.namespace) }
-      let(:params) { { personal: true } }
+      describe 'filter by name for backward compatibility' do
+        let(:params) { { name: 'C' } }
 
-      it { is_expected.to eq([personal_project]) }
-    end
+        it { is_expected.to eq([public_project]) }
+      end
 
-    describe 'filter by search' do
-      let(:params) { { search: 'C' } }
+      describe 'filter by archived' do
+        let!(:archived_project) { create(:project, :public, :archived, name: 'E', path: 'E') }
 
-      it { is_expected.to eq([public_project]) }
-    end
+        context 'non_archived=true' do
+          let(:params) { { non_archived: true } }
 
-    describe 'filter by name for backward compatibility' do
-      let(:params) { { name: 'C' } }
+          it { is_expected.to match_array([public_project, internal_project]) }
+        end
 
-      it { is_expected.to eq([public_project]) }
-    end
+        context 'non_archived=false' do
+          let(:params) { { non_archived: false } }
 
-    describe 'filter by archived' do
-      let!(:archived_project) { create(:project, :public, :archived, name: 'E', path: 'E') }
+          it { is_expected.to match_array([public_project, internal_project, archived_project]) }
+        end
 
-      context 'non_archived=true' do
-        let(:params) { { non_archived: true } }
+        describe 'filter by archived only' do
+          let(:params) { { archived: 'only' } }
 
-        it { is_expected.to match_array([public_project, internal_project]) }
-      end
+          it { is_expected.to eq([archived_project]) }
+        end
 
-      context 'non_archived=false' do
-        let(:params) { { non_archived: false } }
+        describe 'filter by archived for backward compatibility' do
+          let(:params) { { archived: false } }
 
-        it { is_expected.to match_array([public_project, internal_project, archived_project]) }
+          it { is_expected.to match_array([public_project, internal_project]) }
+        end
       end
 
-      describe 'filter by archived only' do
-        let(:params) { { archived: 'only' } }
+      describe 'filter by trending' do
+        let!(:trending_project) { create(:trending_project, project: public_project) }
+        let(:params) { { trending: true } }
 
-        it { is_expected.to eq([archived_project]) }
+        it { is_expected.to eq([public_project]) }
       end
 
-      describe 'filter by archived for backward compatibility' do
-        let(:params) { { archived: false } }
+      describe 'filter by owned' do
+        let(:params) { { owned: true } }
+        let!(:owned_project) { create(:project, :private, namespace: current_user.namespace) }
 
-        it { is_expected.to match_array([public_project, internal_project]) }
+        it { is_expected.to eq([owned_project]) }
       end
-    end
 
-    describe 'filter by trending' do
-      let!(:trending_project) { create(:trending_project, project: public_project) }
-      let(:params) { { trending: true } }
-
-      it { is_expected.to eq([public_project]) }
-    end
+      describe 'filter by non_public' do
+        let(:params) { { non_public: true } }
 
-    describe 'filter by owned' do
-      let(:params) { { owned: true } }
-      let!(:owned_project) { create(:project, :private, namespace: current_user.namespace) }
+        before do
+          private_project.add_developer(current_user)
+        end
 
-      it { is_expected.to eq([owned_project]) }
-    end
+        it { is_expected.to eq([private_project]) }
+      end
 
-    describe 'filter by non_public' do
-      let(:params) { { non_public: true } }
+      describe 'filter by starred' do
+        let(:params) { { starred: true } }
 
-      before do
-        private_project.add_developer(current_user)
-      end
+        before do
+          current_user.toggle_star(public_project)
+        end
 
-      it { is_expected.to eq([private_project]) }
-    end
+        it { is_expected.to eq([public_project]) }
 
-    describe 'filter by starred' do
-      let(:params) { { starred: true } }
+        it 'returns only projects the user has access to' do
+          current_user.toggle_star(private_project)
 
-      before do
-        current_user.toggle_star(public_project)
+          is_expected.to eq([public_project])
+          expect(subject.count).to eq(1)
+          expect(subject.limit(1000).count).to eq(1)
+        end
       end
 
-      it { is_expected.to eq([public_project]) }
+      describe 'filter by without_deleted' do
+        let(:params) { { without_deleted: true } }
+        let!(:pending_delete_project) { create(:project, :public, pending_delete: true) }
 
-      it 'returns only projects the user has access to' do
-        current_user.toggle_star(private_project)
+        it { is_expected.to match_array([public_project, internal_project]) }
+      end
 
-        is_expected.to eq([public_project])
+      describe 'sorting' do
+        let(:params) { { sort: 'name_asc' } }
+
+        it { is_expected.to eq([internal_project, public_project]) }
       end
-    end
 
-    describe 'filter by without_deleted' do
-      let(:params) { { without_deleted: true } }
-      let!(:pending_delete_project) { create(:project, :public, pending_delete: true) }
+      describe 'with admin user' do
+        let(:user) { create(:admin) }
 
-      it { is_expected.to match_array([public_project, internal_project]) }
-    end
+        context 'admin mode enabled' do
+          before do
+            enable_admin_mode!(current_user)
+          end
 
-    describe 'sorting' do
-      let(:params) { { sort: 'name_asc' } }
+          it { is_expected.to match_array([public_project, internal_project, private_project, shared_project]) }
+        end
 
-      it { is_expected.to eq([internal_project, public_project]) }
+        context 'admin mode disabled' do
+          it { is_expected.to match_array([public_project, internal_project]) }
+        end
+      end
     end
 
-    describe 'with admin user' do
-      let(:user) { create(:admin) }
+    describe 'without CTE flag enabled' do
+      let(:use_cte) { false }
 
-      context 'admin mode enabled' do
-        before do
-          enable_admin_mode!(current_user)
-        end
+      it_behaves_like 'ProjectFinder#execute examples'
+    end
 
-        it { is_expected.to match_array([public_project, internal_project, private_project, shared_project]) }
-      end
+    describe 'with CTE flag enabled' do
+      let(:use_cte) { true }
 
-      context 'admin mode disabled' do
-        it { is_expected.to match_array([public_project, internal_project]) }
-      end
+      it_behaves_like 'ProjectFinder#execute examples'
     end
   end
 end

spec/models/project_spec.rb

@@ -3780,6 +3780,25 @@ describe Project do
     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
     let(:project) { create(:project, group: group) }