Merge branch 'pb-remove-explicit-lock-iid-ff' into 'master'

Remove the feature flag for explicit locking iids

See merge request gitlab-org/gitlab!69769

app/models/internal_id.rb

@@ -4,7 +4,7 @@
 # generated for a given scope and usage.
 #
 # The monotone sequence may be broken if an ID is explicitly provided
-# to `.track_greatest_and_save!` or `#track_greatest`.
+# to `#track_greatest`.
 #
 # For example, issues use their project to scope internal ids:
 # In that sense, scope is "project" and usage is "issues".
@@ -29,32 +29,6 @@ class InternalId < ApplicationRecord
     where(**scope, usage: usage)
   end
 
-  # Increments #last_value and saves the record
-  #
-  # 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!
-    update_and_save { self.last_value = (last_value || 0) + 1 }
-  end
-
-  # Increments #last_value with new_value if it is greater than the current,
-  # and saves the record
-  #
-  # 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 track_greatest_and_save!(new_value)
-    update_and_save { self.last_value = [last_value || 0, new_value].max }
-  end
-
-  private
-
-  def update_and_save(&block)
-    lock!
-    yield
-    save!
-    last_value
-  end
-
   class << self
     def track_greatest(subject, scope, usage, new_value, init)
       build_generator(subject, scope, usage, init).track_greatest(new_value)
@@ -99,143 +73,7 @@ class InternalId < ApplicationRecord
     private
 
     def build_generator(subject, scope, usage, init = nil)
-      if Feature.enabled?(:generate_iids_without_explicit_locking)
-        ImplicitlyLockingInternalIdGenerator.new(subject, scope, usage, init)
-      else
-        InternalIdGenerator.new(subject, scope, usage, init)
-      end
-    end
-  end
-
-  class InternalIdGenerator
-    # Generate next internal id for a given scope and usage.
-    #
-    # For currently supported usages, see #usage enum.
-    #
-    # The method implements a locking scheme that has the following properties:
-    # 1) Generated sequence of internal ids is unique per (scope and usage)
-    # 2) The method is thread-safe and may be used in concurrent threads/processes.
-    # 3) The generated sequence is gapless.
-    # 4) In the absence of a record in the internal_ids table, one will be created
-    #    and last_value will be calculated on the fly.
-    #
-    # subject: The instance or class we're generating an internal id for.
-    # scope: Attributes that define the scope for id generation.
-    #        Valid keys are `project/project_id` and `namespace/namespace_id`.
-    # usage: Symbol to define the usage of the internal id, see InternalId.usages
-    # init: Proc that accepts the subject and the scope and returns Integer|NilClass
-    attr_reader :subject, :scope, :scope_attrs, :usage, :init
-
-    def initialize(subject, scope, usage, init = nil)
-      @subject = subject
-      @scope = scope
-      @usage = usage
-      @init = init
-
-      raise ArgumentError, 'Scope is not well-defined, need at least one column for scope (given: 0)' if scope.empty?
-
-      unless InternalId.usages.has_key?(usage.to_s)
-        raise ArgumentError, "Usage '#{usage}' is unknown. Supported values are #{InternalId.usages.keys} from InternalId.usages"
-      end
-    end
-
-    # Generates next internal id and returns it
-    # init: Block that gets called to initialize InternalId record if not present
-    #       Make sure to not throw exceptions in the absence of records (if this is expected).
-    def generate
-      InternalId.internal_id_transactions_increment(operation: :generate, usage: usage)
-
-      subject.transaction do
-        # Create a record in internal_ids if one does not yet exist
-        # and increment its last value
-        #
-        # Note this will acquire a ROW SHARE lock on the InternalId record
-        record.increment_and_save!
-      end
-    end
-
-    # Reset tries to rewind to `value-1`. This will only succeed,
-    # if `value` stored in database is equal to `last_value`.
-    # value: The expected last_value to decrement
-    def reset(value)
-      return false unless value
-
-      InternalId.internal_id_transactions_increment(operation: :reset, usage: usage)
-
-      updated =
-        InternalId
-          .where(**scope, usage: usage_value)
-          .where(last_value: value)
-          .update_all('last_value = last_value - 1')
-
-      updated > 0
-    end
-
-    # Create a record in internal_ids if one does not yet exist
-    # and set its new_value if it is higher than the current last_value
-    #
-    # Note this will acquire a ROW SHARE lock on the InternalId record
-
-    def track_greatest(new_value)
-      InternalId.internal_id_transactions_increment(operation: :track_greatest, usage: usage)
-
-      subject.transaction do
-        record.track_greatest_and_save!(new_value)
-      end
-    end
-
-    def record
-      @record ||= (lookup || create_record)
-    end
-
-    def with_lock(&block)
-      InternalId.internal_id_transactions_increment(operation: :with_lock, usage: usage)
-
-      record.with_lock(&block)
-    end
-
-    private
-
-    # Retrieve InternalId record for (project, usage) combination, if it exists
-    def lookup
-      InternalId.find_by(**scope, usage: usage_value)
-    end
-
-    def initial_value(subject, scope)
-      raise ArgumentError, 'Cannot initialize without init!' unless init
-
-      # `init` computes the maximum based on actual records. We use the
-      # primary to make sure we have up to date results
-      Gitlab::Database::LoadBalancing::Session.current.use_primary do
-        instance = subject.is_a?(::Class) ? nil : subject
-
-        init.call(instance, scope) || 0
-      end
-    end
-
-    def usage_value
-      @usage_value ||= InternalId.usages[usage.to_s]
-    end
-
-    # Create InternalId record for (scope, usage) combination, if it doesn't exist
-    #
-    # We blindly insert ignoring conflicts on the unique key constraint.
-    # If another process was faster in doing this, we'll end up with that record
-    # when we do the lookup after the insert.
-    def create_record
-      scope[:project].save! if scope[:project] && !scope[:project].persisted?
-      scope[:namespace].save! if scope[:namespace] && !scope[:namespace].persisted?
-
-      attributes = {
-        project_id: scope[:project]&.id || scope[:project_id],
-        namespace_id: scope[:namespace]&.id || scope[:namespace_id],
-        usage: usage_value,
-        last_value: initial_value(subject, scope)
-      }
-
-      InternalId.insert(attributes)
-
-      lookup
+      ImplicitlyLockingInternalIdGenerator.new(subject, scope, usage, init)
     end
   end
 

config/feature_flags/development/generate_iids_without_explicit_locking.yml

----
-name: generate_iids_without_explicit_locking
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65590
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335431
-milestone: '14.2'
-type: development
-group: group::database
-default_enabled: false

spec/models/internal_id_spec.rb

@@ -39,270 +39,199 @@ RSpec.describe InternalId do
     end
   end
 
-  shared_examples_for 'a monotonically increasing id generator' do
-    describe '.generate_next' do
-      subject { described_class.generate_next(id_subject, scope, usage, init) }
+  describe '.generate_next' do
+    subject { described_class.generate_next(id_subject, scope, usage, init) }
 
-      context 'in the absence of a record' do
-        it 'creates a record if not yet present' do
-          expect { subject }.to change { described_class.count }.from(0).to(1)
-        end
-
-        it 'stores record attributes' do
-          subject
-
-          described_class.first.tap do |record|
-            expect(record.project).to eq(project)
-            expect(record.usage).to eq(usage.to_s)
-          end
-        end
-
-        context 'with existing issues' do
-          before do
-            create_list(:issue, 2, project: project)
-            described_class.delete_all
-          end
-
-          it 'calculates last_value values automatically' do
-            expect(subject).to eq(project.issues.size + 1)
-          end
-        end
-      end
-
-      it 'generates a strictly monotone, gapless sequence' do
-        seq = Array.new(10).map do
-          described_class.generate_next(issue, scope, usage, init)
-        end
-        normalized = seq.map { |i| i - seq.min }
-
-        expect(normalized).to eq((0..seq.size - 1).to_a)
+    context 'in the absence of a record' do
+      it 'creates a record if not yet present' do
+        expect { subject }.to change { described_class.count }.from(0).to(1)
       end
 
-      context 'there are no instances to pass in' do
-        let(:id_subject) { Issue }
+      it 'stores record attributes' do
+        subject
 
-        it 'accepts classes instead' do
-          expect(subject).to eq(1)
+        described_class.first.tap do |record|
+          expect(record.project).to eq(project)
+          expect(record.usage).to eq(usage.to_s)
         end
       end
 
-      context 'when executed outside of transaction' do
-        it 'increments counter with in_transaction: "false"' do
-          allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } # rubocop: disable Database/MultipleDatabases
-
-          expect(InternalId.internal_id_transactions_total).to receive(:increment)
-            .with(operation: :generate, usage: 'issues', in_transaction: 'false').and_call_original
-
-          subject
+      context 'with existing issues' do
+        before do
+          create_list(:issue, 2, project: project)
+          described_class.delete_all
         end
-      end
 
-      context 'when executed within transaction' do
-        it 'increments counter with in_transaction: "true"' do
-          expect(InternalId.internal_id_transactions_total).to receive(:increment)
-            .with(operation: :generate, usage: 'issues', in_transaction: 'true').and_call_original
-
-          InternalId.transaction { subject }
+        it 'calculates last_value values automatically' do
+          expect(subject).to eq(project.issues.size + 1)
         end
       end
     end
 
-    describe '.reset' do
-      subject { described_class.reset(issue, scope, usage, value) }
-
-      context 'in the absence of a record' do
-        let(:value) { 2 }
-
-        it 'does not revert back the value' do
-          expect { subject }.not_to change { described_class.count }
-          expect(subject).to be_falsey
-        end
+    it 'generates a strictly monotone, gapless sequence' do
+      seq = Array.new(10).map do
+        described_class.generate_next(issue, scope, usage, init)
       end
+      normalized = seq.map { |i| i - seq.min }
 
-      context 'when valid iid is used to reset' do
-        let!(:value) { generate_next }
-
-        context 'and iid is a latest one' do
-          it 'does rewind and next generated value is the same' do
-            expect(subject).to be_truthy
-            expect(generate_next).to eq(value)
-          end
-        end
+      expect(normalized).to eq((0..seq.size - 1).to_a)
+    end
 
-        context 'and iid is not a latest one' do
-          it 'does not rewind' do
-            generate_next
+    context 'there are no instances to pass in' do
+      let(:id_subject) { Issue }
 
-            expect(subject).to be_falsey
-            expect(generate_next).to be > value
-          end
-        end
-
-        def generate_next
-          described_class.generate_next(issue, scope, usage, init)
-        end
+      it 'accepts classes instead' do
+        expect(subject).to eq(1)
       end
+    end
 
-      context 'when executed outside of transaction' do
-        let(:value) { 2 }
-
-        it 'increments counter with in_transaction: "false"' do
-          allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } # rubocop: disable Database/MultipleDatabases
+    context 'when executed outside of transaction' do
+      it 'increments counter with in_transaction: "false"' do
+        allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } # rubocop: disable Database/MultipleDatabases
 
-          expect(InternalId.internal_id_transactions_total).to receive(:increment)
-            .with(operation: :reset, usage: 'issues', in_transaction: 'false').and_call_original
+        expect(InternalId.internal_id_transactions_total).to receive(:increment)
+          .with(operation: :generate, usage: 'issues', in_transaction: 'false').and_call_original
 
-          subject
-        end
+        subject
       end
+    end
 
-      context 'when executed within transaction' do
-        let(:value) { 2 }
-
-        it 'increments counter with in_transaction: "true"' do
-          expect(InternalId.internal_id_transactions_total).to receive(:increment)
-            .with(operation: :reset, usage: 'issues', in_transaction: 'true').and_call_original
+    context 'when executed within transaction' do
+      it 'increments counter with in_transaction: "true"' do
+        expect(InternalId.internal_id_transactions_total).to receive(:increment)
+          .with(operation: :generate, usage: 'issues', in_transaction: 'true').and_call_original
 
-          InternalId.transaction { subject }
-        end
+        InternalId.transaction { subject }
       end
     end
+  end
 
-    describe '.track_greatest' do
-      let(:value) { 9001 }
+  describe '.reset' do
+    subject { described_class.reset(issue, scope, usage, value) }
 
-      subject { described_class.track_greatest(id_subject, scope, usage, value, init) }
+    context 'in the absence of a record' do
+      let(:value) { 2 }
 
-      context 'in the absence of a record' do
-        it 'creates a record if not yet present' do
-          expect { subject }.to change { described_class.count }.from(0).to(1)
-        end
-      end
-
-      it 'stores record attributes' do
-        subject
-
-        described_class.first.tap do |record|
-          expect(record.project).to eq(project)
-          expect(record.usage).to eq(usage.to_s)
-          expect(record.last_value).to eq(value)
-        end
+      it 'does not revert back the value' do
+        expect { subject }.not_to change { described_class.count }
+        expect(subject).to be_falsey
       end
+    end
 
-      context 'with existing issues' do
-        before do
-          create(:issue, project: project)
-          described_class.delete_all
-        end
+    context 'when valid iid is used to reset' do
+      let!(:value) { generate_next }
 
-        it 'still returns the last value to that of the given value' do
-          expect(subject).to eq(value)
+      context 'and iid is a latest one' do
+        it 'does rewind and next generated value is the same' do
+          expect(subject).to be_truthy
+          expect(generate_next).to eq(value)
         end
       end
 
-      context 'when value is less than the current last_value' do
-        it 'returns the current last_value' do
-          described_class.create!(**scope, usage: usage, last_value: 10_001)
+      context 'and iid is not a latest one' do
+        it 'does not rewind' do
+          generate_next
 
-          expect(subject).to eq 10_001
+          expect(subject).to be_falsey
+          expect(generate_next).to be > value
         end
       end
 
-      context 'there are no instances to pass in' do
-        let(:id_subject) { Issue }
-
-        it 'accepts classes instead' do
-          expect(subject).to eq(value)
-        end
+      def generate_next
+        described_class.generate_next(issue, scope, usage, init)
       end
+    end
 
-      context 'when executed outside of transaction' do
-        it 'increments counter with in_transaction: "false"' do
-          allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } # rubocop: disable Database/MultipleDatabases
-
-          expect(InternalId.internal_id_transactions_total).to receive(:increment)
-            .with(operation: :track_greatest, usage: 'issues', in_transaction: 'false').and_call_original
+    context 'when executed outside of transaction' do
+      let(:value) { 2 }
 
-          subject
-        end
-      end
+      it 'increments counter with in_transaction: "false"' do
+        allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } # rubocop: disable Database/MultipleDatabases
 
-      context 'when executed within transaction' do
-        it 'increments counter with in_transaction: "true"' do
-          expect(InternalId.internal_id_transactions_total).to receive(:increment)
-            .with(operation: :track_greatest, usage: 'issues', in_transaction: 'true').and_call_original
+        expect(InternalId.internal_id_transactions_total).to receive(:increment)
+          .with(operation: :reset, usage: 'issues', in_transaction: 'false').and_call_original
 
-          InternalId.transaction { subject }
-        end
+        subject
       end
     end
-  end
 
-  context 'when the explicit locking feature flag is disabled' do
-    before do
-      stub_feature_flags(generate_iids_without_explicit_locking: false)
-    end
+    context 'when executed within transaction' do
+      let(:value) { 2 }
 
-    it_behaves_like 'a monotonically increasing id generator'
-  end
+      it 'increments counter with in_transaction: "true"' do
+        expect(InternalId.internal_id_transactions_total).to receive(:increment)
+          .with(operation: :reset, usage: 'issues', in_transaction: 'true').and_call_original
 
-  context 'when the explicit locking feature flag is enabled' do
-    before do
-      stub_feature_flags(generate_iids_without_explicit_locking: true)
+        InternalId.transaction { subject }
+      end
     end
-
-    it_behaves_like 'a monotonically increasing id generator'
   end
 
-  describe '#increment_and_save!' do
-    let(:id) { create(:internal_id) }
-
-    subject { id.increment_and_save! }
+  describe '.track_greatest' do
+    let(:value) { 9001 }
 
-    it 'returns incremented iid' do
-      value = id.last_value
+    subject { described_class.track_greatest(id_subject, scope, usage, value, init) }
 
-      expect(subject).to eq(value + 1)
+    context 'in the absence of a record' do
+      it 'creates a record if not yet present' do
+        expect { subject }.to change { described_class.count }.from(0).to(1)
+      end
     end
 
-    it 'saves the record' do
+    it 'stores record attributes' do
       subject
 
-      expect(id.changed?).to be_falsey
+      described_class.first.tap do |record|
+        expect(record.project).to eq(project)
+        expect(record.usage).to eq(usage.to_s)
+        expect(record.last_value).to eq(value)
+      end
     end
 
-    context 'with last_value=nil' do
-      let(:id) { build(:internal_id, last_value: nil) }
+    context 'with existing issues' do
+      before do
+        create(:issue, project: project)
+        described_class.delete_all
+      end
 
-      it 'returns 1' do
-        expect(subject).to eq(1)
+      it 'still returns the last value to that of the given value' do
+        expect(subject).to eq(value)
       end
     end
-  end
-
-  describe '#track_greatest_and_save!' do
-    let(:id) { create(:internal_id) }
-    let(:new_last_value) { 9001 }
 
-    subject { id.track_greatest_and_save!(new_last_value) }
+    context 'when value is less than the current last_value' do
+      it 'returns the current last_value' do
+        described_class.create!(**scope, usage: usage, last_value: 10_001)
 
-    it 'returns new last value' do
-      expect(subject).to eq new_last_value
+        expect(subject).to eq 10_001
+      end
     end
 
-    it 'saves the record' do
-      subject
+    context 'there are no instances to pass in' do
+      let(:id_subject) { Issue }
 
-      expect(id.changed?).to be_falsey
+      it 'accepts classes instead' do
+        expect(subject).to eq(value)
+      end
     end
 
-    context 'when new last value is lower than the max' do
-      it 'does not update the last value' do
-        id.update!(last_value: 10_001)
+    context 'when executed outside of transaction' do
+      it 'increments counter with in_transaction: "false"' do
+        allow(ActiveRecord::Base.connection).to receive(:transaction_open?) { false } # rubocop: disable Database/MultipleDatabases
+
+        expect(InternalId.internal_id_transactions_total).to receive(:increment)
+          .with(operation: :track_greatest, usage: 'issues', in_transaction: 'false').and_call_original
 
         subject
+      end
+    end
+
+    context 'when executed within transaction' do
+      it 'increments counter with in_transaction: "true"' do
+        expect(InternalId.internal_id_transactions_total).to receive(:increment)
+          .with(operation: :track_greatest, usage: 'issues', in_transaction: 'true').and_call_original
 
-        expect(id.reload.last_value).to eq 10_001
+        InternalId.transaction { subject }
       end
     end
   end