Optimize DISTINCT queries (#4129)

philippthun · web-flow · commit af5c1e8aedaf · 2025-01-10T11:31:25.000+01:00
The DISTINCT clause instructs the database to ensure that all returned
rows are distinct; so the database compares all returned columns. In
most cases we simply want to prevent duplicate entries in the result set
which can be achieved by a DISTINCT ON (id) clause that does the
comparison on the 'id' column only and is much more efficient (i.e. for
large final or even intermediate result sets the database might need to
write them to disk to perform the comparison).

MySQL does not support DISTINCT ON, but Sequel translates this
transparently to GROUP BY.

PostgreSQL has the following restriction: SELECT DISTINCT ON expressions
must match initial ORDER BY expressions. Thus the SequelPaginator has
been enhanced to use the ORDER BY columns also for the DISTINCT ON
clause if they are unique (i.e. 'id' or 'guid' included). In case the
ORDER BY clause is not unique, DISTINCT is used instead.

DISTINCT statements with a single selected column have not been changed.
diff --git a/app/controllers/v3/spaces_controller.rb b/app/controllers/v3/spaces_controller.rb
@@ -228,13 +228,13 @@ def fetch_isolation_segment(guid)
   def fetch_running_security_group_guids(space)
     space_level_groups = SecurityGroup.where(spaces: space)
     global_groups = SecurityGroup.where(running_default: true)
-    space_level_groups.union(global_groups).map(&:guid)
+    space_level_groups.union(global_groups).select_map(:guid)
   end
 
   def fetch_staging_security_group_guids(space)
     space_level_groups = SecurityGroup.where(staging_spaces: space)
     global_groups = SecurityGroup.where(staging_default: true)
-    space_level_groups.union(global_groups).distinct.map(&:guid)
+    space_level_groups.union(global_groups).distinct.select_map(:guid)
   end
 
   def space_not_found!
diff --git a/app/decorators/field_service_instance_broker_decorator.rb b/app/decorators/field_service_instance_broker_decorator.rb
@@ -21,7 +21,7 @@ def decorate(hash, service_instances)
                 join(:service_plans, service_id: :services__id).
                 join(:service_instances, service_plan_id: :service_plans__id).
                 where(service_instances__id: managed_service_instances.map(&:id)).
-                distinct.
+                distinct(:service_brokers__created_at, :service_brokers__guid).
                 order_by(:service_brokers__created_at, :service_brokers__guid).
                 select(:service_brokers__name, :service_brokers__guid, :service_brokers__created_at).
                 all
diff --git a/app/decorators/field_service_instance_offering_decorator.rb b/app/decorators/field_service_instance_offering_decorator.rb
@@ -20,7 +20,7 @@ def decorate(hash, service_instances)
                   join(:service_plans, service_id: :services__id).
                   join(:service_instances, service_plan_id: :service_plans__id).
                   where(service_instances__id: managed_service_instances.map(&:id)).
-                  distinct.
+                  distinct(:services__created_at, :services__guid).
                   order_by(:services__created_at, :services__guid).
                   select(:services__label, :services__guid, :services__description, :services__tags, :services__extra, :services__service_broker_id, :services__created_at).
                   all
diff --git a/app/fetchers/base_service_list_fetcher.rb b/app/fetchers/base_service_list_fetcher.rb
@@ -161,7 +161,7 @@ def distinct_union(_dataset)
       def join_plan_org_visibilities(dataset)
         dataset = join_service_plans(dataset)
         dataset = join(dataset, :inner, :service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id])
-        dataset.distinct # service plans can have multiple visibilities
+        distinct(dataset) # service plans can have multiple visibilities
       end
 
       def join_service_brokers(dataset)
@@ -187,13 +187,13 @@ def join_broker_orgs(dataset)
       def join_plan_spaces(dataset)
         dataset = join_plan_orgs(dataset)
         dataset = join(dataset, :inner, Sequel[:spaces].as(:plan_spaces), organization_id: Sequel[:plan_orgs][:id])
-        dataset.distinct # organizations can have multiple spaces
+        distinct(dataset) # organizations can have multiple spaces
       end
 
       def join_service_instances(dataset)
         dataset = join_service_plans(dataset)
         dataset = join(dataset, :inner, :service_instances, service_plan_id: Sequel[:service_plans][:id])
-        dataset.distinct # service plans can have multiple instances
+        distinct(dataset) # service plans can have multiple instances
       end
 
       def join(dataset, type, table, on)
@@ -202,6 +202,10 @@ def join(dataset, type, table, on)
         end
         dataset.join_table(type, table, on)
       end
+
+      def distinct(dataset)
+        dataset.distinct(Sequel.qualify(dataset.model.table_name, :id))
+      end
     end
   end
 end
diff --git a/app/fetchers/organization_quota_list_fetcher.rb b/app/fetchers/organization_quota_list_fetcher.rb
@@ -23,7 +23,7 @@ def filter(message, dataset, readable_org_guids_query=nil)
         if message.requested? :organization_guids
           dataset = dataset.
                     join(:organizations, quota_definition_id: :id).
-                    where(organizations__guid: message.organization_guids).distinct.
+                    where(organizations__guid: message.organization_guids).distinct(:id).
                     qualify(:quota_definitions)
 
           dataset = dataset.where(organizations__guid: readable_org_guids_query) if readable_org_guids_query
diff --git a/app/fetchers/service_instance_list_fetcher.rb b/app/fetchers/service_instance_list_fetcher.rb
@@ -23,7 +23,7 @@ def fetch(message, omniscient: false, readable_spaces_dataset: nil, eager_loaded
 
         filter(dataset, message).
           select_all(:service_instances).
-          distinct
+          distinct(:service_instances__id)
       end
 
       private
diff --git a/app/fetchers/service_offering_list_fetcher.rb b/app/fetchers/service_offering_list_fetcher.rb
@@ -33,7 +33,7 @@ def filter(message, dataset, klass)
 
       def join_service_plans(dataset)
         dataset = join(dataset, :inner, :service_plans, service_id: Sequel[:services][:id])
-        dataset.distinct # services can have multiple plans
+        distinct(dataset) # services can have multiple plans
       end
 
       def join_services(dataset)
@@ -43,7 +43,7 @@ def join_services(dataset)
       def distinct_union(dataset)
         # The UNIONed :services datasets (permissions granted on org level for plans / permissions
         # granted on space level for brokers / public plans) might contain duplicate entries.
-        dataset.distinct
+        distinct(dataset)
       end
     end
   end
diff --git a/app/fetchers/service_plan_visibility_fetcher.rb b/app/fetchers/service_plan_visibility_fetcher.rb
@@ -24,7 +24,7 @@ def orgs_query(service_plan_guids:)
 
       dataset.
         select_all(:organizations).
-        distinct.
+        distinct(:organizations__id).
         order_by(:id)
     end
   end
diff --git a/app/fetchers/space_quota_list_fetcher.rb b/app/fetchers/space_quota_list_fetcher.rb
@@ -25,7 +25,7 @@ def filter(message, dataset, readable_space_quota_guids)
         if message.requested? :space_guids
           dataset = dataset.
                     join(:spaces, space_quota_definition_id: :id).
-                    where(Sequel[:spaces][:guid] => message.space_guids).distinct.
+                    where(Sequel[:spaces][:guid] => message.space_guids).distinct(:id).
                     qualify(:space_quota_definitions)
         end
 
diff --git a/app/jobs/runtime/prune_completed_builds.rb b/app/jobs/runtime/prune_completed_builds.rb
@@ -13,8 +13,8 @@ def perform
           logger.info('Cleaning up old builds')
 
           guids_for_apps_with_builds = BuildModel.
-                                       distinct(:app_guid).
-                                       map(&:app_guid)
+                                       distinct.
+                                       select_map(:app_guid)
 
           guids_for_apps_with_builds.each do |app_guid|
             builds_dataset = BuildModel.where(app_guid:)
diff --git a/app/jobs/runtime/prune_completed_deployments.rb b/app/jobs/runtime/prune_completed_deployments.rb
@@ -13,8 +13,8 @@ def perform
           logger.info('Cleaning up old deployments')
 
           guids_for_apps_with_deployments = DeploymentModel.
-                                            distinct(:app_guid).
-                                            map(&:app_guid)
+                                            distinct.
+                                            select_map(:app_guid)
 
           guids_for_apps_with_deployments.each do |app_guid|
             deployments_dataset = DeploymentModel.where(app_guid:)
diff --git a/lib/cloud_controller/paging/sequel_paginator.rb b/lib/cloud_controller/paging/sequel_paginator.rb
@@ -15,6 +15,18 @@ def get_page(dataset, pagination_options)
 
       dataset = dataset.order_append(Sequel.send(order_direction, Sequel.qualify(table_name, :guid))) if order_by != 'id' && has_guid_column
 
+      distinct_opt = dataset.opts[:distinct]
+      if !distinct_opt.nil? && !distinct_opt.empty? # DISTINCT ON
+        order_opt = dataset.opts[:order]
+        dataset = if order_opt.any? { |o| %i[id guid].include?(o.expression.column.to_sym) }
+                    # If ORDER BY columns are unique, use them for the DISTINCT ON clause.
+                    dataset.distinct(*order_opt.map(&:expression))
+                  else
+                    # Otherwise, use DISTINCT.
+                    dataset.distinct
+                  end
+      end
+
       records, count = if can_paginate_with_window_function?(dataset)
                          paginate_with_window_function(dataset, per_page, page, table_name)
                        else
diff --git a/spec/support/bootstrap/fake_model_tables.rb b/spec/support/bootstrap/fake_model_tables.rb
@@ -9,6 +9,7 @@ def create_tables
     tables_for_vcap_relations_spec
     tables_for_sequel_case_insensitive_string_monkeypatch
     tables_for_query_spec
+    tables_for_sequel_paginator_spec
   end
 
   private
@@ -18,6 +19,7 @@ def try_drop_tables
     drop_tables_for_vcap_relations_spec
     drop_tables_for_sequel_case_insensitive_string_monkeypatch
     drop_tables_for_query_spec
+    drop_tables_for_sequel_paginator_spec
   end
 
   def tables_for_model_controller_spec
@@ -273,5 +275,16 @@ def drop_tables_for_query_spec
     db.drop_table? :authors
   end
 
+  def tables_for_sequel_paginator_spec
+    db.create_table :table_without_guid do
+      primary_key :id
+      DateTime :created_at
+    end
+  end
+
+  def drop_tables_for_sequel_paginator_spec
+    db.drop_table? :table_without_guid
+  end
+
   attr_reader :db
 end
diff --git a/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb b/spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb
@@ -4,6 +4,8 @@
 
 module VCAP::CloudController
   RSpec.describe SequelPaginator do
+    class TableWithoutGuid < Sequel::Model(:table_without_guid); end
+
     let(:paginator) { SequelPaginator.new }
 
     describe '#get_page' do
@@ -153,15 +155,16 @@ module VCAP::CloudController
       end
 
       it 'does not order by GUID when the table has no GUID' do
-        options = { page:, per_page: }
+        options = { page: page, per_page: 2, order_by: 'created_at', order_direction: 'asc' }
+
         pagination_options = PaginationOptions.new(options)
-        orphaned_blob_dataset = OrphanedBlob.dataset
-        OrphanedBlob.make.save
-        paginated_result = nil
+        ds = TableWithoutGuid.dataset
         expect do
-          paginated_result = paginator.get_page(orphaned_blob_dataset, pagination_options)
-        end.not_to raise_error
-        expect(paginated_result.total).to be 1
+          paginator.get_page(ds, pagination_options)
+        end.to have_queried_db_times(/ORDER BY .\w*.\..created_at. ASC LIMIT/i, 1)
+        expect do
+          paginator.get_page(ds, pagination_options)
+        end.to have_queried_db_times(/ORDER BY .\w*.\..created_at. ASC, .\w*.\..guid. ASC LIMIT/i, 0)
       end
 
       it 'does not order by GUID when the primary order is by ID' do
@@ -176,6 +179,40 @@ module VCAP::CloudController
         end.to have_queried_db_times(/ORDER BY .\w*.\..id. ASC, .\w*.\..guid. ASC LIMIT/i, 0)
       end
 
+      context 'when a DISTINCT ON clause is used' do # MySQL uses GROUP BY instead
+        let(:distinct_dataset) { dataset.distinct(:id) }
+
+        context 'when ordered by ID' do
+          let(:pagination_options) { PaginationOptions.new({ order_by: 'id' }) }
+
+          it 'uses column ID for DISTINCT ON clause' do
+            expect do
+              paginator.get_page(distinct_dataset, pagination_options)
+            end.to have_queried_db_times(/(select distinct on \(.*id.*\) .* from)|(group by)/i, paginator.can_paginate_with_window_function?(dataset) ? 1 : 2)
+          end
+        end
+
+        context 'when ordered by other column' do
+          let(:pagination_options) { PaginationOptions.new({ order_by: 'created_at' }) }
+
+          it 'uses other column and GUID for DISTINCT ON clause' do
+            expect do
+              paginator.get_page(distinct_dataset, pagination_options)
+            end.to have_queried_db_times(/(select distinct on \(.*created_at.*,.*guid.*\) .* from)|(group by)/i, paginator.can_paginate_with_window_function?(dataset) ? 1 : 2)
+          end
+
+          context 'when table has no GUID column' do
+            let(:dataset) { TableWithoutGuid.dataset }
+
+            it 'uses a DISTINCT clause instead' do
+              expect do
+                paginator.get_page(distinct_dataset, pagination_options)
+              end.to have_queried_db_times(/(select distinct (?!on).* from)|(group by)/i, paginator.can_paginate_with_window_function?(dataset) ? 1 : 2)
+            end
+          end
+        end
+      end
+
       it 'produces a descending order which is exactly the reverse order of the ascending ordering' do
         app_model1.update(guid: '1', created_at: '2019-12-25T13:00:00Z')
         app_model2.update(guid: '2', created_at: '2019-12-25T13:00:01Z')
