Commit c678c525 authored by Stan Hu's avatar Stan Hu
Browse files

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.
parent f6ebd2635419
......@@ -25,6 +25,7 @@ class Project < ActiveRecord::Base
include FastDestroyAll::Helpers
include WithUploads
include BatchDestroyDependentAssociations
extend Gitlab::Cache::RequestCache
extend Gitlab::ConfigHelper
......@@ -2013,6 +2014,11 @@ def gitlab_deploy_token
@gitlab_deploy_token ||= deploy_tokens.gitlab_deploy_token
end
def any_lfs_file_locks?
lfs_file_locks.any?
end
request_cache(:any_lfs_file_locks?) { self.id }
private
def storage
......
---
title: Eliminate N+1 queries in LFS file locks checks during a push
merge_request:
author:
type: performance
......@@ -37,7 +37,7 @@ def validate_file_paths
def validate_lfs_file_locks?
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
......
......@@ -934,6 +934,22 @@ def self.run_permission_checks(permissions_matrix)
expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
expect { push_access_check }.not_to raise_error
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
......
......@@ -2339,6 +2339,22 @@ def create_build(new_pipeline = pipeline, name = 'test')
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
let(:project) { create(:project) }
......
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