diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-05-28 19:57:49 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-05-28 19:57:49 +0300 |
commit | b7467908b9366338bf7c4954f7a0d0a8b5266523 (patch) | |
tree | 30ab2e4cf8a2dccfbf8cf3c9619ddc212c9137ce | |
parent | 47cd6a061b9fc99a33e557a256357148ccb1362d (diff) | |
parent | f2cc1169e8c4e4ed209df0c6b50c1f5b69f5fdb0 (diff) |
Merge branch 'ab-45389-remove-double-checked-internal-id-generation' into 'master'
Remove double-checked internal id generation.
Closes #45389
See merge request gitlab-org/gitlab-ce!19181
-rw-r--r-- | app/models/internal_id.rb | 24 | ||||
-rw-r--r-- | changelogs/unreleased/ab-45389-remove-double-checked-internal-id-generation.yml | 5 | ||||
-rw-r--r-- | spec/models/internal_id_spec.rb | 37 |
3 files changed, 11 insertions, 55 deletions
diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index 189942c5ad8..f7f930e86ed 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -24,12 +24,9 @@ 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. - # - # 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) + def increment_and_save! lock! - self.last_value = [(last_value || 0) + 1, (maximum_iid || 0) + 1].max + self.last_value = (last_value || 0) + 1 save! last_value end @@ -93,16 +90,7 @@ class InternalId < ActiveRecord::Base # and increment its last value # # Note this will acquire a ROW SHARE lock on the InternalId record - - # 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) + (lookup || create_record).increment_and_save! end end @@ -128,15 +116,11 @@ class InternalId < ActiveRecord::Base InternalId.create!( **scope, usage: usage_value, - last_value: maximum_iid + last_value: init.call(subject) || 0 ) end rescue ActiveRecord::RecordNotUnique lookup end - - def maximum_iid - @maximum_iid ||= init.call(subject) || 0 - end end end diff --git a/changelogs/unreleased/ab-45389-remove-double-checked-internal-id-generation.yml b/changelogs/unreleased/ab-45389-remove-double-checked-internal-id-generation.yml new file mode 100644 index 00000000000..d87604de0f8 --- /dev/null +++ b/changelogs/unreleased/ab-45389-remove-double-checked-internal-id-generation.yml @@ -0,0 +1,5 @@ +--- +title: Remove double-checked internal id generation. +merge_request: 19181 +author: +type: performance diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb index 8ef91e8fab5..581fd0293cc 100644 --- a/spec/models/internal_id_spec.rb +++ b/spec/models/internal_id_spec.rb @@ -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.maximum(:iid) } } + let(:init) { ->(s) { s.project.issues.size } } context 'validations' do it { is_expected.to validate_presence_of(:usage) } @@ -39,29 +39,6 @@ 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] } @@ -104,8 +81,7 @@ describe InternalId do describe '#increment_and_save!' do let(:id) { create(:internal_id) } - let(:maximum_iid) { nil } - subject { id.increment_and_save!(maximum_iid) } + subject { id.increment_and_save! } it 'returns incremented iid' do value = id.last_value @@ -126,14 +102,5 @@ 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 |