Merge branch 'optimize-pipelines-for-merge-requests-finder-query' into 'master'

Optimize the database query for pipelines for MRs

See merge request gitlab-org/gitlab!49083

app/finders/ci/pipelines_for_merge_request_finder.rb

@@ -5,8 +5,6 @@ module Ci
   class PipelinesForMergeRequestFinder
     include Gitlab::Utils::StrongMemoize
 
-    EVENT = 'merge_request_event'
-
     def initialize(merge_request, current_user)
       @merge_request = merge_request
       @current_user = current_user
@@ -36,7 +34,11 @@ module Ci
 
         pipelines =
           if merge_request.persisted?
-            pipelines_using_cte
+            if Feature.enabled?(:ci_pipelines_for_merge_request_finder_new_cte, target_project)
+              pipelines_using_cte
+            else
+              pipelines_using_legacy_cte
+            end
           else
             triggered_for_branch.for_sha(commit_shas)
           end
@@ -47,7 +49,7 @@ module Ci
 
     private
 
-    def pipelines_using_cte
+    def pipelines_using_legacy_cte
       cte = Gitlab::SQL::CTE.new(:shas, merge_request.all_commits.select(:sha))
 
       source_sha_join = cte.table[:sha].eq(Ci::Pipeline.arel_table[:source_sha])
@@ -59,6 +61,16 @@ module Ci
         .from_union([merged_result_pipelines, detached_merge_request_pipelines, pipelines_for_branch])
     end
 
+    def pipelines_using_cte
+      cte = Gitlab::SQL::CTE.new(:shas, merge_request.all_commits.select(:sha))
+
+      pipelines_for_merge_requests = triggered_by_merge_request
+      pipelines_for_branch = filter_by_sha(triggered_for_branch, cte)
+
+      Ci::Pipeline.with(cte.to_arel) # rubocop: disable CodeReuse/ActiveRecord
+        .from_union([pipelines_for_merge_requests, pipelines_for_branch])
+    end
+
     def filter_by_sha(pipelines, cte)
       hex = Arel::Nodes::SqlLiteral.new("'hex'")
       string_sha = Arel::Nodes::NamedFunction.new('encode', [cte.table[:sha], hex])
@@ -84,14 +96,7 @@ module Ci
     end
 
     def triggered_for_branch
-      source_project.ci_pipelines
-        .where(source: branch_pipeline_sources, ref: source_branch, tag: false) # rubocop: disable CodeReuse/ActiveRecord
-    end
-
-    def branch_pipeline_sources
-      strong_memoize(:branch_pipeline_sources) do
-        Ci::Pipeline.sources.reject { |source| source == EVENT }.values
-      end
+      source_project.all_pipelines.ci_branch_sources.for_branch(source_branch)
     end
 
     def sort(pipelines)

app/models/ci/pipeline.rb

@@ -277,12 +277,14 @@ module Ci
     scope :internal, -> { where(source: internal_sources) }
     scope :no_child, -> { where.not(source: :parent_pipeline) }
     scope :ci_sources, -> { where(source: Enums::Ci::Pipeline.ci_sources.values) }
+    scope :ci_branch_sources, -> { where(source: Enums::Ci::Pipeline.ci_branch_sources.values) }
     scope :ci_and_parent_sources, -> { where(source: Enums::Ci::Pipeline.ci_and_parent_sources.values) }
     scope :for_user, -> (user) { where(user: user) }
     scope :for_sha, -> (sha) { where(sha: sha) }
     scope :for_source_sha, -> (source_sha) { where(source_sha: source_sha) }
     scope :for_sha_or_source_sha, -> (sha) { for_sha(sha).or(for_source_sha(sha)) }
     scope :for_ref, -> (ref) { where(ref: ref) }
+    scope :for_branch, -> (branch) { for_ref(branch).where(tag: false) }
     scope :for_id, -> (id) { where(id: id) }
     scope :for_iid, -> (iid) { where(iid: iid) }
     scope :for_project, -> (project) { where(project: project) }
@@ -310,9 +312,9 @@ module Ci
     # In general, please use `Ci::PipelinesForMergeRequestFinder` instead,
     # for checking permission of the actor.
     scope :triggered_by_merge_request, -> (merge_request) do
-      ci_sources.where(source: :merge_request_event,
-                       merge_request: merge_request,
-                       project: [merge_request.source_project, merge_request.target_project])
+      where(source: :merge_request_event,
+            merge_request: merge_request,
+            project: [merge_request.source_project, merge_request.target_project])
     end
 
     # Returns the pipelines in descending order (= newest first), optionally

app/models/concerns/enums/ci/pipeline.rb

@@ -54,6 +54,10 @@ module Enums
         sources.except(*dangling_sources.keys)
       end
 
+      def self.ci_branch_sources
+        ci_sources.except(:merge_request_event)
+      end
+
       def self.ci_and_parent_sources
         ci_sources.merge(sources.slice(:parent_pipeline))
       end

config/feature_flags/development/ci_pipelines_for_merge_request_finder_new_cte.yml

+---
+name: ci_pipelines_for_merge_request_finder_new_cte
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49083
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/291006
+milestone: '13.7'
+type: development
+group: group::continuous integration
+default_enabled: false

spec/finders/ci/pipelines_for_merge_request_finder_spec.rb

@@ -225,6 +225,24 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
                     branch_pipeline_2,
                     branch_pipeline])
         end
+
+        context 'when ci_pipelines_for_merge_request_finder_new_cte feature flag is disabled' do
+          before do
+            stub_feature_flags(ci_pipelines_for_merge_request_finder_new_cte: false)
+          end
+
+          it 'returns only related merge request pipelines' do
+            expect(subject.all)
+              .to eq([detached_merge_request_pipeline,
+                      branch_pipeline_2,
+                      branch_pipeline])
+
+            expect(described_class.new(merge_request_2, nil).all)
+              .to eq([detached_merge_request_pipeline_2,
+                      branch_pipeline_2,
+                      branch_pipeline])
+          end
+        end
       end
 
       context 'when detached merge request pipeline is run on head ref of the merge request' do

spec/models/ci/pipeline_spec.rb

@@ -222,6 +222,26 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
   end
 
+  describe '.for_branch' do
+    subject { described_class.for_branch(branch) }
+
+    let(:branch) { 'master' }
+    let!(:pipeline) { create(:ci_pipeline, ref: 'master') }
+
+    it 'returns the pipeline' do
+      is_expected.to contain_exactly(pipeline)
+    end
+
+    context 'with tag pipeline' do
+      let(:branch) { 'v1.0' }
+      let!(:pipeline) { create(:ci_pipeline, ref: 'v1.0', tag: true) }
+
+      it 'returns nothing' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
   describe '.ci_sources' do
     subject { described_class.ci_sources }
 
@@ -242,6 +262,27 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
   end
 
+  describe '.ci_branch_sources' do
+    subject { described_class.ci_branch_sources }
+
+    let_it_be(:push_pipeline)   { create(:ci_pipeline, source: :push) }
+    let_it_be(:web_pipeline)    { create(:ci_pipeline, source: :web) }
+    let_it_be(:api_pipeline)    { create(:ci_pipeline, source: :api) }
+    let_it_be(:webide_pipeline) { create(:ci_pipeline, source: :webide) }
+    let_it_be(:child_pipeline)  { create(:ci_pipeline, source: :parent_pipeline) }
+    let_it_be(:merge_request_pipeline) { create(:ci_pipeline, :detached_merge_request_pipeline) }
+
+    it 'contains pipelines having CI only sources' do
+      expect(subject).to contain_exactly(push_pipeline, web_pipeline, api_pipeline)
+    end
+
+    it 'filters on expected sources' do
+      expect(::Enums::Ci::Pipeline.ci_branch_sources.keys).to contain_exactly(
+        *%i[unknown push web trigger schedule api external pipeline chat
+            external_pull_request_event])
+    end
+  end
+
   describe '.outside_pipeline_family' do
     subject(:outside_pipeline_family) { described_class.outside_pipeline_family(upstream_pipeline) }