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

Remove LOCK_VERSION monkeypatch

See merge request gitlab-org/gitlab!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

+# 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/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/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

@@ -1052,7 +1052,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
@@ -1362,7 +1363,8 @@ CREATE TABLE public.ci_pipelines (
     source_sha bytea,
     target_sha bytea,
     external_pull_request_id bigint,
-    ci_ref_id bigint
+    ci_ref_id bigint,
+    CONSTRAINT check_d7e99a025e CHECK ((lock_version IS NOT NULL))
 );
 
 CREATE TABLE public.ci_pipelines_config (
@@ -1547,7 +1549,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
@@ -2506,7 +2509,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
@@ -3532,7 +3536,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
@@ -4111,7 +4116,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 (
@@ -9835,8 +9841,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);
@@ -10079,8 +10083,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);
@@ -10247,8 +10249,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);
@@ -11249,12 +11249,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 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);
@@ -13989,6 +13983,14 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200605093113
 20200608072931
 20200608075553
+20200608195222
+20200608205813
+20200608212030
+20200608212435
+20200608212549
+20200608212652
+20200608212807
+20200608212824
 20200608214008
 20200609002841
 20200609142506

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 do
+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 @@ 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,29 +55,6 @@ 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