From 4f9068dfc06269ca7e2247fb0be2796452546de1 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Mon, 18 Jun 2018 16:38:34 -0700
Subject: [PATCH] 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.
---
 app/models/project.rb                            |  6 ++++++
 .../unreleased/sh-optimize-locks-check-ce.yml    |  5 +++++
 lib/gitlab/checks/commit_check.rb                |  2 +-
 spec/lib/gitlab/git_access_spec.rb               | 16 ++++++++++++++++
 spec/models/project_spec.rb                      | 16 ++++++++++++++++
 5 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/sh-optimize-locks-check-ce.yml

diff --git a/app/models/project.rb b/app/models/project.rb
index e5fa1c4db7b..0d777515536 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -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 @@ class Project < ActiveRecord::Base
     @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
diff --git a/changelogs/unreleased/sh-optimize-locks-check-ce.yml b/changelogs/unreleased/sh-optimize-locks-check-ce.yml
new file mode 100644
index 00000000000..933ec9b79bf
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-locks-check-ce.yml
@@ -0,0 +1,5 @@
+---
+title: Eliminate N+1 queries in LFS file locks checks during a push
+merge_request:
+author:
+type: performance
diff --git a/lib/gitlab/checks/commit_check.rb b/lib/gitlab/checks/commit_check.rb
index 43a52b493bb..22310e313ac 100644
--- a/lib/gitlab/checks/commit_check.rb
+++ b/lib/gitlab/checks/commit_check.rb
@@ -37,7 +37,7 @@ module Gitlab
 
       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
 
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 0d5f6a0b576..ff32025253a 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -934,6 +934,22 @@ describe Gitlab::GitAccess do
         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
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index bc9cce6b0c3..a2f8fac2f38 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2339,6 +2339,22 @@ describe Project do
     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) }