Merge branch 'undo-cte-hack' into 'master'

Undo CTE fix for PG11 [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!58499

app/models/namespaces/traversal/recursive.rb

@@ -78,7 +78,7 @@ module Namespaces
       alias_method :recursive_self_and_descendant_ids, :self_and_descendant_ids
 
       def object_hierarchy(ancestors_base)
-        Gitlab::ObjectHierarchy.new(ancestors_base, options: { use_distinct: Feature.enabled?(:use_distinct_in_object_hierarchy, self) })
+        Gitlab::ObjectHierarchy.new(ancestors_base)
       end
     end
   end

app/services/ci/queue/build_queue_service.rb

@@ -28,7 +28,7 @@ module Ci
         groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
 
         hierarchy_groups = Gitlab::ObjectHierarchy
-          .new(groups, options: { use_distinct: ::Feature.enabled?(:use_distinct_in_register_job_object_hierarchy) })
+          .new(groups)
           .base_and_descendants
 
         projects = Project.where(namespace_id: hierarchy_groups)

config/feature_flags/development/use_distinct_for_all_object_hierarchy.yml

----
-name: use_distinct_for_all_object_hierarchy
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57052
-rollout_issue_url: 
-milestone: '13.11'
-type: development
-group: group::database
-default_enabled: false

config/feature_flags/development/use_distinct_in_object_hierarchy.yml

----
-name: use_distinct_in_object_hierarchy
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56509
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/324644
-milestone: '13.10'
-type: development
-group: group::optimize
-default_enabled: false

config/feature_flags/development/use_distinct_in_register_job_object_hierarchy.yml

----
-name: use_distinct_in_register_job_object_hierarchy
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57045
-rollout_issue_url:
-milestone: '13.11'
-type: development
-group: group::pipeline execution
-default_enabled: false

ee/app/services/ee/ci/queue/build_queue_service.rb

@@ -47,7 +47,7 @@ module EE
               .joins('INNER JOIN namespaces as project_namespaces ON project_namespaces.id = projects.namespace_id')
           else
             namespaces = ::Namespace.reorder(nil).where('namespaces.id = projects.namespace_id')
-            ::Gitlab::ObjectHierarchy.new(namespaces, options: { skip_ordering: true }).roots
+            ::Gitlab::ObjectHierarchy.new(namespaces).roots
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord

lib/gitlab/object_hierarchy.rb

@@ -65,32 +65,9 @@ module Gitlab
     # Note: By default the order is breadth-first
     # rubocop: disable CodeReuse/ActiveRecord
     def base_and_ancestors(upto: nil, hierarchy_order: nil)
-      if use_distinct?
-        expose_depth = hierarchy_order.present?
-        hierarchy_order ||= :asc
-
-        # if hierarchy_order is given, the calculated `depth` should be present in SELECT
-        if expose_depth
-          recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(unscoped_model.all).distinct
-          read_only(unscoped_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(unscoped_model.all)
-
-          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 = unscoped_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(unscoped_model.all)
-        recursive_query = recursive_query.order(depth: hierarchy_order) if hierarchy_order
-        read_only(recursive_query)
-      end
+      recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(unscoped_model.all)
+      recursive_query = recursive_query.order(depth: hierarchy_order) if hierarchy_order
+      read_only(recursive_query)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -101,27 +78,7 @@ module Gitlab
     # and incremented as we go down the descendant tree
     # rubocop: disable CodeReuse/ActiveRecord
     def base_and_descendants(with_depth: false)
-      if use_distinct?
-        # Always calculate `depth`, remove it later if with_depth is false
-        if with_depth
-          base_cte = base_and_descendants_cte(with_depth: true).apply_to(unscoped_model.all).distinct
-          read_only(unscoped_model.from(Arel::Nodes::As.new(base_cte.arel, objects_table)).order(depth: :asc))
-        else
-          base_cte = base_and_descendants_cte.apply_to(unscoped_model.all)
-
-          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 = unscoped_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(unscoped_model.all))
-      end
+      read_only(base_and_descendants_cte(with_depth: with_depth).apply_to(unscoped_model.all))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -158,11 +115,6 @@ module Gitlab
       ancestors_scope = unscoped_model.from(ancestors_table)
       descendants_scope = unscoped_model.from(descendants_table)
 
-      if use_distinct?
-        ancestors_scope = ancestors_scope.distinct
-        descendants_scope = descendants_scope.distinct
-      end
-
       relation = unscoped_model
         .with
         .recursive(ancestors.to_arel, descendants.to_arel)
@@ -177,23 +129,6 @@ module Gitlab
 
     private
 
-    # Use distinct on the Namespace queries to avoid bad planner behavior in PG11.
-    def use_distinct?
-      return unless model <= Namespace
-      # Global use_distinct_for_all_object_hierarchy takes precedence over use_distinct_in_object_hierarchy
-      return true if Feature.enabled?(:use_distinct_for_all_object_hierarchy)
-      return options[:use_distinct] if options.key?(:use_distinct)
-
-      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

@@ -9,265 +9,178 @@ RSpec.describe Gitlab::ObjectHierarchy do
 
   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), options: options).base_and_ancestors
-      end
-
-      it 'includes the base rows' do
-        expect(relation).to include(child2)
-      end
-
-      it 'includes all of the ancestors' do
-        expect(relation).to include(parent, child1)
-      end
-
-      it 'can find ancestors upto a certain level' do
-        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, options: options).base_and_ancestors
+  describe '#base_and_ancestors' do
+    let(:relation) do
+      described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors
+    end
 
-        expect(relation).to include(parent, child1, child2)
-      end
+    it 'includes the base rows' do
+      expect(relation).to include(child2)
+    end
 
-      it 'does not allow the use of #update_all' do
-        expect { relation.update_all(share_with_group_lock: false) }
-          .to raise_error(ActiveRecord::ReadOnlyRecord)
-      end
+    it 'includes all of the ancestors' do
+      expect(relation).to include(parent, child1)
+    end
 
-      describe 'hierarchy_order option' do
-        let(:relation) do
-          described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors(hierarchy_order: hierarchy_order)
-        end
+    it 'can find ancestors upto a certain level' do
+      relation = described_class.new(Group.where(id: child2), options: options).base_and_ancestors(upto: child1)
 
-        context ':asc' do
-          let(:hierarchy_order) { :asc }
+      expect(relation).to contain_exactly(child2)
+    end
 
-          it 'orders by child to parent' do
-            expect(relation).to eq([child2, child1, parent])
-          end
-        end
+    it 'uses ancestors_base #initialize argument' do
+      relation = described_class.new(Group.where(id: child2.id), Group.none, options: options).base_and_ancestors
 
-        context ':desc' do
-          let(:hierarchy_order) { :desc }
+      expect(relation).to include(parent, child1, child2)
+    end
 
-          it 'orders by parent to child' do
-            expect(relation).to eq([parent, child1, child2])
-          end
-        end
-      end
+    it 'does not allow the use of #update_all' do
+      expect { relation.update_all(share_with_group_lock: false) }
+        .to raise_error(ActiveRecord::ReadOnlyRecord)
     end
 
-    describe '#base_and_descendants' do
+    describe 'hierarchy_order option' do
       let(:relation) do
-        described_class.new(Group.where(id: parent.id), options: options).base_and_descendants
-      end
-
-      it 'includes the base rows' do
-        expect(relation).to include(parent)
-      end
-
-      it 'includes all the descendants' do
-        expect(relation).to include(child1, child2)
+        described_class.new(Group.where(id: child2.id), options: options).base_and_ancestors(hierarchy_order: hierarchy_order)
       end
 
-      it 'uses descendants_base #initialize argument' do
-        relation = described_class.new(Group.none, Group.where(id: parent.id), options: options).base_and_descendants
+      context ':asc' do
+        let(:hierarchy_order) { :asc }
 
-        expect(relation).to include(parent, child1, child2)
-      end
-
-      it 'does not allow the use of #update_all' do
-        expect { relation.update_all(share_with_group_lock: false) }
-          .to raise_error(ActiveRecord::ReadOnlyRecord)
-      end
-
-      context 'when with_depth is true' do
-        let(:relation) do
-          described_class.new(Group.where(id: parent.id), options: options).base_and_descendants(with_depth: true)
+        it 'orders by child to parent' do
+          expect(relation).to eq([child2, child1, parent])
         end
+      end
 
-        it 'includes depth in the results' do
-          object_depths = {
-            parent.id => 1,
-            child1.id => 2,
-            child2.id => 3
-          }
+      context ':desc' do
+        let(:hierarchy_order) { :desc }
 
-          relation.each do |object|
-            expect(object.depth).to eq(object_depths[object.id])
-          end
+        it 'orders by parent to child' do
+          expect(relation).to eq([parent, child1, child2])
         end
       end
     end
+  end
 
-    describe '#descendants' do
-      it 'includes only the descendants' do
-        relation = described_class.new(Group.where(id: parent), options: options).descendants
-
-        expect(relation).to contain_exactly(child1, child2)
-      end
+  describe '#base_and_descendants' do
+    let(:relation) do
+      described_class.new(Group.where(id: parent.id), options: options).base_and_descendants
     end
 
-    describe '#max_descendants_depth' do
-      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) }
-
-        it { expect(subject).to be_nil }
-      end
-
-      context 'when base has no children' do
-        let(:base_relation) { Group.where(id: child2) }
-
-        it { expect(subject).to eq(1) }
-      end
-
-      context 'when base has grandchildren' do
-        let(:base_relation) { Group.where(id: parent) }
-
-        it { expect(subject).to eq(3) }
-      end
+    it 'includes the base rows' do
+      expect(relation).to include(parent)
     end
 
-    describe '#ancestors' do
-      it 'includes only the ancestors' do
-        relation = described_class.new(Group.where(id: child2), options: options).ancestors
+    it 'includes all the descendants' do
+      expect(relation).to include(child1, child2)
+    end
 
-        expect(relation).to contain_exactly(child1, parent)
-      end
+    it 'uses descendants_base #initialize argument' do
+      relation = described_class.new(Group.none, Group.where(id: parent.id), options: options).base_and_descendants
 
-      it 'can find ancestors upto a certain level' do
-        relation = described_class.new(Group.where(id: child2), options: options).ancestors(upto: child1)
+      expect(relation).to include(parent, child1, child2)
+    end
 
-        expect(relation).to be_empty
-      end
+    it 'does not allow the use of #update_all' do
+      expect { relation.update_all(share_with_group_lock: false) }
+        .to raise_error(ActiveRecord::ReadOnlyRecord)
     end
 
-    describe '#all_objects' do
+    context 'when with_depth is true' do
       let(:relation) do
-        described_class.new(Group.where(id: child1.id), options: options).all_objects
+        described_class.new(Group.where(id: parent.id), options: options).base_and_descendants(with_depth: true)
       end
 
-      it 'includes the base rows' do
-        expect(relation).to include(child1)
-      end
-
-      it 'includes the ancestors' do
-        expect(relation).to include(parent)
-      end
+      it 'includes depth in the results' do
+        object_depths = {
+          parent.id => 1,
+          child1.id => 2,
+          child2.id => 3
+        }
 
-      it 'includes the descendants' do
-        expect(relation).to include(child2)
-      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), options: options).all_objects
-
-        expect(relation).to include(parent)
+        relation.each do |object|
+          expect(object.depth).to eq(object_depths[object.id])
+        end
       end
+    end
+  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), options: options).all_objects
-
-        expect(relation).to include(child2)
-      end
+  describe '#descendants' do
+    it 'includes only the descendants' do
+      relation = described_class.new(Group.where(id: parent), options: options).descendants
 
-      it 'does not allow the use of #update_all' do
-        expect { relation.update_all(share_with_group_lock: false) }
-          .to raise_error(ActiveRecord::ReadOnlyRecord)
-      end
+      expect(relation).to contain_exactly(child1, child2)
     end
   end
 
-  context 'when the use_distinct_in_object_hierarchy feature flag is enabled' do
-    before do
-      stub_feature_flags(use_distinct_in_object_hierarchy: true)
-      stub_feature_flags(use_distinct_for_all_object_hierarchy: false)
-    end
+  describe '#max_descendants_depth' do
+    subject { described_class.new(base_relation, options: options).max_descendants_depth }
 
-    it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+    context 'when base relation is empty' do
+      let(:base_relation) { Group.where(id: nil) }
 
-    it 'calls DISTINCT' do
-      expect(child2.self_and_ancestors.to_sql).to include("DISTINCT")
+      it { expect(subject).to be_nil }
     end
 
-    context 'when use_traversal_ids feature flag is enabled' do
-      it 'does not call DISTINCT' do
-        expect(parent.self_and_descendants.to_sql).not_to include("DISTINCT")
-      end
+    context 'when base has no children' do
+      let(:base_relation) { Group.where(id: child2) }
+
+      it { expect(subject).to eq(1) }
     end
 
-    context 'when use_traversal_ids feature flag is disabled' do
-      before do
-        stub_feature_flags(use_traversal_ids: false)
-      end
+    context 'when base has grandchildren' do
+      let(:base_relation) { Group.where(id: parent) }
 
-      it 'calls DISTINCT' do
-        expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
-      end
+      it { expect(subject).to eq(3) }
     end
   end
 
-  context 'when the use_distinct_for_all_object_hierarchy feature flag is enabled' do
-    before do
-      stub_feature_flags(use_distinct_in_object_hierarchy: false)
-      stub_feature_flags(use_distinct_for_all_object_hierarchy: true)
+  describe '#ancestors' do
+    it 'includes only the ancestors' do
+      relation = described_class.new(Group.where(id: child2), options: options).ancestors
+
+      expect(relation).to contain_exactly(child1, parent)
     end
 
-    it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+    it 'can find ancestors upto a certain level' do
+      relation = described_class.new(Group.where(id: child2), options: options).ancestors(upto: child1)
 
-    it 'calls DISTINCT' do
-      expect(child2.self_and_ancestors.to_sql).to include("DISTINCT")
+      expect(relation).to be_empty
     end
+  end
 
-    context 'when use_traversal_ids feature flag is enabled' do
-      it 'does not call DISTINCT' do
-        expect(parent.self_and_descendants.to_sql).not_to include("DISTINCT")
-      end
+  describe '#all_objects' do
+    let(:relation) do
+      described_class.new(Group.where(id: child1.id), options: options).all_objects
     end
 
-    context 'when use_traversal_ids feature flag is disabled' do
-      before do
-        stub_feature_flags(use_traversal_ids: false)
-      end
-
-      it 'calls DISTINCT' do
-        expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
-      end
+    it 'includes the base rows' do
+      expect(relation).to include(child1)
+    end
 
-      context 'when the skip_ordering option is set' do
-        let(:options) { { skip_ordering: true } }
+    it 'includes the ancestors' do
+      expect(relation).to include(parent)
+    end
 
-        it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+    it 'includes the descendants' do
+      expect(relation).to include(child2)
+    end
 
-        it 'does not include ROW_NUMBER()' do
-          query = described_class.new(Group.where(id: parent.id), options: options).base_and_descendants.to_sql
+    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), options: options).all_objects
 
-          expect(query).to include("DISTINCT")
-          expect(query).not_to include("ROW_NUMBER()")
-        end
-      end
+      expect(relation).to include(parent)
     end
-  end
 
-  context 'when the use_distinct_in_object_hierarchy feature flag is disabled' do
-    before do
-      stub_feature_flags(use_distinct_in_object_hierarchy: false)
-      stub_feature_flags(use_distinct_for_all_object_hierarchy: false)
-    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), options: options).all_objects
 
-    it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+      expect(relation).to include(child2)
+    end
 
-    it 'does not call DISTINCT' do
-      expect(parent.self_and_descendants.to_sql).not_to include("DISTINCT")
-      expect(child2.self_and_ancestors.to_sql).not_to include("DISTINCT")
+    it 'does not allow the use of #update_all' do
+      expect { relation.update_all(share_with_group_lock: false) }
+        .to raise_error(ActiveRecord::ReadOnlyRecord)
     end
   end
 end

spec/services/ci/register_job_service_spec.rb

@@ -294,32 +294,6 @@ module Ci
             end
           end
 
-          context 'when the use_distinct_in_register_job_object_hierarchy feature flag is enabled' do
-            before do
-              stub_feature_flags(use_distinct_in_register_job_object_hierarchy: true)
-              stub_feature_flags(use_distinct_for_all_object_hierarchy: true)
-            end
-
-            it 'calls DISTINCT' do
-              queue = ::Ci::Queue::BuildQueueService.new(group_runner)
-
-              expect(queue.builds_for_group_runner.to_sql).to include("DISTINCT")
-            end
-          end
-
-          context 'when the use_distinct_in_register_job_object_hierarchy feature flag is disabled' do
-            before do
-              stub_feature_flags(use_distinct_in_register_job_object_hierarchy: false)
-              stub_feature_flags(use_distinct_for_all_object_hierarchy: false)
-            end
-
-            it 'does not call DISTINCT' do
-              queue = ::Ci::Queue::BuildQueueService.new(group_runner)
-
-              expect(queue.builds_for_group_runner.to_sql).not_to include("DISTINCT")
-            end
-          end
-
           context 'group runner' do
             let(:build) { execute(group_runner) }