Merge branch 'use-for-no-key-update-lock' into 'master'

Use the for no key update lock when upadting traversal ids

See merge request gitlab-org/gitlab!81239

app/models/namespace/traversal_hierarchy.rb

@@ -31,15 +31,21 @@ class Namespace
       # ActiveRecord. https://github.com/rails/rails/issues/13496
       # Ideally it would be:
       #   `incorrect_traversal_ids.update_all('traversal_ids = cte.traversal_ids')`
-      sql = """
-            UPDATE namespaces
-            SET traversal_ids = cte.traversal_ids
-            FROM (#{recursive_traversal_ids}) as cte
-            WHERE namespaces.id = cte.id
-              AND namespaces.traversal_ids::bigint[] <> cte.traversal_ids
-            """
+      sql = <<-SQL
+        UPDATE namespaces
+        SET traversal_ids = cte.traversal_ids
+        FROM (#{recursive_traversal_ids}) as cte
+        WHERE namespaces.id = cte.id
+          AND namespaces.traversal_ids::bigint[] <> cte.traversal_ids
+      SQL
+
       Namespace.transaction do
-        @root.lock!
+        if Feature.enabled?(:for_no_key_update_lock, default_enabled: :yaml)
+          @root.lock!("FOR NO KEY UPDATE")
+        else
+          @root.lock!
+        end
+
         Namespace.connection.exec_query(sql)
       end
     rescue ActiveRecord::Deadlocked

config/feature_flags/development/for_no_key_update_lock.yml

+---
+name: for_no_key_update_lock
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/81239
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353619
+milestone: '14.9'
+type: development
+group: group::workspaces
+default_enabled: false

spec/models/group_spec.rb

@@ -385,23 +385,43 @@ RSpec.describe Group do
         end
       end
 
-      before do
-        subject
-        reload_models(old_parent, new_parent, group)
-      end
-
       context 'within the same hierarchy' do
         let!(:root) { create(:group).reload }
         let!(:old_parent) { create(:group, parent: root) }
         let!(:new_parent) { create(:group, parent: root) }
 
-        it 'updates traversal_ids' do
-          expect(group.traversal_ids).to eq [root.id, new_parent.id, group.id]
+        context 'with FOR UPDATE lock' do
+          before do
+            stub_feature_flags(for_no_key_update_lock: false)
+            subject
+            reload_models(old_parent, new_parent, group)
+          end
+
+          it 'updates traversal_ids' do
+            expect(group.traversal_ids).to eq [root.id, new_parent.id, group.id]
+          end
+
+          it_behaves_like 'hierarchy with traversal_ids'
+          it_behaves_like 'locked row', 'FOR UPDATE' do
+            let(:row) { root }
+          end
         end
 
-        it_behaves_like 'hierarchy with traversal_ids'
-        it_behaves_like 'locked row' do
-          let(:row) { root }
+        context 'with FOR NO KEY UPDATE lock' do
+          before do
+            stub_feature_flags(for_no_key_update_lock: true)
+            subject
+            reload_models(old_parent, new_parent, group)
+          end
+
+          it 'updates traversal_ids' do
+            expect(group.traversal_ids).to eq [root.id, new_parent.id, group.id]
+          end
+
+          it_behaves_like 'hierarchy with traversal_ids'
+          it_behaves_like 'locked row', 'FOR NO KEY UPDATE' do
+            let(:row) { root }
+          end
         end
       end
 
@@ -410,6 +430,11 @@ RSpec.describe Group do
         let!(:new_parent) { create(:group) }
         let!(:group) { create(:group, parent: old_parent) }
 
+        before do
+          subject
+          reload_models(old_parent, new_parent, group)
+        end
+
         it 'updates traversal_ids' do
           expect(group.traversal_ids).to eq [new_parent.id, group.id]
         end
@@ -435,6 +460,11 @@ RSpec.describe Group do
         let!(:old_parent) { nil }
         let!(:new_parent) { create(:group) }
 
+        before do
+          subject
+          reload_models(old_parent, new_parent, group)
+        end
+
         it 'updates traversal_ids' do
           expect(group.traversal_ids).to eq [new_parent.id, group.id]
         end
@@ -452,6 +482,11 @@ RSpec.describe Group do
         let!(:old_parent) { create(:group) }
         let!(:new_parent) { nil }
 
+        before do
+          subject
+          reload_models(old_parent, new_parent, group)
+        end
+
         it 'updates traversal_ids' do
           expect(group.traversal_ids).to eq [group.id]
         end

spec/models/namespace/traversal_hierarchy_spec.rb

@@ -68,11 +68,24 @@ RSpec.describe Namespace::TraversalHierarchy, type: :model do
       end
     end
 
-    it_behaves_like 'locked row' do
+    it_behaves_like 'locked row', 'FOR UPDATE' do
       let(:recorded_queries) { ActiveRecord::QueryRecorder.new }
       let(:row) { root }
 
       before do
+        stub_feature_flags(for_no_key_update_lock: false)
+
+        recorded_queries.record { subject }
+      end
+    end
+
+    it_behaves_like 'locked row', 'FOR NO KEY UPDATE' do
+      let(:recorded_queries) { ActiveRecord::QueryRecorder.new }
+      let(:row) { root }
+
+      before do
+        stub_feature_flags(for_no_key_update_lock: true)
+
         recorded_queries.record { subject }
       end
     end

spec/support/shared_examples/row_lock_shared_examples.rb

@@ -4,10 +4,10 @@
 # Ensure a transaction also occurred.
 # Be careful! This form of spec is not foolproof, but better than nothing.
 
-RSpec.shared_examples 'locked row' do
+RSpec.shared_examples 'locked row' do |lock_type|
   it "has locked row" do
     table_name = row.class.table_name
-    ids_regex = /SELECT.*FROM.*#{table_name}.*"#{table_name}"."id" = #{row.id}.+FOR UPDATE/m
+    ids_regex = /SELECT.*FROM.*#{table_name}.*"#{table_name}"."id" = #{row.id}.+#{lock_type}/m
 
     expect(recorded_queries.log).to include a_string_matching 'SAVEPOINT'
     expect(recorded_queries.log).to include a_string_matching ids_regex