summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--.gitlab/ci/rails.gitlab-ci.yml20
-rw-r--r--app/models/ci/pipeline.rb6
-rw-r--r--app/serializers/pipeline_entity.rb2
-rw-r--r--app/serializers/pipeline_serializer.rb5
-rw-r--r--changelogs/unreleased/sh-optimize-pipeline-sql.yml5
-rw-r--r--spec/controllers/projects/pipelines_controller_spec.rb48
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml2
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb4
8 files changed, 68 insertions, 24 deletions
diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml
index 4ac187e1670..885fd109227 100644
--- a/.gitlab/ci/rails.gitlab-ci.yml
+++ b/.gitlab/ci/rails.gitlab-ci.yml
@@ -57,7 +57,7 @@ setup-test-env:
dependencies: ["setup-test-env", "retrieve-tests-metadata", "compile-assets pull-cache"]
script:
- source scripts/rspec_helpers.sh
- - rspec_paralellized_job "--tag ~quarantine --tag ~geo"
+ - rspec_paralellized_job "--tag ~quarantine --tag ~geo --tag ~level:migration"
artifacts:
expire_in: 31d
when: always
@@ -92,12 +92,21 @@ setup-test-env:
- .use-pg10
- .only-master
+.rspec-base-migration:
+ script:
+ - source scripts/rspec_helpers.sh
+ - rspec_paralellized_job "--tag ~quarantine --tag ~geo --tag level:migration"
+
rspec migration pg9:
- extends: .rspec-base-pg9
+ extends:
+ - .rspec-base-pg9
+ - .rspec-base-migration
parallel: 4
rspec migration pg9-foss:
- extends: .rspec-base-pg9-foss
+ extends:
+ - .rspec-base-pg9-foss
+ - .rspec-base-migration
parallel: 4
rspec unit pg9:
@@ -149,7 +158,9 @@ rspec system pg10:
- .use-pg10-ee
rspec-ee migration pg9:
- extends: .rspec-ee-base-pg9
+ extends:
+ - .rspec-ee-base-pg9
+ - .rspec-base-migration
parallel: 2
rspec-ee unit pg9:
@@ -167,6 +178,7 @@ rspec-ee system pg9:
rspec-ee migration pg10:
extends:
- .rspec-ee-base-pg10
+ - .rspec-base-migration
- .only-master
parallel: 2
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 4cecf6f3fe4..ab0a4fd6289 100644
--- a/app/models/ci/pipeline.rb
+++ b/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
diff --git a/app/serializers/pipeline_entity.rb b/app/serializers/pipeline_entity.rb
index cddb894fd64..6b2a1bfe666 100644
--- a/app/serializers/pipeline_entity.rb
+++ b/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
diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb
index fc3160e3c69..b25a1ea9209 100644
--- a/app/serializers/pipeline_serializer.rb
+++ b/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 }],
diff --git a/changelogs/unreleased/sh-optimize-pipeline-sql.yml b/changelogs/unreleased/sh-optimize-pipeline-sql.yml
new file mode 100644
index 00000000000..f40fff99454
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-pipeline-sql.yml
@@ -0,0 +1,5 @@
+---
+title: Eliminate N+1 queries in PipelinesController#index
+merge_request: 22189
+author:
+type: performance
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index b4549e4e635..902a84a843b 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index a625d2f6cce..0dd2ebdb70d 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/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
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index f1f761a6fd0..8158277ffbc 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/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)