Eliminate N+1 queries in PipelinesController#index

https://gitlab.com/gitlab-org/gitlab/merge_requests/20615 introduced a
performance regression: each failed build would generate additional SQL
queries, since they would be rendered by JobEntity. This commit fixes
that by preloading failed builds among other items and adds an N+1 query
test. This adds 4 additional queries per pipeline:

1. user
2. latest_statuses_ordered_by_stage
3. failed_builds
4. failed_builds metadata

Closes https://gitlab.com/gitlab-org/gitlab/issues/118427

app/models/ci/pipeline.rb

@@ -32,6 +32,7 @@ module Ci
 
     has_many :stages, -> { order(position: :asc) }, inverse_of: :pipeline
     has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
+    has_many :latest_statuses_ordered_by_stage, -> { latest.order(:stage_idx, :stage) }, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :processables, -> { processables },
              class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline
@@ -45,6 +46,7 @@ module Ci
     has_many :merge_requests_as_head_pipeline, foreign_key: "head_pipeline_id", class_name: 'MergeRequest'
 
     has_many :pending_builds, -> { pending }, foreign_key: :commit_id, class_name: 'Ci::Build'
+    has_many :failed_builds, -> { latest.failed }, foreign_key: :commit_id, class_name: 'Ci::Build', inverse_of: :pipeline
     has_many :retryable_builds, -> { latest.failed_or_canceled.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build'
     has_many :cancelable_statuses, -> { cancelable }, foreign_key: :commit_id, class_name: 'CommitStatus'
     has_many :manual_actions, -> { latest.manual_actions.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build'
@@ -383,9 +385,7 @@ module Ci
     end
 
     def legacy_stages_using_composite_status
-      stages = statuses.latest
-        .order(:stage_idx, :stage)
-        .group_by(&:stage)
+      stages = latest_statuses_ordered_by_stage.group_by(&:stage)
 
       stages.map do |stage_name, jobs|
         composite_status = Gitlab::Ci::Status::Composite

app/serializers/pipeline_entity.rb

@@ -78,7 +78,7 @@ class PipelineEntity < Grape::Entity
   end
 
   expose :failed_builds, if: -> (*) { can_retry? }, using: JobEntity do |pipeline|
-    pipeline.builds.failed
+    pipeline.failed_builds
   end
 
   private

app/serializers/pipeline_serializer.rb

@@ -40,7 +40,11 @@ class PipelineSerializer < BaseSerializer
 
   def preloaded_relations
     [
+      :latest_statuses_ordered_by_stage,
       :stages,
+      {
+        failed_builds: %i(project metadata)
+      },
       :retryable_builds,
       :cancelable_statuses,
       :trigger_requests,
@@ -48,6 +52,7 @@ class PipelineSerializer < BaseSerializer
       :scheduled_actions,
       :artifacts,
       :merge_request,
+      :user,
       {
         pending_builds: :project,
         project: [:route, { namespace: :route }],

changelogs/unreleased/sh-optimize-pipeline-sql.yml

+---
+title: Eliminate N+1 queries in PipelinesController#index
+merge_request: 22189
+author:
+type: performance

spec/controllers/projects/pipelines_controller_spec.rb

@@ -6,7 +6,7 @@ describe Projects::PipelinesController do
   include ApiHelpers
 
   let_it_be(:user) { create(:user) }
-  let(:project) { create(:project, :public, :repository) }
+  let_it_be(:project) { create(:project, :public, :repository) }
   let(:feature) { ProjectFeature::ENABLED }
 
   before do
@@ -19,12 +19,12 @@ describe Projects::PipelinesController do
 
   describe 'GET index.json' do
     before do
-      %w(pending running success failed canceled).each_with_index do |status, index|
-        create_pipeline(status, project.commit("HEAD~#{index}"))
-      end
+      create_all_pipeline_types
     end
 
     context 'when using persisted stages', :request_store do
+      render_views
+
       before do
         stub_feature_flags(ci_pipeline_persisted_stages: true)
       end
@@ -32,9 +32,7 @@ describe Projects::PipelinesController do
       it 'returns serialized pipelines', :request_store do
         expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
 
-        queries = ActiveRecord::QueryRecorder.new do
-          get_pipelines_index_json
-        end
+        get_pipelines_index_json
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(response).to match_response_schema('pipeline')
@@ -49,8 +47,22 @@ describe Projects::PipelinesController do
         json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages|
           expect(stages.count).to eq 3
         end
+      end
+
+      it 'does not execute N+1 queries' do
+        get_pipelines_index_json
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          get_pipelines_index_json
+        end.count
 
-        expect(queries.count).to be
+        create_all_pipeline_types
+
+        # There appears to be one extra query for Pipelines#has_warnings? for some reason
+        expect { get_pipelines_index_json }.not_to exceed_query_limit(control_count + 1)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['pipelines'].count).to eq 10
       end
     end
 
@@ -133,19 +145,27 @@ describe Projects::PipelinesController do
                   format: :json
     end
 
+    def create_all_pipeline_types
+      %w(pending running success failed canceled).each_with_index do |status, index|
+        create_pipeline(status, project.commit("HEAD~#{index}"))
+      end
+    end
+
     def create_pipeline(status, sha)
+      user = create(:user)
       pipeline = create(:ci_empty_pipeline, status: status,
                                             project: project,
-                                            sha: sha)
+                                            sha: sha,
+                                            user: user)
 
-      create_build(pipeline, 'build', 1, 'build')
-      create_build(pipeline, 'test', 2, 'test')
-      create_build(pipeline, 'deploy', 3, 'deploy')
+      create_build(pipeline, 'build', 1, 'build', user)
+      create_build(pipeline, 'test', 2, 'test', user)
+      create_build(pipeline, 'deploy', 3, 'deploy', user)
     end
 
-    def create_build(pipeline, stage, stage_idx, name)
+    def create_build(pipeline, stage, stage_idx, name, user = nil)
       status = %w[created running pending success failed canceled].sample
-      create(:ci_build, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name, status: status)
+      create(:ci_build, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name, status: status, user: user)
     end
   end
 

spec/lib/gitlab/import_export/all_models.yml

@@ -160,6 +160,7 @@ ci_pipelines:
 - user
 - stages
 - statuses
+- latest_statuses_ordered_by_stage
 - builds
 - processables
 - trigger_requests
@@ -168,6 +169,7 @@ ci_pipelines:
 - auto_canceled_pipelines
 - auto_canceled_jobs
 - pending_builds
+- failed_builds
 - retryable_builds
 - cancelable_statuses
 - manual_actions

spec/serializers/pipeline_serializer_spec.rb

@@ -159,7 +159,7 @@ describe PipelineSerializer do
 
         it 'verifies number of queries', :request_store do
           recorded = ActiveRecord::QueryRecorder.new { subject }
-          expected_queries = Gitlab.ee? ? 38 : 35
+          expected_queries = Gitlab.ee? ? 42 : 39
 
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)
@@ -180,7 +180,7 @@ describe PipelineSerializer do
           # pipeline. With the same ref this check is cached but if refs are
           # different then there is an extra query per ref
           # https://gitlab.com/gitlab-org/gitlab-foss/issues/46368
-          expected_queries = Gitlab.ee? ? 41 : 38
+          expected_queries = Gitlab.ee? ? 45 : 42
 
           expect(recorded.count).to be_within(2).of(expected_queries)
           expect(recorded.cached_count).to eq(0)