Eliminate N+1 queries in LFS file locks checks during a push
This significantly improves performance when a user pushes many references. project.path_locks.any? doesn't cache the output and runs `SELECT 1 AS one FROM "path_locks" WHERE project_id = N` each time. When there are thousands of refs being pushed, this can time out the unicorn worker. CE port for https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6159.
This commit is contained in:
parent
02c007bda5
commit
4f9068dfc0
|
@ -25,6 +25,7 @@ class Project < ActiveRecord::Base
|
||||||
include FastDestroyAll::Helpers
|
include FastDestroyAll::Helpers
|
||||||
include WithUploads
|
include WithUploads
|
||||||
include BatchDestroyDependentAssociations
|
include BatchDestroyDependentAssociations
|
||||||
|
extend Gitlab::Cache::RequestCache
|
||||||
|
|
||||||
extend Gitlab::ConfigHelper
|
extend Gitlab::ConfigHelper
|
||||||
|
|
||||||
|
@ -2013,6 +2014,11 @@ class Project < ActiveRecord::Base
|
||||||
@gitlab_deploy_token ||= deploy_tokens.gitlab_deploy_token
|
@gitlab_deploy_token ||= deploy_tokens.gitlab_deploy_token
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def any_lfs_file_locks?
|
||||||
|
lfs_file_locks.any?
|
||||||
|
end
|
||||||
|
request_cache(:any_lfs_file_locks?) { self.id }
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def storage
|
def storage
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Eliminate N+1 queries in LFS file locks checks during a push
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: performance
|
|
@ -37,7 +37,7 @@ module Gitlab
|
||||||
|
|
||||||
def validate_lfs_file_locks?
|
def validate_lfs_file_locks?
|
||||||
strong_memoize(:validate_lfs_file_locks) do
|
strong_memoize(:validate_lfs_file_locks) do
|
||||||
project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev
|
project.lfs_enabled? && newrev && oldrev && project.any_lfs_file_locks?
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -934,6 +934,22 @@ describe Gitlab::GitAccess do
|
||||||
expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
|
expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
|
||||||
expect { push_access_check }.not_to raise_error
|
expect { push_access_check }.not_to raise_error
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'avoids N+1 queries', :request_store do
|
||||||
|
# Run this once to establish a baseline. Cached queries should get
|
||||||
|
# cached, so that when we introduce another change we shouldn't see
|
||||||
|
# additional queries.
|
||||||
|
access.check('git-receive-pack', changes)
|
||||||
|
|
||||||
|
control_count = ActiveRecord::QueryRecorder.new do
|
||||||
|
access.check('git-receive-pack', changes)
|
||||||
|
end
|
||||||
|
|
||||||
|
changes = ['6f6d7e7ed 570e7b2ab refs/heads/master', '6f6d7e7ed 570e7b2ab refs/heads/feature']
|
||||||
|
|
||||||
|
# There is still an N+1 query with protected branches
|
||||||
|
expect { access.check('git-receive-pack', changes) }.not_to exceed_query_limit(control_count).with_threshold(1)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -2339,6 +2339,22 @@ describe Project do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe '#any_lfs_file_locks?', :request_store do
|
||||||
|
set(:project) { create(:project) }
|
||||||
|
|
||||||
|
it 'returns false when there are no LFS file locks' do
|
||||||
|
expect(project.any_lfs_file_locks?).to be_falsey
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns a cached true when there are LFS file locks' do
|
||||||
|
create(:lfs_file_lock, project: project)
|
||||||
|
|
||||||
|
expect(project.lfs_file_locks).to receive(:any?).once.and_call_original
|
||||||
|
|
||||||
|
2.times { expect(project.any_lfs_file_locks?).to be_truthy }
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '#protected_for?' do
|
describe '#protected_for?' do
|
||||||
let(:project) { create(:project) }
|
let(:project) { create(:project) }
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue