Merge branch '11922-geo-remove-legacy-queries' into 'master'

Geo - Clean up code related to legacy queries

Closes #11922

See merge request gitlab-org/gitlab-ee!14355

ee/app/finders/geo/attachment_registry_finder.rb

@@ -2,10 +2,6 @@
 
 module Geo
   class AttachmentRegistryFinder < FileRegistryFinder
-    def initialize(current_node:)
-      @current_node = Geo::Fdw::GeoNode.find(current_node.id)
-    end
-
     def count_registry
       Geo::FileRegistry.attachments.count
     end

ee/app/finders/geo/job_artifact_registry_finder.rb

 # frozen_string_literal: true
 
 module Geo
-  class JobArtifactRegistryFinder < RegistryFinder
-    def initialize(current_node:)
-      @current_node = Geo::Fdw::GeoNode.find(current_node.id)
-    end
-
+  class JobArtifactRegistryFinder < FileRegistryFinder
     def count_registry
       Geo::JobArtifactRegistry.count
     end

ee/app/finders/geo/lfs_object_registry_finder.rb

@@ -2,10 +2,6 @@
 
 module Geo
   class LfsObjectRegistryFinder < FileRegistryFinder
-    def initialize(current_node:)
-      @current_node = Geo::Fdw::GeoNode.find(current_node.id)
-    end
-
     def count_registry
       Geo::FileRegistry.lfs_objects.count
     end

ee/app/finders/geo/registry_finder.rb

@@ -2,66 +2,22 @@
 
 module Geo
   class RegistryFinder
-    attr_reader :current_node
+    include ::Gitlab::Utils::StrongMemoize
 
-    delegate :selective_sync?, to: :current_node, allow_nil: true
-
-    def initialize(current_node: nil)
-      @current_node = current_node
-    end
-
-    protected
-
-    # When this feature isn't present, FDW queries pull every row from the
-    # remote database and perform aggregates locally, leading to surprisingly
-    # slow COUNT queries on large tables. For more details, see this link:
-    # https://www.enterprisedb.com/blog/postgresql-aggregate-push-down-postgresfdw
-    def aggregate_pushdown_supported?
-      Gitlab::Geo::Fdw.enabled? && Gitlab::Database.version.to_f >= 10.0
-    end
-
-    def use_legacy_queries?
-      # Selective project replication adds a wrinkle to FDW
-      # queries, so we fallback to the legacy version for now.
-      Gitlab::Geo::Fdw.disabled? || selective_sync?
-    end
+    attr_reader :current_node_id
 
-    def use_legacy_queries_for_selective_sync?
-      use_legacy_queries? && !Gitlab::Geo::Fdw.enabled_for_selective_sync?
-    end
-
-    # rubocop: disable CodeReuse/ActiveRecord
-    def legacy_inner_join_registry_ids(objects, registry_ids, klass, foreign_key: :id)
-      return klass.none if registry_ids.empty?
-
-      joined_relation = objects.joins(<<~SQL)
-        INNER JOIN
-        (VALUES #{registry_ids.map { |id| "(#{id})" }.join(',')})
-        registry(id)
-        ON #{klass.table_name}.#{foreign_key} = registry.id
-      SQL
+    delegate :selective_sync?, to: :current_node, allow_nil: true
 
-      joined_relation
+    def initialize(current_node_id: nil)
+      @current_node_id = current_node_id
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def legacy_left_outer_join_registry_ids(objects, registry_ids, klass)
-      return objects if registry_ids.empty?
-
-      joined_relation = objects.joins(<<~SQL)
-        LEFT OUTER JOIN
-        (VALUES #{registry_ids.map { |id| "(#{id}, 't')" }.join(',')})
-         registry(id, registry_present)
-         ON #{klass.table_name}.id = registry.id
-      SQL
-
-      joined_relation.where(registry: { registry_present: [nil, false] })
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
+    private
 
-    def quote_value(value)
-      ::Gitlab::SQL::Glob.q(value)
+    def current_node
+      strong_memoize(:current_node) do
+        Geo::Fdw::GeoNode.find(current_node_id) if current_node_id
+      end
     end
   end
 end

ee/app/models/geo_node_status.rb

@@ -369,15 +369,15 @@ class GeoNodeStatus < ApplicationRecord
   end
 
   def attachments_finder
-    @attachments_finder ||= Geo::AttachmentRegistryFinder.new(current_node: geo_node)
+    @attachments_finder ||= Geo::AttachmentRegistryFinder.new(current_node_id: geo_node.id)
   end
 
   def lfs_objects_finder
-    @lfs_objects_finder ||= Geo::LfsObjectRegistryFinder.new(current_node: geo_node)
+    @lfs_objects_finder ||= Geo::LfsObjectRegistryFinder.new(current_node_id: geo_node.id)
   end
 
   def job_artifacts_finder
-    @job_artifacts_finder ||= Geo::JobArtifactRegistryFinder.new(current_node: geo_node)
+    @job_artifacts_finder ||= Geo::JobArtifactRegistryFinder.new(current_node_id: geo_node.id)
   end
 
   def registries_for_synced_projects(type)

ee/app/workers/geo/file_download_dispatch_worker/attachment_job_finder.rb

@@ -6,7 +6,7 @@ module Geo
       EXCEPT_RESOURCE_IDS_KEY = :except_file_ids
 
       def registry_finder
-        @registry_finder ||= Geo::AttachmentRegistryFinder.new(current_node: Gitlab::Geo.current_node)
+        @registry_finder ||= Geo::AttachmentRegistryFinder.new(current_node_id: Gitlab::Geo.current_node.id)
       end
 
       private

ee/app/workers/geo/file_download_dispatch_worker/job_artifact_job_finder.rb

@@ -8,7 +8,7 @@ module Geo
       FILE_SERVICE_OBJECT_TYPE = :job_artifact
 
       def registry_finder
-        @registry_finder ||= Geo::JobArtifactRegistryFinder.new(current_node: Gitlab::Geo.current_node)
+        @registry_finder ||= Geo::JobArtifactRegistryFinder.new(current_node_id: Gitlab::Geo.current_node.id)
       end
     end
   end

ee/app/workers/geo/file_download_dispatch_worker/lfs_object_job_finder.rb

@@ -8,7 +8,7 @@ module Geo
       FILE_SERVICE_OBJECT_TYPE = :lfs
 
       def registry_finder
-        @registry_finder ||= Geo::LfsObjectRegistryFinder.new(current_node: Gitlab::Geo.current_node)
+        @registry_finder ||= Geo::LfsObjectRegistryFinder.new(current_node_id: Gitlab::Geo.current_node.id)
       end
     end
   end

ee/app/workers/geo/migrated_local_files_clean_up_worker.rb

@@ -94,15 +94,15 @@ module Geo
     end
 
     def attachments_finder
-      @attachments_finder ||= AttachmentRegistryFinder.new(current_node: current_node)
+      @attachments_finder ||= AttachmentRegistryFinder.new(current_node_id: current_node.id)
     end
 
     def lfs_objects_finder
-      @lfs_objects_finder ||= LfsObjectRegistryFinder.new(current_node: current_node)
+      @lfs_objects_finder ||= LfsObjectRegistryFinder.new(current_node_id: current_node.id)
     end
 
     def job_artifacts_finder
-      @job_artifacts_finder ||= JobArtifactRegistryFinder.new(current_node: current_node)
+      @job_artifacts_finder ||= JobArtifactRegistryFinder.new(current_node_id: current_node.id)
     end
   end
 end

ee/lib/gitlab/geo/fdw.rb

@@ -23,10 +23,6 @@ module Gitlab
           !enabled?
         end
 
-        def enabled_for_selective_sync?
-          enabled? && Feature.enabled?(:use_fdw_queries_for_selective_sync, default_enabled: true)
-        end
-
         # Return full table name with foreign schema
         #
         # @param [String] table_name

ee/spec/finders/geo/attachment_registry_finder_spec.rb

@@ -22,7 +22,7 @@ describe Geo::AttachmentRegistryFinder, :geo, :geo_fdw do
   let(:upload_4) { create(:upload, model: unsynced_project) }
   let(:upload_5) { create(:upload, model: synced_project) }
 
-  subject { described_class.new(current_node: secondary) }
+  subject { described_class.new(current_node_id: secondary.id) }
 
   before do
     stub_current_geo_node(secondary)
@@ -456,25 +456,5 @@ describe Geo::AttachmentRegistryFinder, :geo, :geo_fdw do
     end
   end
 
-  it 'responds to file registry finder methods' do
-    file_registry_finder_methods = %i{
-      syncable
-      count_syncable
-      count_synced
-      count_failed
-      count_synced_missing_on_primary
-      count_registry
-      find_unsynced
-      find_migrated_local
-      find_retryable_failed_registries
-      find_retryable_synced_missing_on_primary_registries
-    }
-
-    file_registry_finder_methods.each do |method|
-      expect(subject).to respond_to(method)
-    end
-  end
-
-  include_examples 'counts all the things'
-  include_examples 'finds all the things'
+  it_behaves_like 'a file registry finder'
 end

ee/spec/finders/geo/job_artifact_registry_finder_spec.rb

@@ -13,7 +13,7 @@ describe Geo::JobArtifactRegistryFinder, :geo_fdw do
   let(:unsynced_project) { create(:project) }
   let(:project_broken_storage) { create(:project, :broken_storage) }
 
-  subject { described_class.new(current_node: secondary) }
+  subject { described_class.new(current_node_id: secondary.id) }
 
   before do
     stub_current_geo_node(secondary)
@@ -563,25 +563,5 @@ describe Geo::JobArtifactRegistryFinder, :geo_fdw do
     end
   end
 
-  it 'responds to file registry finder methods' do
-    file_registry_finder_methods = %i{
-      syncable
-      count_syncable
-      count_synced
-      count_failed
-      count_synced_missing_on_primary
-      count_registry
-      find_unsynced
-      find_migrated_local
-      find_retryable_failed_registries
-      find_retryable_synced_missing_on_primary_registries
-    }
-
-    file_registry_finder_methods.each do |method|
-      expect(subject).to respond_to(method)
-    end
-  end
-
-  include_examples 'counts all the things'
-  include_examples 'finds all the things'
+  it_behaves_like 'a file registry finder'
 end

ee/spec/finders/geo/lfs_object_registry_finder_spec.rb

@@ -24,7 +24,7 @@ describe Geo::LfsObjectRegistryFinder, :geo_fdw do
   let(:lfs_object_remote_2) { create(:lfs_object, :object_storage) }
   let(:lfs_object_remote_3) { create(:lfs_object, :object_storage) }
 
-  subject { described_class.new(current_node: secondary) }
+  subject { described_class.new(current_node_id: secondary.id) }
 
   before do
     stub_current_geo_node(secondary)
@@ -642,25 +642,5 @@ describe Geo::LfsObjectRegistryFinder, :geo_fdw do
     end
   end
 
-  it 'responds to file registry finder methods' do
-    file_registry_finder_methods = %i{
-      syncable
-      count_syncable
-      count_synced
-      count_failed
-      count_synced_missing_on_primary
-      count_registry
-      find_unsynced
-      find_migrated_local
-      find_retryable_failed_registries
-      find_retryable_synced_missing_on_primary_registries
-    }
-
-    file_registry_finder_methods.each do |method|
-      expect(subject).to respond_to(method)
-    end
-  end
-
-  include_examples 'counts all the things'
-  include_examples 'finds all the things'
+  it_behaves_like 'a file registry finder'
 end

ee/spec/finders/geo/project_registry_status_finder_spec.rb

@@ -18,8 +18,6 @@ describe Geo::ProjectRegistryStatusFinder, :geo, :geo_tracking_db do
   set(:never_synced_registry) { create(:geo_project_registry) }
   set(:never_synced_registry_with_failure) { create(:geo_project_registry, :repository_sync_failed) }
 
-  subject { described_class.new(current_node: secondary) }
-
   before do
     stub_current_geo_node(secondary)
   end

ee/spec/lib/gitlab/geo/fdw_spec.rb

@@ -99,40 +99,6 @@ describe Gitlab::Geo::Fdw, :geo do
     end
   end
 
-  describe '.enabled_for_selective_sync?' do
-    context 'when the feature flag is enabled' do
-      before do
-        stub_feature_flags(use_fdw_queries_for_selective_sync: true)
-      end
-
-      it 'returns false when FDW is disabled' do
-        allow(described_class).to receive(:enabled?).and_return(false)
-
-        expect(described_class.enabled_for_selective_sync?).to eq false
-      end
-
-      it 'returns true when FDW is enabled' do
-        expect(described_class.enabled_for_selective_sync?).to eq true
-      end
-    end
-
-    context 'when the feature flag is disabled' do
-      before do
-        stub_feature_flags(use_fdw_queries_for_selective_sync: false)
-      end
-
-      it 'returns false when FDW is disabled' do
-        allow(described_class).to receive(:enabled?).and_return(false)
-
-        expect(described_class.enabled_for_selective_sync?).to eq false
-      end
-
-      it 'returns false when FDW is enabled' do
-        expect(described_class.enabled_for_selective_sync?).to eq false
-      end
-    end
-  end
-
   describe '.foreign_tables_up_to_date?' do
     it 'returns false when foreign schema does not exist' do
       drop_foreign_schema

ee/spec/support/shared_examples/finders/file_registry_finders.rb

@@ -18,34 +18,6 @@ shared_examples_for 'a file registry finder' do
     end
   end
 
-  # Disable transactions via :delete method because a foreign table
-  # can't see changes inside a transaction of a different connection.
-  context 'FDW', :geo_fdw, :delete do
-    context 'with use_fdw_queries_for_selective_sync disabled' do
-      before do
-        stub_feature_flags(use_fdw_queries_for_selective_sync: false)
-      end
-
-      include_examples 'counts all the things'
-      include_examples 'finds all the things'
-    end
-
-    context 'with use_fdw_queries_for_selective_sync enabled' do
-      before do
-        stub_feature_flags(use_fdw_queries_for_selective_sync: true)
-      end
-
-      include_examples 'counts all the things'
-      include_examples 'finds all the things'
-    end
-  end
-
-  context 'Legacy' do
-    before do
-      stub_fdw_disabled
-    end
-
-    include_examples 'counts all the things'
-    include_examples 'finds all the things'
-  end
+  include_examples 'counts all the things'
+  include_examples 'finds all the things'
 end