From 754272e392c0da088200a1b56156600973f63267 Mon Sep 17 00:00:00 2001
From: Andreas Brandl <abrandl@gitlab.com>
Date: Tue, 6 Mar 2018 20:09:01 +0100
Subject: [PATCH] Atomic generation of internal ids for issues.

---
 app/models/concerns/atomic_internal_id.rb     | 19 ++++
 app/models/internal_id.rb                     | 84 ++++++++++++++++++
 app/models/issue.rb                           |  2 +-
 app/models/project.rb                         |  2 +
 ...0180305095250_create_internal_ids_table.rb | 35 ++++++++
 db/schema.rb                                  |  9 ++
 spec/factories/internal_ids.rb                |  6 ++
 spec/models/internal_id_spec.rb               | 87 +++++++++++++++++++
 8 files changed, 243 insertions(+), 1 deletion(-)
 create mode 100644 app/models/concerns/atomic_internal_id.rb
 create mode 100644 app/models/internal_id.rb
 create mode 100644 db/migrate/20180305095250_create_internal_ids_table.rb
 create mode 100644 spec/factories/internal_ids.rb
 create mode 100644 spec/models/internal_id_spec.rb

diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb
new file mode 100644
index 00000000000..3cc9ce7f03f
--- /dev/null
+++ b/app/models/concerns/atomic_internal_id.rb
@@ -0,0 +1,19 @@
+module AtomicInternalId
+  extend ActiveSupport::Concern
+
+  included do
+    before_validation(on: :create) do
+      set_iid
+    end
+
+    validates :iid, presence: true, numericality: true
+  end
+
+  def set_iid
+    self.iid = InternalId.generate_next(self.project, :issues) if iid.blank?
+  end
+
+  def to_param
+    iid.to_s
+  end
+end
diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb
new file mode 100644
index 00000000000..24c7cbf988f
--- /dev/null
+++ b/app/models/internal_id.rb
@@ -0,0 +1,84 @@
+# An InternalId is a strictly monotone sequence of integers
+# for a given project and usage (e.g. issues).
+#
+# For possible usages, see InternalId#usage enum.
+class InternalId < ActiveRecord::Base
+  belongs_to :project
+
+  enum usage: { issues: 0 }
+
+  validates :usage, presence: true
+  validates :project_id, presence: true
+
+  # 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!
+    lock!
+    self.last_value = (last_value || 0) + 1
+    save!
+    last_value
+  end
+
+  before_create :calculate_last_value!
+
+  # Calculate #last_value by counting the number of
+  # existing records for this usage.
+  def calculate_last_value!
+    return if last_value
+
+    parent = project # ??|| group
+    self.last_value = parent.send(usage.to_sym).maximum(:iid) || 0 # rubocop:disable GitlabSecurity/PublicSend
+  end
+
+  class << self
+    # Generate next internal id for a given project 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 (project, 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.
+    def generate_next(project, usage)
+      raise 'project not set - this is required' unless project
+
+      project.transaction do
+        # Create a record in internal_ids if one does not yet exist
+        id = (lookup(project, usage) || create_record(project, usage))
+
+        # This will lock the InternalId record with ROW SHARE
+        # and increment #last_value
+        id.increment_and_save!
+      end
+    end
+
+    private
+
+    # Retrieve InternalId record for (project, usage) combination, if it exists
+    def lookup(project, usage)
+      project.internal_ids.find_by(usage: usages[usage.to_s])
+    end
+
+    # Create InternalId record for (project, usage) combination, if it doesn't exist
+    #
+    # We blindly insert without any synchronization. If another process
+    # was faster in doing this, we'll realize once we hit the unique key constraint
+    # violation. We can safely roll-back the nested transaction and perform
+    # a lookup instead to retrieve the record.
+    def create_record(project, usage)
+      begin
+        project.transaction(requires_new: true) do
+          create!(project: project, usage: usages[usage.to_s])
+        end
+      rescue ActiveRecord::RecordNotUnique
+        lookup(project, usage)
+      end
+    end
+  end
+end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index cca6224d65c..cf547106122 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -1,7 +1,7 @@
 require 'carrierwave/orm/activerecord'
 
 class Issue < ActiveRecord::Base
-  include NonatomicInternalId
+  include AtomicInternalId
   include Issuable
   include Noteable
   include Referable
diff --git a/app/models/project.rb b/app/models/project.rb
index a291ad7eed5..93b0da5cce1 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -188,6 +188,8 @@ class Project < ActiveRecord::Base
   has_many :todos
   has_many :notification_settings, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
+  has_many :internal_ids
+
   has_one :import_data, class_name: 'ProjectImportData', inverse_of: :project, autosave: true
   has_one :project_feature, inverse_of: :project
   has_one :statistics, class_name: 'ProjectStatistics'
diff --git a/db/migrate/20180305095250_create_internal_ids_table.rb b/db/migrate/20180305095250_create_internal_ids_table.rb
new file mode 100644
index 00000000000..19c1904bb43
--- /dev/null
+++ b/db/migrate/20180305095250_create_internal_ids_table.rb
@@ -0,0 +1,35 @@
+class CreateInternalIdsTable < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    create_table :internal_ids do |t|
+      t.references :project
+      t.integer :usage, null: false
+      t.integer :last_value, null: false
+    end
+
+    unless index_exists?(:internal_ids, [:usage, :project_id])
+      add_index :internal_ids, [:usage, :project_id], unique: true
+    end
+
+    unless foreign_key_exists?(:internal_ids, :project_id)
+      add_concurrent_foreign_key :internal_ids, :projects, column: :project_id, on_delete: :cascade
+    end
+  end
+
+  def down
+    drop_table :internal_ids
+  end
+
+  private
+
+  def foreign_key_exists?(table, column)
+    foreign_keys(table).any? do |key|
+      key.options[:column] == column.to_s
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ab4370e2754..3785bf14d5c 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -866,6 +866,14 @@ ActiveRecord::Schema.define(version: 20180309160427) do
 
   add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree
 
+  create_table "internal_ids", force: :cascade do |t|
+    t.integer "project_id"
+    t.integer "usage", null: false
+    t.integer "last_value", null: false
+  end
+
+  add_index "internal_ids", ["usage", "project_id"], name: "index_internal_ids_on_usage_and_project_id", unique: true, using: :btree
+
   create_table "issue_assignees", id: false, force: :cascade do |t|
     t.integer "user_id", null: false
     t.integer "issue_id", null: false
@@ -2058,6 +2066,7 @@ ActiveRecord::Schema.define(version: 20180309160427) do
   add_foreign_key "gpg_signatures", "gpg_keys", on_delete: :nullify
   add_foreign_key "gpg_signatures", "projects", on_delete: :cascade
   add_foreign_key "group_custom_attributes", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "internal_ids", "projects", name: "fk_f7d46b66c6", on_delete: :cascade
   add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade
   add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade
   add_foreign_key "issue_metrics", "issues", on_delete: :cascade
diff --git a/spec/factories/internal_ids.rb b/spec/factories/internal_ids.rb
new file mode 100644
index 00000000000..b4c14d22a29
--- /dev/null
+++ b/spec/factories/internal_ids.rb
@@ -0,0 +1,6 @@
+FactoryBot.define do
+  factory :internal_id do
+    project
+    usage { InternalId.usages.keys.first }
+  end
+end
diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb
new file mode 100644
index 00000000000..b953b6a2df8
--- /dev/null
+++ b/spec/models/internal_id_spec.rb
@@ -0,0 +1,87 @@
+require 'spec_helper'
+
+describe InternalId do
+  let(:project) { create(:project) }
+  let(:usage) { :issues }
+
+  context 'validations' do
+    it { is_expected.to validate_presence_of(:usage) }
+    it { is_expected.to validate_presence_of(:project_id) }
+  end
+
+  describe '.generate_next' do
+    context 'in the absence of a record' do
+      subject { described_class.generate_next(project, usage) }
+
+      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) # TODO
+        end
+      end
+
+      context 'with existing issues' do
+        before do
+          rand(10).times { create(:issue, project: project) }
+        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 = (0..rand(1000)).map do
+        described_class.generate_next(project, usage)
+      end
+      normalized = seq.map { |i| i - seq.min }
+      expect(normalized).to eq((0..seq.size - 1).to_a)
+    end
+  end
+
+  describe '#increment_and_save!' do
+    let(:id) { create(:internal_id) }
+    subject { id.increment_and_save! }
+
+    it 'returns incremented iid' do
+      value = id.last_value
+      expect(subject).to eq(value + 1)
+    end
+
+    it 'saves the record' do
+      subject
+      expect(id.changed?).to be_falsey
+    end
+
+    context 'with last_value=nil' do
+      let(:id) { build(:internal_id, last_value: nil) }
+
+      it 'returns 1' do
+        expect(subject).to eq(1)
+      end
+    end
+  end
+
+  describe '#calculate_last_value! (for issues)' do
+    subject do
+      build(:internal_id, project: project, usage: :issues)
+    end
+
+    context 'with existing issues' do
+      before do
+        rand(10).times { create(:issue, project: project) }
+      end
+
+      it 'counts related issues and saves' do
+        expect { subject.calculate_last_value! }.to change { subject.last_value }.from(nil).to(project.issues.size)
+      end
+    end
+  end
+end
-- 
GitLab