Commit cb401fb0 authored by Toon Claes's avatar Toon Claes

Merge branch '325274-use-cte-to-optimize-packages-query' into 'master'

Fix maven package query performance [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57041
parents 8ab79de8 be33d356
...@@ -33,7 +33,7 @@ module Packages ...@@ -33,7 +33,7 @@ module Packages
end end
def packages_with_path def packages_with_path
matching_packages = base.only_maven_packages_with_path(path) matching_packages = base.only_maven_packages_with_path(path, use_cte: group.present?)
matching_packages = matching_packages.order_by_package_file if versionless_package?(matching_packages) matching_packages = matching_packages.order_by_package_file if versionless_package?(matching_packages)
matching_packages matching_packages
......
...@@ -141,8 +141,19 @@ class Packages::Package < ApplicationRecord ...@@ -141,8 +141,19 @@ class Packages::Package < ApplicationRecord
where(project_id: projects) where(project_id: projects)
end end
def self.only_maven_packages_with_path(path) def self.only_maven_packages_with_path(path, use_cte: false)
joins(:maven_metadatum).where(packages_maven_metadata: { path: path }) if use_cte && Feature.enabled?(:maven_metadata_by_path_with_optimization_fence)
# This is an optimization fence which assumes that looking up the Metadatum record by path (globally)
# and then filter down the packages (by project or by group and subgroups) will be cheaper than
# looking up all packages within a project or group and filter them by path.
inner_query = Packages::Maven::Metadatum.where(path: path).select(:id, :package_id)
cte = Gitlab::SQL::CTE.new(:maven_metadata_by_path, inner_query)
with(cte.to_arel)
.joins('INNER JOIN maven_metadata_by_path ON maven_metadata_by_path.package_id=packages_packages.id')
else
joins(:maven_metadatum).where(packages_maven_metadata: { path: path })
end
end end
def self.by_name_and_file_name(name, file_name) def self.by_name_and_file_name(name, file_name)
......
---
name: maven_metadata_by_path_with_optimization_fence
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57041
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325460
milestone: '13.11'
type: development
group: group::optimize
default_enabled: false
...@@ -17,65 +17,89 @@ RSpec.describe ::Packages::Maven::PackageFinder do ...@@ -17,65 +17,89 @@ RSpec.describe ::Packages::Maven::PackageFinder do
group.add_developer(user) group.add_developer(user)
end end
describe '#execute!' do shared_examples 'Packages::Maven::PackageFinder examples' do
subject { finder.execute! } describe '#execute!' do
subject { finder.execute! }
shared_examples 'handling valid and invalid paths' do shared_examples 'handling valid and invalid paths' do
context 'with a valid path' do context 'with a valid path' do
let(:param_path) { package.maven_metadatum.path } let(:param_path) { package.maven_metadatum.path }
it { is_expected.to eq(package) } it { is_expected.to eq(package) }
end
context 'with an invalid path' do
let(:param_path) { 'com/example/my-app/1.0-SNAPSHOT' }
it 'raises an error' do
expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
context 'within the project' do
let(:param_project) { project }
it_behaves_like 'handling valid and invalid paths'
end end
context 'with an invalid path' do context 'within a group' do
let(:param_path) { 'com/example/my-app/1.0-SNAPSHOT' } let(:param_group) { group }
it_behaves_like 'handling valid and invalid paths'
end
context 'across all projects' do
it 'raises an error' do it 'raises an error' do
expect { subject }.to raise_error(ActiveRecord::RecordNotFound) expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
end end
end end
end
context 'within the project' do context 'versionless maven-metadata.xml package' do
let(:param_project) { project } let_it_be(:sub_group1) { create(:group, parent: group) }
let_it_be(:sub_group2) { create(:group, parent: group) }
let_it_be(:project1) { create(:project, group: sub_group1) }
let_it_be(:project2) { create(:project, group: sub_group2) }
let_it_be(:project3) { create(:project, group: sub_group1) }
let_it_be(:package_name) { 'foo' }
let_it_be(:package1) { create(:maven_package, project: project1, name: package_name, version: nil) }
let_it_be(:package2) { create(:maven_package, project: project2, name: package_name, version: nil) }
let_it_be(:package3) { create(:maven_package, project: project3, name: package_name, version: nil) }
let(:param_group) { group }
let(:param_path) { package_name }
before do
sub_group1.add_developer(user)
sub_group2.add_developer(user)
# the package with the most recently published file should be returned
create(:package_file, :xml, package: package2)
end
it_behaves_like 'handling valid and invalid paths' it { is_expected.to eq(package2) }
end
end end
end
context 'within a group' do context 'when the maven_metadata_by_path_with_optimization_fence feature flag is off' do
let(:param_group) { group } before do
stub_feature_flags(maven_metadata_by_path_with_optimization_fence: false)
it_behaves_like 'handling valid and invalid paths'
end end
context 'across all projects' do it_behaves_like 'Packages::Maven::PackageFinder examples'
it 'raises an error' do end
expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
end context 'when the maven_metadata_by_path_with_optimization_fence feature flag is on' do
before do
stub_feature_flags(maven_metadata_by_path_with_optimization_fence: true)
end end
context 'versionless maven-metadata.xml package' do it_behaves_like 'Packages::Maven::PackageFinder examples'
let_it_be(:sub_group1) { create(:group, parent: group) }
let_it_be(:sub_group2) { create(:group, parent: group) } it 'uses CTE in the query' do
let_it_be(:project1) { create(:project, group: sub_group1) } sql = described_class.new('some_path', user, group: group).send(:packages_with_path).to_sql
let_it_be(:project2) { create(:project, group: sub_group2) }
let_it_be(:project3) { create(:project, group: sub_group1) }
let_it_be(:package_name) { 'foo' }
let_it_be(:package1) { create(:maven_package, project: project1, name: package_name, version: nil) }
let_it_be(:package2) { create(:maven_package, project: project2, name: package_name, version: nil) }
let_it_be(:package3) { create(:maven_package, project: project3, name: package_name, version: nil) }
let(:param_group) { group }
let(:param_path) { package_name }
before do
sub_group1.add_developer(user)
sub_group2.add_developer(user)
# the package with the most recently published file should be returned
create(:package_file, :xml, package: package2)
end
it { is_expected.to eq(package2) } expect(sql).to include('WITH "maven_metadata_by_path" AS')
end end
end end
end end
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