Remove LOCK_VERSION monkeypatch

This removes the monkeypatch for Rails optimistic locking by setting the
relevant rows to be NOT NULL and then removing the extra code.

In past commits we've migrated the data and set defaults for all columns

changelogs/unreleased/223041-lock_version_monkeypatch_removal_unrevert.yml

+---
+title: Remove Rails Optimistic Locking monkeypatch
+merge_request: 36893
+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

+# frozen_string_literal: true
+
+class SetLockVersionNotNullConstraint < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  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
+    end
+  end
+
+  def down
+    TABLES.each do |table|
+      remove_not_null_constraint table, :lock_version
+    end
+  end
+end

db/post_migrate/20200608203426_set_proper_lock_version_indices.rb

+# frozen_string_literal: true
+
+class SetProperLockVersionIndices < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :epics, :lock_version, where: "lock_version IS NULL"
+    remove_concurrent_index :merge_requests, :lock_version, where: "lock_version IS NULL"
+    remove_concurrent_index :issues, :lock_version, where: "lock_version IS NULL"
+
+    add_concurrent_index :epics, :id, where: "lock_version IS NULL"
+    add_concurrent_index :merge_requests, :id, where: "lock_version IS NULL"
+    add_concurrent_index :issues, :id, where: "lock_version IS NULL"
+  end
+
+  def down
+    add_concurrent_index :epics, :lock_version, where: "lock_version IS NULL"
+    add_concurrent_index :merge_requests, :lock_version, where: "lock_version IS NULL"
+    add_concurrent_index :issues, :lock_version, where: "lock_version IS NULL"
+
+    remove_concurrent_index :epics, :id, where: "lock_version IS NULL"
+    remove_concurrent_index :merge_requests, :id, where: "lock_version IS NULL"
+    remove_concurrent_index :issues, :id, where: "lock_version IS NULL"
+  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
+
+  TABLES = %w(epics merge_requests issues ci_stages ci_builds ci_pipelines).freeze
+  BATCH_SIZE = 10_000
+
+  disable_ddl_transaction!
+
+  def declare_class(table)
+    Class.new(ActiveRecord::Base) do
+      include EachBatch
+
+      self.table_name = table
+      self.inheritance_column = :_type_disabled # Disable STI
+    end
+  end
+
+  def up
+    TABLES.each do |table|
+      declare_class(table).where(lock_version: nil).each_batch(of: BATCH_SIZE) do |batch|
+        batch.update_all(lock_version: 0)
+      end
+    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, :id, where: "lock_version IS NULL"
+  end
+
+  def down
+    add_concurrent_index :epics, :id, 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, :id, where: "lock_version IS NULL"
+  end
+
+  def down
+    add_concurrent_index :merge_requests, :id, 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, :id, where: "lock_version IS NULL"
+  end
+
+  def down
+    add_concurrent_index :issues, :id, 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/post_migrate/20200608212824_lock_version_cleanup_for_ci_pipelines.rb

+# frozen_string_literal: true
+
+class LockVersionCleanupForCiPipelines < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  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"
+  end
+
+  def down
+    add_concurrent_index :ci_pipelines, :id, where: "lock_version IS NULL", name: "tmp_index_ci_pipelines_lock_version"
+  end
+end

db/structure.sql

@@ -9785,7 +9785,8 @@ CREATE TABLE public.ci_builds (
     resource_group_id bigint,
     waiting_for_resource_at timestamp with time zone,
     processed boolean,
-    scheduling_type smallint
+    scheduling_type smallint,
+    CONSTRAINT check_1e2fbd1b39 CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE SEQUENCE public.ci_builds_id_seq
@@ -10095,7 +10096,8 @@ CREATE TABLE public.ci_pipelines (
     target_sha bytea,
     external_pull_request_id bigint,
     ci_ref_id bigint,
-    locked smallint DEFAULT 0 NOT NULL
+    locked smallint DEFAULT 0 NOT NULL,
+    CONSTRAINT check_d7e99a025e CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE TABLE public.ci_pipelines_config (
@@ -10280,7 +10282,8 @@ CREATE TABLE public.ci_stages (
     name character varying,
     status integer,
     lock_version integer DEFAULT 0,
-    "position" integer
+    "position" integer,
+    CONSTRAINT check_81b431e49b CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE SEQUENCE public.ci_stages_id_seq
@@ -11304,7 +11307,8 @@ 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)
+    external_key character varying(255),
+    CONSTRAINT check_fcfb4a93ff CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE SEQUENCE public.epics_id_seq
@@ -12347,7 +12351,8 @@ CREATE TABLE public.issues (
     promoted_to_epic_id integer,
     health_status smallint,
     external_key character varying(255),
-    sprint_id bigint
+    sprint_id bigint,
+    CONSTRAINT check_fba63f706d CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE SEQUENCE public.issues_id_seq
@@ -12930,7 +12935,8 @@ CREATE TABLE public.merge_requests (
     state_id smallint DEFAULT 1 NOT NULL,
     rebase_jid character varying,
     squash_commit_sha bytea,
-    sprint_id bigint
+    sprint_id bigint,
+    CONSTRAINT check_970d272570 CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE TABLE public.merge_requests_closing_issues (
@@ -19176,8 +19182,6 @@ 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);
@@ -19424,8 +19428,6 @@ 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);
@@ -19588,8 +19590,6 @@ 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);
@@ -20612,12 +20612,6 @@ 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 INDEX tmp_index_for_email_unconfirmation_migration ON public.emails USING btree (id) WHERE (confirmed_at IS NOT NULL);
 
 CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON public.merge_request_metrics USING btree (merge_request_id);
@@ -23683,6 +23677,15 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200605160851
 20200608072931
 20200608075553
+20200608195222
+20200608203426
+20200608205813
+20200608212030
+20200608212435
+20200608212549
+20200608212652
+20200608212807
+20200608212824
 20200608214008
 20200609002841
 20200609012539

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')
 
-RSpec.describe CleanupOptimisticLockingNullsPt2Fixed, :migration do
+RSpec.describe CleanupOptimisticLockingNullsPt2Fixed, :migration, schema: 20200219193117 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,29 +95,6 @@ RSpec.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

@@ -57,29 +57,6 @@ RSpec.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