Commit a0a39f65 authored by Dylan Griffith's avatar Dylan Griffith

Remove cross-join from Build.with_project_and_metadata

This method didn't actually need to join. `preload` is sufficient in
this case. This commit also introduces documentation for this specific
case in case others run into a similar problem in future.
parent fd4ced7a
...@@ -151,7 +151,7 @@ module Ci ...@@ -151,7 +151,7 @@ module Ci
scope :with_project_and_metadata, -> do scope :with_project_and_metadata, -> do
if Feature.enabled?(:non_public_artifacts, type: :development) if Feature.enabled?(:non_public_artifacts, type: :development)
joins(:metadata).includes(:project, :metadata) joins(:metadata).includes(:metadata).preload(:project)
end end
end end
......
...@@ -131,6 +131,20 @@ to evaluate, because `UsageData` is not critical to users and it may be possible ...@@ -131,6 +131,20 @@ to evaluate, because `UsageData` is not critical to users and it may be possible
to get a similarly useful metric with a simpler approach. Alternatively we may to get a similarly useful metric with a simpler approach. Alternatively we may
find that nobody is using these metrics, so we can remove them. find that nobody is using these metrics, so we can remove them.
#### Use `preload` instead of `includes`
The `includes` and `preload` methods in Rails are both ways to avoid an N+1
query. The `includes` method in Rails uses a heuristic approach to determine
whether or not it needs to actually join to the table or whether it can load
all the records in a separate query. It assumes it needs to join if it thinks
you need to query the columns from the other table in some way, but sometimes
this method gets it wrong and it executes a join even when it isn't needed. In
this case using `preload` to explicitly load the data in a separate query will
allow you to avoid the join while still avoiding the N+1 query.
You can see a real example of this solution being used in
<https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67655>.
#### De-normalize some foreign key to the table #### De-normalize some foreign key to the table
De-normalization refers to adding redundant precomputed (duplicated) data to De-normalization refers to adding redundant precomputed (duplicated) data to
......
...@@ -5188,6 +5188,14 @@ RSpec.describe Ci::Build do ...@@ -5188,6 +5188,14 @@ RSpec.describe Ci::Build do
end end
end end
describe '.with_project_and_metadata' do
it 'does not join across databases' do
with_cross_joins_prevented do
::Ci::Build.with_project_and_metadata.to_a
end
end
end
describe '.without_coverage' do describe '.without_coverage' do
let!(:build_with_coverage) { create(:ci_build, pipeline: pipeline, coverage: 100.0) } let!(:build_with_coverage) { create(:ci_build, pipeline: pipeline, coverage: 100.0) }
......
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