Set all NULL `lock_version` values to 0 Part 1

Rails silently casts NULL `lock_version` values to 0 while doing
optimistic locking, which causes false stale object exceptions. We had a
monkey patch that would change it to check for [NULL, 0] but want to
avoid monkey-patching if possible, which means we need to clean up our
database values.

This commit deals with Issuables (Issues, Epics, Merge Requests)

changelogs/unreleased/28560_cleanup_optimistic_locking_db.yml

+---
+title: Set all NULL `lock_version` values to 0 for issuables
+merge_request: 18418
+author:
+type: fixed

db/post_migrate/20200128210353_cleanup_optimistic_locking_nulls.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class CleanupOptimisticLockingNulls < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  TABLES = %w(epics merge_requests issues).freeze
+  BATCH_SIZE = 10_000
+
+  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|
+      add_concurrent_index table.to_sym, :lock_version, where: "lock_version IS NULL"
+
+      queue_background_migration_jobs_by_range_at_intervals(
+        declare_class(table).where(lock_version: nil),
+        'CleanupOptimisticLockingNulls',
+        2.minutes,
+        batch_size: BATCH_SIZE,
+        other_arguments: [table]
+      )
+    end
+  end
+
+  def down
+    TABLES.each do |table|
+      remove_concurrent_index table.to_sym, :lock_version, where: "lock_version IS NULL"
+    end
+  end
+end

db/schema.rb

@@ -1571,6 +1571,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_220211) do
     t.index ["end_date"], name: "index_epics_on_end_date"
     t.index ["group_id"], name: "index_epics_on_group_id"
     t.index ["iid"], name: "index_epics_on_iid"
+    t.index ["lock_version"], name: "index_epics_on_lock_version", where: "(lock_version IS NULL)"
     t.index ["parent_id"], name: "index_epics_on_parent_id"
     t.index ["start_date"], name: "index_epics_on_start_date"
     t.index ["start_date_sourcing_epic_id"], name: "index_epics_on_start_date_sourcing_epic_id", where: "(start_date_sourcing_epic_id IS NOT NULL)"
@@ -2188,6 +2189,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_220211) do
     t.index ["confidential"], name: "index_issues_on_confidential"
     t.index ["description"], name: "index_issues_on_description_trigram", opclass: :gin_trgm_ops, using: :gin
     t.index ["duplicated_to_id"], name: "index_issues_on_duplicated_to_id", where: "(duplicated_to_id IS NOT NULL)"
+    t.index ["lock_version"], name: "index_issues_on_lock_version", where: "(lock_version IS NULL)"
     t.index ["milestone_id"], name: "index_issues_on_milestone_id"
     t.index ["moved_to_id"], name: "index_issues_on_moved_to_id", where: "(moved_to_id IS NOT NULL)"
     t.index ["project_id", "created_at", "id", "state"], name: "index_issues_on_project_id_and_created_at_and_id_and_state"
@@ -2606,6 +2608,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_220211) do
     t.index ["id", "merge_jid"], name: "idx_merge_requests_on_id_and_merge_jid", where: "((merge_jid IS NOT NULL) AND (state_id = 4))"
     t.index ["id", "merge_jid"], name: "index_merge_requests_on_id_and_merge_jid", where: "((merge_jid IS NOT NULL) AND ((state)::text = 'locked'::text))"
     t.index ["latest_merge_request_diff_id"], name: "index_merge_requests_on_latest_merge_request_diff_id"
+    t.index ["lock_version"], name: "index_merge_requests_on_lock_version", where: "(lock_version IS NULL)"
     t.index ["merge_user_id"], name: "index_merge_requests_on_merge_user_id", where: "(merge_user_id IS NOT NULL)"
     t.index ["milestone_id"], name: "index_merge_requests_on_milestone_id"
     t.index ["source_branch"], name: "index_merge_requests_on_source_branch"

lib/gitlab/background_migration/cleanup_optimistic_locking_nulls.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class CleanupOptimisticLockingNulls
+      QUERY_ITEM_SIZE = 1_000
+
+      # table - The name of the table the migration is performed for.
+      # start_id - The ID of the object to start at
+      # stop_id - The ID of the object to end at
+      def perform(start_id, stop_id, table)
+        model = define_model_for(table)
+
+        # After analysis done, a batch size of 1,000 items per query was found to be
+        # the most optimal. Discussion in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/18418#note_282285336
+        (start_id..stop_id).each_slice(QUERY_ITEM_SIZE).each do |range|
+          model
+              .where(lock_version: nil)
+              .where(id: range)
+              .update_all(lock_version: 0)
+        end
+      end
+
+      def define_model_for(table)
+        Class.new(ActiveRecord::Base) do
+          self.table_name = table
+        end
+      end
+    end
+  end
+end

lib/gitlab/database/migration_helpers.rb

@@ -1042,6 +1042,7 @@ into similar problems in the future (e.g. when new tables are created).
       # job_class_name - The background migration job class as a string
       # delay_interval - The duration between each job's scheduled time (must respond to `to_f`)
       # batch_size - The maximum number of rows per job
+      # other_arguments - Other arguments to send to the job
       #
       # Example:
       #
@@ -1059,7 +1060,7 @@ into similar problems in the future (e.g. when new tables are created).
       #         # do something
       #       end
       #     end
-      def queue_background_migration_jobs_by_range_at_intervals(model_class, job_class_name, delay_interval, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE)
+      def queue_background_migration_jobs_by_range_at_intervals(model_class, job_class_name, delay_interval, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE, other_arguments: [])
         raise "#{model_class} does not have an ID to use for batch ranges" unless model_class.column_names.include?('id')
 
         # To not overload the worker too much we enforce a minimum interval both
@@ -1074,7 +1075,7 @@ into similar problems in the future (e.g. when new tables are created).
           # `BackgroundMigrationWorker.bulk_perform_in` schedules all jobs for
           # the same time, which is not helpful in most cases where we wish to
           # spread the work over time.
-          migrate_in(delay_interval * index, job_class_name, [start_id, end_id])
+          migrate_in(delay_interval * index, job_class_name, [start_id, end_id] + other_arguments)
         end
       end
 

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -1332,6 +1332,15 @@ describe Gitlab::Database::MigrationHelpers do
           end
         end
       end
+
+      context 'with other_arguments option' do
+        it 'queues jobs correctly' do
+          model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, other_arguments: [1, 2])
+
+          expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3, 1, 2]])
+          expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.minutes.from_now.to_f)
+        end
+      end
     end
 
     context "when the model doesn't have an ID column" do

spec/migrations/cleanup_optimistic_locking_nulls_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200128210353_cleanup_optimistic_locking_nulls')
+
+describe CleanupOptimisticLockingNulls, :migration do
+  TABLES = %w(epics merge_requests issues).freeze
+  TABLES.each do |table|
+    let(table.to_sym) { table(table.to_sym) }
+  end
+  let(:tables) { TABLES.map { |t| method(t.to_sym).call } }
+
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:users) { table(:users)}
+
+  before do
+    namespaces.create!(id: 123, name: 'gitlab1', path: 'gitlab1')
+    projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1', namespace_id: 123)
+    users.create!(id: 123, username: 'author', projects_limit: 1000)
+
+    # Create necessary rows
+    epics.create!(iid: 123, group_id: 123, author_id: 123, title: 'a', title_html: 'a')
+    merge_requests.create!(iid: 123, target_project_id: 123, source_project_id: 123, target_branch: 'master', source_branch: 'hmm', title: 'a', title_html: 'a')
+    issues.create!(iid: 123, project_id: 123, title: 'a', title_html: 'a')
+
+    # Nullify `lock_version` column for all rows
+    # Needs to be done with a SQL fragment, otherwise Rails will coerce it to 0
+    tables.each do |table|
+      table.update_all('lock_version = NULL')
+    end
+  end
+
+  it 'correctly migrates nullified lock_version column', :sidekiq_inline do
+    tables.each do |table|
+      expect(table.where(lock_version: nil).count).to eq(1)
+    end
+
+    tables.each do |table|
+      expect(table.where(lock_version: 0).count).to eq(0)
+    end
+
+    migrate!
+
+    tables.each do |table|
+      expect(table.where(lock_version: nil).count).to eq(0)
+    end
+
+    tables.each do |table|
+      expect(table.where(lock_version: 0).count).to eq(1)
+    end
+  end
+end