Revert "Merge branch '207312-remove-optimisticlocking-monkeypatch' into 'master'"

This reverts merge request !25566

changelogs/unreleased/207312-remove-optimisticlocking-monkeypatch.yml

----
-title: Remove Rails Optimistic Locking monkeypatch
-merge_request: 25566
-author:
-type: fixed

config/initializers/config_initializers_active_record_locking.rb

+# frozen_string_literal: true
+
+# ensure ActiveRecord's version has been required already
+require 'active_record/locking/optimistic'
+
+# rubocop:disable Lint/RescueException
+module ActiveRecord
+  module Locking
+    module Optimistic
+      private
+
+      def _update_row(attribute_names, attempted_action = "update")
+        return super unless locking_enabled?
+
+        begin
+          locking_column = self.class.locking_column
+          previous_lock_value = read_attribute_before_type_cast(locking_column)
+          attribute_names << locking_column
+
+          self[locking_column] += 1
+
+          # Patched because when `lock_version` is read as `0`, it may actually be `NULL` in the DB.
+          possible_previous_lock_value = previous_lock_value.to_i == 0 ? [nil, 0] : previous_lock_value
+
+          affected_rows = self.class.unscoped.where(
+            locking_column => possible_previous_lock_value,
+            self.class.primary_key => id_in_database
+          ).update_all(
+            attributes_with_values(attribute_names)
+          )
+
+          if affected_rows != 1
+            raise ActiveRecord::StaleObjectError.new(self, attempted_action)
+          end
+
+          affected_rows
+
+        # If something went wrong, revert the locking_column value.
+        rescue Exception
+          self[locking_column] = previous_lock_value.to_i
+          raise
+        end
+      end
+    end
+  end
+end

db/post_migrate/20200608195222_set_lock_version_not_null_constraint.rb → db/migrate/20200618134223_restore_previous_schema_without_lock_version_null_constraint.rb

 # frozen_string_literal: true
 
-class SetLockVersionNotNullConstraint < ActiveRecord::Migration[6.0]
+class RestorePreviousSchemaWithoutLockVersionNullConstraint < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers
 
   DOWNTIME = false
+  TABLES = %i(epics merge_requests issues ci_stages ci_builds ci_pipelines).freeze
 
   disable_ddl_transaction!
 
-  TABLES = %i(epics merge_requests issues ci_stages ci_builds ci_pipelines).freeze
-
   def up
     TABLES.each do |table|
-      add_not_null_constraint table, :lock_version, validate: false
+      remove_not_null_constraint table, :lock_version
     end
   end
 
   def down
-    TABLES.each do |table|
-      remove_not_null_constraint table, :lock_version
-    end
+    # no-op
   end
 end

db/post_migrate/20200608212824_lock_version_cleanup_for_ci_pipelines.rb → db/migrate/20200618134723_restore_previous_schema_with_lock_version_indices.rb

 # frozen_string_literal: true
 
-class LockVersionCleanupForCiPipelines < ActiveRecord::Migration[6.0]
+class RestorePreviousSchemaWithLockVersionIndices < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers
 
   DOWNTIME = false
@@ -8,11 +8,15 @@ class LockVersionCleanupForCiPipelines < ActiveRecord::Migration[6.0]
   disable_ddl_transaction!
 
   def up
-    validate_not_null_constraint :ci_pipelines, :lock_version
-    remove_concurrent_index :ci_pipelines, :id, where: "lock_version IS NULL", name: "tmp_index_ci_pipelines_lock_version"
+    add_concurrent_index :issues, :lock_version, where: "lock_version IS NULL"
+    add_concurrent_index :merge_requests, :lock_version, where: "lock_version IS NULL"
+    add_concurrent_index :epics, :lock_version, where: "lock_version IS NULL"
+    add_concurrent_index :ci_stages, :id, where: "lock_version IS NULL", name: "tmp_index_ci_stages_lock_version"
+    add_concurrent_index :ci_builds, :id, where: "lock_version IS NULL", name: "tmp_index_ci_builds_lock_version"
+    add_concurrent_index :ci_pipelines, :id, where: "lock_version IS NULL", name: "tmp_index_ci_pipelines_lock_version"
   end
 
   def down
-    add_concurrent_index :ci_pipelines, :id, where: "lock_version IS NULL", name: "tmp_index_ci_pipelines_lock_version"
+    # no-op
   end
 end

db/post_migrate/20200608205813_set_lock_version_to_not_null.rb

-# frozen_string_literal: true
-
-class SetLockVersionToNotNull < ActiveRecord::Migration[6.0]
-  DOWNTIME = false
-
-  MODELS = [Epic, MergeRequest, Issue, Ci::Stage, Ci::Build, Ci::Pipeline].freeze
-
-  disable_ddl_transaction!
-
-  def up
-    MODELS.each do |model|
-      model.where(lock_version: nil).update_all(lock_version: 0)
-    end
-  end
-
-  def down
-    # Nothing to do...
-  end
-end

db/post_migrate/20200608212030_lock_version_cleanup_for_epics.rb

-# frozen_string_literal: true
-
-class LockVersionCleanupForEpics < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    validate_not_null_constraint :epics, :lock_version
-    remove_concurrent_index :epics, :lock_version, where: "lock_version IS NULL"
-  end
-
-  def down
-    add_concurrent_index :epics, :lock_version, where: "lock_version IS NULL"
-  end
-end

db/post_migrate/20200608212435_lock_version_cleanup_for_merge_requests.rb

-# frozen_string_literal: true
-
-class LockVersionCleanupForMergeRequests < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    validate_not_null_constraint :merge_requests, :lock_version
-    remove_concurrent_index :merge_requests, :lock_version, where: "lock_version IS NULL"
-  end
-
-  def down
-    add_concurrent_index :merge_requests, :lock_version, where: "lock_version IS NULL"
-  end
-end

db/post_migrate/20200608212549_lock_version_cleanup_for_issues.rb

-# frozen_string_literal: true
-
-class LockVersionCleanupForIssues < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    validate_not_null_constraint :issues, :lock_version
-    remove_concurrent_index :issues, :lock_version, where: "lock_version IS NULL"
-  end
-
-  def down
-    add_concurrent_index :issues, :lock_version, where: "lock_version IS NULL"
-  end
-end

db/post_migrate/20200608212652_lock_version_cleanup_for_ci_stages.rb

-# frozen_string_literal: true
-
-class LockVersionCleanupForCiStages < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    validate_not_null_constraint :ci_stages, :lock_version
-    remove_concurrent_index :ci_stages, :id, where: "lock_version IS NULL", name: "tmp_index_ci_stages_lock_version"
-  end
-
-  def down
-    add_concurrent_index :ci_stages, :id, where: "lock_version IS NULL", name: "tmp_index_ci_stages_lock_version"
-  end
-end

db/post_migrate/20200608212807_lock_version_cleanup_for_ci_builds.rb

-# frozen_string_literal: true
-
-class LockVersionCleanupForCiBuilds < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    validate_not_null_constraint :ci_builds, :lock_version
-    remove_concurrent_index :ci_builds, :id, where: "lock_version IS NULL", name: "tmp_index_ci_builds_lock_version"
-  end
-
-  def down
-    add_concurrent_index :ci_builds, :id, where: "lock_version IS NULL", name: "tmp_index_ci_builds_lock_version"
-  end
-end

db/structure.sql

@@ -1056,8 +1056,7 @@ CREATE TABLE public.ci_builds (
     resource_group_id bigint,
     waiting_for_resource_at timestamp with time zone,
     processed boolean,
-    scheduling_type smallint,
-    CONSTRAINT check_1e2fbd1b39 CHECK ((lock_version IS NOT NULL))
+    scheduling_type smallint
 );
 
 CREATE SEQUENCE public.ci_builds_id_seq
@@ -1368,8 +1367,7 @@ CREATE TABLE public.ci_pipelines (
     source_sha bytea,
     target_sha bytea,
     external_pull_request_id bigint,
-    ci_ref_id bigint,
-    CONSTRAINT check_d7e99a025e CHECK ((lock_version IS NOT NULL))
+    ci_ref_id bigint
 );
 
 CREATE TABLE public.ci_pipelines_config (
@@ -1554,8 +1552,7 @@ CREATE TABLE public.ci_stages (
     name character varying,
     status integer,
     lock_version integer DEFAULT 0,
-    "position" integer,
-    CONSTRAINT check_81b431e49b CHECK ((lock_version IS NOT NULL))
+    "position" integer
 );
 
 CREATE SEQUENCE public.ci_stages_id_seq
@@ -2514,8 +2511,7 @@ CREATE TABLE public.epics (
     start_date_sourcing_epic_id integer,
     due_date_sourcing_epic_id integer,
     confidential boolean DEFAULT false NOT NULL,
-    external_key character varying(255),
-    CONSTRAINT check_fcfb4a93ff CHECK ((lock_version IS NOT NULL))
+    external_key character varying(255)
 );
 
 CREATE SEQUENCE public.epics_id_seq
@@ -3558,8 +3554,7 @@ CREATE TABLE public.issues (
     promoted_to_epic_id integer,
     health_status smallint,
     external_key character varying(255),
-    sprint_id bigint,
-    CONSTRAINT check_fba63f706d CHECK ((lock_version IS NOT NULL))
+    sprint_id bigint
 );
 
 CREATE SEQUENCE public.issues_id_seq
@@ -4139,8 +4134,7 @@ CREATE TABLE public.merge_requests (
     state_id smallint DEFAULT 1 NOT NULL,
     rebase_jid character varying,
     squash_commit_sha bytea,
-    sprint_id bigint,
-    CONSTRAINT check_970d272570 CHECK ((lock_version IS NOT NULL))
+    sprint_id bigint
 );
 
 CREATE TABLE public.merge_requests_closing_issues (
@@ -9892,6 +9886,8 @@ CREATE INDEX index_epics_on_iid ON public.epics USING btree (iid);
 
 CREATE INDEX index_epics_on_last_edited_by_id ON public.epics USING btree (last_edited_by_id);
 
+CREATE INDEX index_epics_on_lock_version ON public.epics USING btree (lock_version) WHERE (lock_version IS NULL);
+
 CREATE INDEX index_epics_on_parent_id ON public.epics USING btree (parent_id);
 
 CREATE INDEX index_epics_on_start_date ON public.epics USING btree (start_date);
@@ -10138,6 +10134,8 @@ CREATE INDEX index_issues_on_duplicated_to_id ON public.issues USING btree (dupl
 
 CREATE INDEX index_issues_on_last_edited_by_id ON public.issues USING btree (last_edited_by_id);
 
+CREATE INDEX index_issues_on_lock_version ON public.issues USING btree (lock_version) WHERE (lock_version IS NULL);
+
 CREATE INDEX index_issues_on_milestone_id ON public.issues USING btree (milestone_id);
 
 CREATE INDEX index_issues_on_moved_to_id ON public.issues USING btree (moved_to_id) WHERE (moved_to_id IS NOT NULL);
@@ -10304,6 +10302,8 @@ CREATE INDEX index_merge_requests_on_head_pipeline_id ON public.merge_requests U
 
 CREATE INDEX index_merge_requests_on_latest_merge_request_diff_id ON public.merge_requests USING btree (latest_merge_request_diff_id);
 
+CREATE INDEX index_merge_requests_on_lock_version ON public.merge_requests USING btree (lock_version) WHERE (lock_version IS NULL);
+
 CREATE INDEX index_merge_requests_on_merge_user_id ON public.merge_requests USING btree (merge_user_id) WHERE (merge_user_id IS NOT NULL);
 
 CREATE INDEX index_merge_requests_on_milestone_id ON public.merge_requests USING btree (milestone_id);
@@ -11308,6 +11308,12 @@ CREATE INDEX tmp_build_stage_position_index ON public.ci_builds USING btree (sta
 
 CREATE INDEX tmp_idx_on_user_id_where_bio_is_filled ON public.users USING btree (id) WHERE ((COALESCE(bio, ''::character varying))::text IS DISTINCT FROM ''::text);
 
+CREATE INDEX tmp_index_ci_builds_lock_version ON public.ci_builds USING btree (id) WHERE (lock_version IS NULL);
+
+CREATE INDEX tmp_index_ci_pipelines_lock_version ON public.ci_pipelines USING btree (id) WHERE (lock_version IS NULL);
+
+CREATE INDEX tmp_index_ci_stages_lock_version ON public.ci_stages USING btree (id) WHERE (lock_version IS NULL);
+
 CREATE UNIQUE INDEX users_security_dashboard_projects_unique_index ON public.users_security_dashboard_projects USING btree (project_id, user_id);
 
 CREATE UNIQUE INDEX vulnerability_feedback_unique_idx ON public.vulnerability_feedback USING btree (project_id, category, feedback_type, project_fingerprint);
@@ -14053,14 +14059,6 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200605093113
 20200608072931
 20200608075553
-20200608195222
-20200608205813
-20200608212030
-20200608212435
-20200608212549
-20200608212652
-20200608212807
-20200608212824
 20200608214008
 20200609002841
 20200609142506
@@ -14080,5 +14078,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200617001637
 20200617001848
 20200617002030
+20200618134223
+20200618134723
 \.
 

spec/migrations/cleanup_optimistic_locking_nulls_pt2_fixed_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20200427064130_cleanup_optimistic_locking_nulls_pt2_fixed.rb')
 
-describe CleanupOptimisticLockingNullsPt2Fixed, :migration, schema: 20200219193117 do
+describe CleanupOptimisticLockingNullsPt2Fixed, :migration do
   test_tables = %w(ci_stages ci_builds ci_pipelines).freeze
   test_tables.each do |table|
     let(table.to_sym) { table(table.to_sym) }

spec/models/issue_spec.rb

@@ -95,6 +95,29 @@ describe Issue do
     end
   end
 
+  describe 'locking' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:lock_version) do
+      [
+        [0],
+        ["0"]
+      ]
+    end
+
+    with_them do
+      it 'works when an issue has a NULL lock_version' do
+        issue = create(:issue)
+
+        described_class.where(id: issue.id).update_all('lock_version = NULL')
+
+        issue.update!(lock_version: lock_version, title: 'locking test')
+
+        expect(issue.reload.title).to eq('locking test')
+      end
+    end
+  end
+
   describe '.simple_sorts' do
     it 'includes all keys' do
       expect(described_class.simple_sorts.keys).to include(

spec/models/merge_request_spec.rb

@@ -55,6 +55,29 @@ describe MergeRequest do
     end
   end
 
+  describe 'locking' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:lock_version) do
+      [
+        [0],
+        ["0"]
+      ]
+    end
+
+    with_them do
+      it 'works when a merge request has a NULL lock_version' do
+        merge_request = create(:merge_request)
+
+        described_class.where(id: merge_request.id).update_all('lock_version = NULL')
+
+        merge_request.update!(lock_version: lock_version, title: 'locking test')
+
+        expect(merge_request.reload.title).to eq('locking test')
+      end
+    end
+  end
+
   describe '#squash_in_progress?' do
     let(:repo_path) do
       Gitlab::GitalyClient::StorageSettings.allow_disk_access do