Commit 14acbf24 authored by Andreas Brandl's avatar Andreas Brandl

Double-check next value for internal ids.

This is useful for a transition period to migrate away from
`NoninternalAtomicId`. In a situation where both the old and new code to
generate a iid value is run at the same time (for example, during a
deploy different nodes may serve both versions), this will lead to
problems regarding the correct `last_value`. That is, what we track in
`InternalId` may get out of sync with the maximum iid present for
issues.

With this change, we double-check that and correct the `last_value` with
the maximum iid found in issues if necessary.

This is subject to be removed with the 10.8 release and tracked over
here: https://gitlab.com/gitlab-org/gitlab-ce/issues/45389

Closes #45269.
parent c3e26860
...@@ -23,9 +23,12 @@ class InternalId < ActiveRecord::Base ...@@ -23,9 +23,12 @@ class InternalId < ActiveRecord::Base
# #
# The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL). # The operation locks the record and gathers a `ROW SHARE` lock (in PostgreSQL).
# As such, the increment is atomic and safe to be called concurrently. # As such, the increment is atomic and safe to be called concurrently.
def increment_and_save! #
# If a `maximum_iid` is passed in, this overrides the incremented value if it's
# greater than that. This can be used to correct the increment value if necessary.
def increment_and_save!(maximum_iid)
lock! lock!
self.last_value = (last_value || 0) + 1 self.last_value = [(last_value || 0) + 1, (maximum_iid || 0) + 1].max
save! save!
last_value last_value
end end
...@@ -89,7 +92,16 @@ class InternalId < ActiveRecord::Base ...@@ -89,7 +92,16 @@ class InternalId < ActiveRecord::Base
# and increment its last value # and increment its last value
# #
# Note this will acquire a ROW SHARE lock on the InternalId record # Note this will acquire a ROW SHARE lock on the InternalId record
(lookup || create_record).increment_and_save!
# Note we always calculate the maximum iid present here and
# pass it in to correct the InternalId entry if it's last_value is off.
#
# This can happen in a transition phase where both `AtomicInternalId` and
# `NonatomicInternalId` code runs (e.g. during a deploy).
#
# This is subject to be cleaned up with the 10.8 release:
# https://gitlab.com/gitlab-org/gitlab-ce/issues/45389.
(lookup || create_record).increment_and_save!(maximum_iid)
end end
end end
...@@ -115,11 +127,15 @@ class InternalId < ActiveRecord::Base ...@@ -115,11 +127,15 @@ class InternalId < ActiveRecord::Base
InternalId.create!( InternalId.create!(
**scope, **scope,
usage: usage_value, usage: usage_value,
last_value: init.call(subject) || 0 last_value: maximum_iid
) )
end end
rescue ActiveRecord::RecordNotUnique rescue ActiveRecord::RecordNotUnique
lookup lookup
end end
def maximum_iid
@maximum_iid ||= init.call(subject) || 0
end
end end
end end
...@@ -5,7 +5,7 @@ describe InternalId do ...@@ -5,7 +5,7 @@ describe InternalId do
let(:usage) { :issues } let(:usage) { :issues }
let(:issue) { build(:issue, project: project) } let(:issue) { build(:issue, project: project) }
let(:scope) { { project: project } } let(:scope) { { project: project } }
let(:init) { ->(s) { s.project.issues.size } } let(:init) { ->(s) { s.project.issues.maximum(:iid) } }
context 'validations' do context 'validations' do
it { is_expected.to validate_presence_of(:usage) } it { is_expected.to validate_presence_of(:usage) }
...@@ -39,6 +39,29 @@ describe InternalId do ...@@ -39,6 +39,29 @@ describe InternalId do
end end
end end
context 'with an InternalId record present and existing issues with a higher internal id' do
# This can happen if the old NonatomicInternalId is still in use
before do
issues = Array.new(rand(1..10)).map { create(:issue, project: project) }
issue = issues.last
issue.iid = issues.map { |i| i.iid }.max + 1
issue.save
end
let(:maximum_iid) { project.issues.map { |i| i.iid }.max }
it 'updates last_value to the maximum internal id present' do
subject
expect(described_class.find_by(project: project, usage: described_class.usages[usage.to_s]).last_value).to eq(maximum_iid + 1)
end
it 'returns next internal id correctly' do
expect(subject).to eq(maximum_iid + 1)
end
end
context 'with concurrent inserts on table' do context 'with concurrent inserts on table' do
it 'looks up the record if it was created concurrently' do it 'looks up the record if it was created concurrently' do
args = { **scope, usage: described_class.usages[usage.to_s] } args = { **scope, usage: described_class.usages[usage.to_s] }
...@@ -81,7 +104,8 @@ describe InternalId do ...@@ -81,7 +104,8 @@ describe InternalId do
describe '#increment_and_save!' do describe '#increment_and_save!' do
let(:id) { create(:internal_id) } let(:id) { create(:internal_id) }
subject { id.increment_and_save! } let(:maximum_iid) { nil }
subject { id.increment_and_save!(maximum_iid) }
it 'returns incremented iid' do it 'returns incremented iid' do
value = id.last_value value = id.last_value
...@@ -102,5 +126,14 @@ describe InternalId do ...@@ -102,5 +126,14 @@ describe InternalId do
expect(subject).to eq(1) expect(subject).to eq(1)
end end
end end
context 'with maximum_iid given' do
let(:id) { create(:internal_id, last_value: 1) }
let(:maximum_iid) { id.last_value + 10 }
it 'returns maximum_iid instead' do
expect(subject).to eq(12)
end
end
end end
end end
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