Commit 527cf391 authored by Alexandru Croitor's avatar Alexandru Croitor Committed by Adam Hegyi

Use the for no key update lock when upadting traversal ids

Updating traversal_ids for a namespace does require an update
lock however we can use a weaker lock to avoid deadlocks
in cases where multiple projects or namespaces are created
concurently at the root namespace level due to KEY SHARE and
UPDATE locks blocking each other. NO KEY UPDATE lock
and KEY SHARE lock do not block each other.
parent 174311a0
......@@ -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
......
---
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
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment