Commit ddd7a81d authored by Yorick Peterse's avatar Yorick Peterse

Merge branch '45269-double-checked-internal-ids' into 'master'

Double-checked internal ID generation for transition phase

Closes #45269

See merge request gitlab-org/gitlab-ce!18392
parents 5d6fb753 14acbf24
......@@ -23,9 +23,12 @@ class InternalId < ActiveRecord::Base
#
# 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.
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!
self.last_value = (last_value || 0) + 1
self.last_value = [(last_value || 0) + 1, (maximum_iid || 0) + 1].max
save!
last_value
end
......@@ -89,7 +92,16 @@ class InternalId < ActiveRecord::Base
# and increment its last value
#
# 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
......@@ -115,11 +127,15 @@ class InternalId < ActiveRecord::Base
InternalId.create!(
**scope,
usage: usage_value,
last_value: init.call(subject) || 0
last_value: maximum_iid
)
end
rescue ActiveRecord::RecordNotUnique
lookup
end
def maximum_iid
@maximum_iid ||= init.call(subject) || 0
end
end
end
......@@ -5,7 +5,7 @@ describe InternalId do
let(:usage) { :issues }
let(:issue) { build(:issue, project: project) }
let(:scope) { { project: project } }
let(:init) { ->(s) { s.project.issues.size } }
let(:init) { ->(s) { s.project.issues.maximum(:iid) } }
context 'validations' do
it { is_expected.to validate_presence_of(:usage) }
......@@ -39,6 +39,29 @@ describe InternalId do
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
it 'looks up the record if it was created concurrently' do
args = { **scope, usage: described_class.usages[usage.to_s] }
......@@ -81,7 +104,8 @@ describe InternalId do
describe '#increment_and_save!' do
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
value = id.last_value
......@@ -102,5 +126,14 @@ describe InternalId do
expect(subject).to eq(1)
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
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