Throttle the number of UPDATEs triggered by touchthrottle-touching-of-objects

This throttles the number of UPDATE queries that can be triggered by
calling "touch" on a Note, Issue, or MergeRequest. For Note objects we
also take care of updating the associated "noteable" relation in a
smarter way than Rails does by default.

10 files changed, 81 insertions, 3 deletions

diff --git a/app/models/concerns/throttled_touch.rb b/app/models/concerns/throttled_touch.rb
new file mode 100644
index 00000000000..ad0ff0f20d4
--- /dev/null
+++ b/app/models/concerns/throttled_touch.rb
@@ -0,0 +1,10 @@
+# ThrottledTouch can be used to throttle the number of updates triggered by
+# calling "touch" on an ActiveRecord model.
+module ThrottledTouch
+  # The amount of time to wait before "touch" can update a record again.
+  TOUCH_INTERVAL = 1.minute
+
+  def touch(*args)
+    super if (Time.zone.now - updated_at) > TOUCH_INTERVAL
+  end
+end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index d6ef58d150b..33db197e612 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -9,6 +9,7 @@ class Issue < ActiveRecord::Base
   include FasterCacheKeys
   include RelativePositioning
   include TimeTrackable
+  include ThrottledTouch
 
   DueDateStruct = Struct.new(:title, :name).freeze
   NoDueDate     = DueDateStruct.new('No Due Date', '0').freeze
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index bbc01e9677c..f0f76baba22 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -7,6 +7,7 @@ class MergeRequest < ActiveRecord::Base
   include TimeTrackable
   include ManualInverseAssociation
   include EachBatch
+  include ThrottledTouch
 
   ignore_column :locked_at,
                 :ref_fetched
diff --git a/app/models/note.rb b/app/models/note.rb
index 340fe087f82..581adabbb96 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -15,6 +15,7 @@ class Note < ActiveRecord::Base
   include IgnorableColumn
   include Editable
   include Gitlab::SQL::Pattern
+  include ThrottledTouch
 
   module SpecialRole
     FIRST_TIME_CONTRIBUTOR = :first_time_contributor
@@ -55,7 +56,7 @@ class Note < ActiveRecord::Base
   participant :author
 
   belongs_to :project
-  belongs_to :noteable, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
+  belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
   belongs_to :author, class_name: "User"
   belongs_to :updated_by, class_name: "User"
   belongs_to :last_edited_by, class_name: 'User'
@@ -118,6 +119,7 @@ class Note < ActiveRecord::Base
   before_validation :set_discussion_id, on: :create
   after_save :keep_around_commit, if: :for_project_noteable?
   after_save :expire_etag_cache
+  after_save :touch_noteable
   after_destroy :expire_etag_cache
 
   class << self
@@ -367,6 +369,38 @@ class Note < ActiveRecord::Base
     Gitlab::EtagCaching::Store.new.touch(key)
   end
 
+  def touch(*args)
+    # We're not using an explicit transaction here because this would in all
+    # cases result in all future queries going to the primary, even if no writes
+    # are performed.
+    #
+    # We touch the noteable first so its SELECT query can run before our writes,
+    # ensuring it runs on a secondary (if no prior write took place).
+    touch_noteable
+    super
+  end
+
+  # By default Rails will issue an "SELECT *" for the relation, which is an
+  # overkill for just updating the timestamps. To work around this we manually
+  # touch the data so we can SELECT only the columns we need.
+  def touch_noteable
+    # Commits are not stored in the DB so we can't touch them.
+    return if for_commit?
+
+    assoc = association(:noteable)
+
+    noteable_object =
+      if assoc.loaded?
+        noteable
+      else
+        # If the object is not loaded (e.g. when notes are loaded async) we
+        # _only_ want the data we actually need.
+        assoc.scope.select(:id, :updated_at).take
+      end
+
+    noteable_object&.touch
+  end
+
   private
 
   def keep_around_commit
diff --git a/changelogs/unreleased/throttle-touching-of-objects.yml b/changelogs/unreleased/throttle-touching-of-objects.yml
new file mode 100644
index 00000000000..0a57bea7c83
--- /dev/null
+++ b/changelogs/unreleased/throttle-touching-of-objects.yml
@@ -0,0 +1,5 @@
+---
+title: Throttle the number of UPDATEs triggered by touch
+merge_request:
+author:
+type: performance
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index a53b59c4e08..9df26f06a11 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -171,7 +171,7 @@ describe Issuable do
 
     it "returns false when record has been updated" do
       allow(issue).to receive(:today?).and_return(true)
-      issue.touch
+      issue.update_attribute(:updated_at, 1.hour.ago)
       expect(issue.new?).to be_falsey
     end
   end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 5f901262598..0ea287d007a 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -765,4 +765,8 @@ describe Issue do
       expect(described_class.public_only).to eq([public_issue])
     end
   end
+
+  it_behaves_like 'throttled touch' do
+    subject { create(:issue, updated_at: 1.hour.ago) }
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 728028746d8..46771ee83a5 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1824,4 +1824,8 @@ describe MergeRequest do
         .to change { project.open_merge_requests_count }.from(1).to(0)
     end
   end
+
+  it_behaves_like 'throttled touch' do
+    subject { create(:merge_request, updated_at: 1.hour.ago) }
+  end
 end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 6e7e8c4c570..e1a0c55b6a6 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -5,7 +5,7 @@ describe Note do
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
-    it { is_expected.to belong_to(:noteable).touch(true) }
+    it { is_expected.to belong_to(:noteable).touch(false) }
     it { is_expected.to belong_to(:author).class_name('User') }
 
     it { is_expected.to have_many(:todos).dependent(:destroy) }
diff --git a/spec/support/shared_examples/throttled_touch.rb b/spec/support/shared_examples/throttled_touch.rb
new file mode 100644
index 00000000000..4caebc83334
--- /dev/null
+++ b/spec/support/shared_examples/throttled_touch.rb
@@ -0,0 +1,19 @@
+shared_examples_for 'throttled touch' do
+  describe '#touch' do
+    it 'updates the updated_at timestamp' do
+      Timecop.freeze do
+        subject.touch
+        expect(subject.updated_at).to eq(Time.zone.now)
+      end
+    end
+
+    it 'updates the object at most once per minute' do
+      updated_at = 120.seconds.ago
+
+      Timecop.freeze(updated_at) { subject.touch }
+      Timecop.freeze(90.seconds.ago) { subject.touch }
+
+      expect(subject.updated_at).to eq(updated_at)
+    end
+  end
+end