Merge branch 'ag-adapt-verification-queries' into 'master'

Improve performance of verification queries

See merge request gitlab-org/gitlab!71312

ee/app/models/ee/merge_request_diff.rb

@@ -29,6 +29,7 @@ module EE
       scope :has_external_diffs, -> { with_files.where(stored_externally: true) }
       scope :project_id_in, ->(ids) { where(merge_request_id: ::MergeRequest.where(target_project_id: ids)) }
       scope :available_replicables, -> { has_external_diffs }
+      scope :available_verifiables, -> { joins(:merge_request_diff_detail) }
       scope :with_verification_state, ->(state) { joins(:merge_request_diff_detail).where(merge_request_diff_details: { verification_state: verification_state_value(state) }) }
       scope :checksummed, -> { joins(:merge_request_diff_detail).where.not(merge_request_diff_details: { verification_checksum: nil } ) }
       scope :not_checksummed, -> { joins(:merge_request_diff_detail).where(merge_request_diff_details: { verification_checksum: nil } ) }

ee/app/services/geo/verification_state_backfill_service.rb

@@ -40,13 +40,17 @@ module Geo
     end
 
     # This method creates or deletes verification details records.
-    # It creates for available verifiables where records don't exist yet.
+    #
+    # It looks for replicable records that are eligible for verification (scoped
+    # as `verifiables`) but whose corresponding verification details record doesn't
+    # exist yet.
     # These would be replicable records that have recently become scoped as
-    # available_verifiables.
+    # `verifiables`, but were not so at the time of creation.
     # New replicable records will automatically create child records in the
     # verification details table, hence not created in this method.
+    #
     # When a replicable record is no longer a part of the scope
-    # available_veriables, it is deleted.
+    # `verifiables`, the corresponding verification state record needs to be deleted.
     # When a replicable record is deleted, the child record in the verification
     # details table is automatically removed, hence not deleted in this method.
     #

ee/lib/gitlab/geo/replicable_model.rb

@@ -21,7 +21,28 @@ module Gitlab
 
         # These scopes are intended to be overridden as needed
         scope :available_replicables, -> { all }
-        scope :available_verifiables, -> { self.respond_to?(:with_files_stored_locally) ? available_replicables.with_files_stored_locally : available_replicables }
+
+        # On primary, `verifiables` are records that can be checksummed and/or are replicable.
+
+        # On secondary, `verifiables` are records that have already been replicated
+        # and (ideally) have been checksummed on the primary
+
+        scope :verifiables, -> { self.respond_to?(:with_files_stored_locally) ? available_replicables.with_files_stored_locally : available_replicables }
+
+        # When storing verification details in the same table as the model,
+        # the scope `available_verifiables` returns only those records
+        # that are eligible for verification, i.e. the same as the scope
+        # `verifiables`.
+
+        # When using a separate table to store verification details,
+        # the scope `available_verifiables` should return all records
+        # from the separate table because the separate table will
+        # always only have records corresponding to replicables that are verifiable.
+        # For this, override the scope in the replicable model, e.g. like so in
+        # `MergeRequestDiff`,
+        # `scope :available_verifiables, -> { joins(:merge_request_diff_detail) }`
+
+        scope :available_verifiables, -> { verifiables }
       end
 
       class_methods do

ee/lib/gitlab/geo/verification_state.rb

@@ -95,7 +95,7 @@ module Gitlab
         def save_verification_details
           return unless self.class.separate_verification_state_table?
 
-          return unless self.class.available_verifiables.primary_key_in(self).exists?
+          return unless self.class.verifiables.primary_key_in(self).exists?
 
           # During a transaction, `verification_state_object` could be built before
           # a value for `verification_state_model_key` exists. So we check for that
@@ -306,7 +306,7 @@ module Gitlab
 
         def pluck_verifiable_ids_in_range(range)
           self
-            .available_verifiables
+            .verifiables
             .primary_key_in(range)
             .pluck_primary_key
         end

ee/spec/workers/geo/verification_state_backfill_service_spec.rb

@@ -3,15 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe Geo::VerificationStateBackfillService, :geo do
-  let(:replicable_1) { FactoryBot.create(:merge_request_diff, :external) }
-  let(:verifiable_1) { FactoryBot.create(:merge_request_diff_detail, merge_request_diff: replicable_1) }
+  let_it_be(:replicable) { create(:merge_request_diff, :external) }
 
   subject(:job) { described_class.new(MergeRequestDiff, batch_size: 1000) }
 
   describe '#execute' do
     context 'when a replicable is missing a corresponding verifiable' do
       before do
-        replicable_1.merge_request_diff_detail.destroy!
+        replicable.merge_request_diff_detail.destroy!
       end
 
       it 'adds a new verifiable' do
@@ -20,8 +19,10 @@ RSpec.describe Geo::VerificationStateBackfillService, :geo do
     end
 
     context 'when some replicables were removed from scope' do
+      let(:verifiable) { create(:merge_request_diff_detail, merge_request_diff: replicable) }
+
       before do
-        replicable_1.update_attribute(:stored_externally, false)
+        replicable.update_attribute(:stored_externally, false)
       end
 
       it 'deletes the verifiable' do