Merge branch '325725-skip-ordering-for-ci-shared-runners-cte-query' into 'master'

Add option to skip ordering when using DISTINCT

See merge request gitlab-org/gitlab!57312

ee/app/services/ee/ci/register_job_service.rb

@@ -75,7 +75,7 @@ module EE
       # rubocop: disable CodeReuse/ActiveRecord
       def all_namespaces
         namespaces = ::Namespace.reorder(nil).where('namespaces.id = projects.namespace_id')
-        ::Gitlab::ObjectHierarchy.new(namespaces).roots
+        ::Gitlab::ObjectHierarchy.new(namespaces, options: { skip_ordering: true }).roots
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

ee/changelogs/unreleased/325725-skip-ordering-for-ci-shared-runners-cte-query.yml

+---
+title: Skip ordering when using distinct in shared runners CTE query
+merge_request: 57312
+author:
+type: performance

lib/gitlab/object_hierarchy.rb

@@ -74,9 +74,16 @@ module Gitlab
           read_only(model.from(Arel::Nodes::As.new(recursive_query.arel, objects_table)).order(depth: hierarchy_order))
         else
           recursive_query = base_and_ancestors_cte(upto).apply_to(model.all)
-          recursive_query = recursive_query.reselect(*recursive_query.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
-          recursive_query = model.from(Arel::Nodes::As.new(recursive_query.arel, objects_table))
-          read_only(remove_depth_and_maintain_order(recursive_query, hierarchy_order: hierarchy_order))
+
+          if skip_ordering?
+            recursive_query = recursive_query.distinct
+          else
+            recursive_query = recursive_query.reselect(*recursive_query.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
+            recursive_query = model.from(Arel::Nodes::As.new(recursive_query.arel, objects_table))
+            recursive_query = remove_depth_and_maintain_order(recursive_query, hierarchy_order: hierarchy_order)
+          end
+
+          read_only(recursive_query)
         end
       else
         recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(model.all)
@@ -100,9 +107,16 @@ module Gitlab
           read_only(model.from(Arel::Nodes::As.new(base_cte.arel, objects_table)).order(depth: :asc))
         else
           base_cte = base_and_descendants_cte.apply_to(model.all)
-          base_cte = base_cte.reselect(*base_cte.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
-          base_cte = model.from(Arel::Nodes::As.new(base_cte.arel, objects_table))
-          read_only(remove_depth_and_maintain_order(base_cte, hierarchy_order: :asc))
+
+          if skip_ordering?
+            base_cte = base_cte.distinct
+          else
+            base_cte = base_cte.reselect(*base_cte.arel.projections, 'ROW_NUMBER() OVER () as depth').distinct
+            base_cte = model.from(Arel::Nodes::As.new(base_cte.arel, objects_table))
+            base_cte = remove_depth_and_maintain_order(base_cte, hierarchy_order: :asc)
+          end
+
+          read_only(base_cte)
         end
       else
         read_only(base_and_descendants_cte(with_depth: with_depth).apply_to(model.all))
@@ -173,6 +187,13 @@ module Gitlab
       false
     end
 
+    # Skips the extra ordering when using distinct on the namespace queries
+    def skip_ordering?
+      return options[:skip_ordering] if options.key?(:skip_ordering)
+
+      false
+    end
+
     # Remove the extra `depth` field using an INNER JOIN to avoid breaking UNION queries
     # and ordering the rows based on the `depth` column to maintain the row order.
     #

spec/lib/gitlab/object_hierarchy_spec.rb

@@ -3,14 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::ObjectHierarchy do
-  let!(:parent) { create(:group) }
-  let!(:child1) { create(:group, parent: parent) }
-  let!(:child2) { create(:group, parent: child1) }
+  let_it_be(:parent) { create(:group) }
+  let_it_be(:child1) { create(:group, parent: parent) }
+  let_it_be(:child2) { create(:group, parent: child1) }
+
+  let(:options) { {} }
 
   shared_context 'Gitlab::ObjectHierarchy test cases' do
     describe '#base_and_ancestors' do
       let(:relation) do
-        described_class.new(Group.where(id: child2.id)).base_and_ancestors
+        described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors
       end
 
       it 'includes the base rows' do
@@ -22,13 +24,13 @@ RSpec.describe Gitlab::ObjectHierarchy do
       end
 
       it 'can find ancestors upto a certain level' do
-        relation = described_class.new(Group.where(id: child2)).base_and_ancestors(upto: child1)
+        relation = described_class.new(Group.where(id: child2), options: options).base_and_ancestors(upto: child1)
 
         expect(relation).to contain_exactly(child2)
       end
 
       it 'uses ancestors_base #initialize argument' do
-        relation = described_class.new(Group.where(id: child2.id), Group.none).base_and_ancestors
+        relation = described_class.new(Group.where(id: child2.id), Group.none, options: options).base_and_ancestors
 
         expect(relation).to include(parent, child1, child2)
       end
@@ -40,7 +42,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
       describe 'hierarchy_order option' do
         let(:relation) do
-          described_class.new(Group.where(id: child2.id)).base_and_ancestors(hierarchy_order: hierarchy_order)
+          described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors(hierarchy_order: hierarchy_order)
         end
 
         context ':asc' do
@@ -63,7 +65,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#base_and_descendants' do
       let(:relation) do
-        described_class.new(Group.where(id: parent.id)).base_and_descendants
+        described_class.new(Group.where(id: parent.id), options: options).base_and_descendants
       end
 
       it 'includes the base rows' do
@@ -75,7 +77,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
       end
 
       it 'uses descendants_base #initialize argument' do
-        relation = described_class.new(Group.none, Group.where(id: parent.id)).base_and_descendants
+        relation = described_class.new(Group.none, Group.where(id: parent.id), options: options).base_and_descendants
 
         expect(relation).to include(parent, child1, child2)
       end
@@ -87,7 +89,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
       context 'when with_depth is true' do
         let(:relation) do
-          described_class.new(Group.where(id: parent.id)).base_and_descendants(with_depth: true)
+          described_class.new(Group.where(id: parent.id), options: options).base_and_descendants(with_depth: true)
         end
 
         it 'includes depth in the results' do
@@ -106,14 +108,14 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#descendants' do
       it 'includes only the descendants' do
-        relation = described_class.new(Group.where(id: parent)).descendants
+        relation = described_class.new(Group.where(id: parent), options: options).descendants
 
         expect(relation).to contain_exactly(child1, child2)
       end
     end
 
     describe '#max_descendants_depth' do
-      subject { described_class.new(base_relation).max_descendants_depth }
+      subject { described_class.new(base_relation, options: options).max_descendants_depth }
 
       context 'when base relation is empty' do
         let(:base_relation) { Group.where(id: nil) }
@@ -136,13 +138,13 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#ancestors' do
       it 'includes only the ancestors' do
-        relation = described_class.new(Group.where(id: child2)).ancestors
+        relation = described_class.new(Group.where(id: child2), options: options).ancestors
 
         expect(relation).to contain_exactly(child1, parent)
       end
 
       it 'can find ancestors upto a certain level' do
-        relation = described_class.new(Group.where(id: child2)).ancestors(upto: child1)
+        relation = described_class.new(Group.where(id: child2), options: options).ancestors(upto: child1)
 
         expect(relation).to be_empty
       end
@@ -150,7 +152,7 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
     describe '#all_objects' do
       let(:relation) do
-        described_class.new(Group.where(id: child1.id)).all_objects
+        described_class.new(Group.where(id: child1.id), options: options).all_objects
       end
 
       it 'includes the base rows' do
@@ -166,13 +168,13 @@ RSpec.describe Gitlab::ObjectHierarchy do
       end
 
       it 'uses ancestors_base #initialize argument for ancestors' do
-        relation = described_class.new(Group.where(id: child1.id), Group.where(id: non_existing_record_id)).all_objects
+        relation = described_class.new(Group.where(id: child1.id), Group.where(id: non_existing_record_id), options: options).all_objects
 
         expect(relation).to include(parent)
       end
 
       it 'uses descendants_base #initialize argument for descendants' do
-        relation = described_class.new(Group.where(id: non_existing_record_id), Group.where(id: child1.id)).all_objects
+        relation = described_class.new(Group.where(id: non_existing_record_id), Group.where(id: child1.id), options: options).all_objects
 
         expect(relation).to include(child2)
       end
@@ -210,6 +212,19 @@ RSpec.describe Gitlab::ObjectHierarchy do
       expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
       expect(child2.self_and_ancestors.to_sql).to include("DISTINCT")
     end
+
+    context 'when the skip_ordering option is set' do
+      let(:options) { { skip_ordering: true } }
+
+      it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+
+      it 'does not include ROW_NUMBER()' do
+        query = described_class.new(Group.where(id: parent.id), options: options).base_and_descendants.to_sql
+
+        expect(query).to include("DISTINCT")
+        expect(query).not_to include("ROW_NUMBER()")
+      end
+    end
   end
 
   context 'when the use_distinct_in_object_hierarchy feature flag is disabled' do