Merge branch '328600-revert-bigint-conversion-helpers' into 'master'

Add migration helpers to revert conversion to bigint

See merge request gitlab-org/gitlab!59957

lib/gitlab/database/migration_helpers.rb

@@ -905,6 +905,10 @@ module Gitlab
         end
       end
 
+      def convert_to_bigint_column(column)
+        "#{column}_convert_to_bigint"
+      end
+
       # Initializes the conversion of a set of integer columns to bigint
       #
       # It can be used for converting both a Primary Key and any Foreign Keys
@@ -948,7 +952,7 @@ module Gitlab
 
         check_trigger_permissions!(table)
 
-        conversions = columns.to_h { |column| [column, "#{column}_convert_to_bigint"] }
+        conversions = columns.to_h { |column| [column, convert_to_bigint_column(column)] }
 
         with_lock_retries do
           conversions.each do |(source_column, temporary_name)|
@@ -969,6 +973,20 @@ module Gitlab
         end
       end
 
+      # Reverts `initialize_conversion_of_integer_to_bigint`
+      #
+      # table - The name of the database table containing the columns
+      # columns - The name, or array of names, of the column(s) that we're converting to bigint.
+      def revert_initialize_conversion_of_integer_to_bigint(table, columns)
+        columns = Array.wrap(columns)
+        temporary_columns = columns.map { |column| convert_to_bigint_column(column) }
+
+        trigger_name = rename_trigger_name(table, columns, temporary_columns)
+        remove_rename_triggers_for_postgresql(table, trigger_name)
+
+        temporary_columns.each { |column| remove_column(table, column) }
+      end
+
       # Backfills the new columns used in an integer-to-bigint conversion using background migrations.
       #
       # - This helper should be called from a post-deployment migration.
@@ -1025,7 +1043,7 @@ module Gitlab
         conversions = Array.wrap(columns).to_h do |column|
           raise ArgumentError, "Column #{column} does not exist on #{table}" unless column_exists?(table, column)
 
-          temporary_name = "#{column}_convert_to_bigint"
+          temporary_name = convert_to_bigint_column(column)
           raise ArgumentError, "Column #{temporary_name} does not exist on #{table}" unless column_exists?(table, temporary_name)
 
           [column, temporary_name]
@@ -1042,6 +1060,25 @@ module Gitlab
           sub_batch_size: sub_batch_size)
       end
 
+      # Reverts `backfill_conversion_of_integer_to_bigint`
+      #
+      # table - The name of the database table containing the column
+      # columns - The name, or an array of names, of the column(s) we want to convert to bigint.
+      # primary_key - The name of the primary key column (most often :id)
+      def revert_backfill_conversion_of_integer_to_bigint(table, columns, primary_key: :id)
+        columns = Array.wrap(columns)
+
+        conditions = ActiveRecord::Base.sanitize_sql([
+          'job_class_name = :job_class_name AND table_name = :table_name AND column_name = :column_name AND job_arguments = :job_arguments',
+          job_class_name: 'CopyColumnUsingBackgroundMigrationJob',
+          table_name: table,
+          column_name: primary_key,
+          job_arguments: [columns, columns.map { |column| convert_to_bigint_column(column) }].to_json
+        ])
+
+        execute("DELETE FROM batched_background_migrations WHERE #{conditions}")
+      end
+
       # Performs a concurrent column rename when using PostgreSQL.
       def install_rename_triggers_for_postgresql(table, old, new, trigger_name: nil)
         Gitlab::Database::UnidirectionalCopyTrigger.on_table(table).create(old, new, trigger_name: trigger_name)

spec/lib/gitlab/background_migration/copy_column_using_background_migration_job_spec.rb

@@ -8,6 +8,10 @@ RSpec.describe Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJo
   let(:sub_batch_size) { 1000 }
   let(:pause_ms) { 0 }
 
+  let(:helpers) do
+    ActiveRecord::Migration.new.extend(Gitlab::Database::MigrationHelpers)
+  end
+
   before do
     ActiveRecord::Base.connection.execute(<<~SQL)
       CREATE TABLE #{table_name}
@@ -15,8 +19,8 @@ RSpec.describe Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJo
        id integer NOT NULL,
        name character varying,
        fk integer NOT NULL,
-       id_convert_to_bigint bigint DEFAULT 0 NOT NULL,
-       fk_convert_to_bigint bigint DEFAULT 0 NOT NULL,
+       #{helpers.convert_to_bigint_column(:id)} bigint DEFAULT 0 NOT NULL,
+       #{helpers.convert_to_bigint_column(:fk)} bigint DEFAULT 0 NOT NULL,
        name_convert_to_text text DEFAULT 'no name'
       );
     SQL
@@ -41,18 +45,20 @@ RSpec.describe Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJo
     let(:migration_class) { described_class.name }
 
     it 'copies all primary keys in range' do
-      copy_columns.perform(12, 15, table_name, 'id', sub_batch_size, pause_ms, 'id', 'id_convert_to_bigint')
+      temporary_column = helpers.convert_to_bigint_column(:id)
+      copy_columns.perform(12, 15, table_name, 'id', sub_batch_size, pause_ms, 'id', temporary_column)
 
-      expect(test_table.where('id = id_convert_to_bigint').pluck(:id)).to contain_exactly(12, 15)
-      expect(test_table.where(id_convert_to_bigint: 0).pluck(:id)).to contain_exactly(11, 19)
+      expect(test_table.where("id = #{temporary_column}").pluck(:id)).to contain_exactly(12, 15)
+      expect(test_table.where(temporary_column => 0).pluck(:id)).to contain_exactly(11, 19)
       expect(test_table.all.count).to eq(4)
     end
 
     it 'copies all foreign keys in range' do
-      copy_columns.perform(10, 14, table_name, 'id', sub_batch_size, pause_ms, 'fk', 'fk_convert_to_bigint')
+      temporary_column = helpers.convert_to_bigint_column(:fk)
+      copy_columns.perform(10, 14, table_name, 'id', sub_batch_size, pause_ms, 'fk', temporary_column)
 
-      expect(test_table.where('fk = fk_convert_to_bigint').pluck(:id)).to contain_exactly(11, 12)
-      expect(test_table.where(fk_convert_to_bigint: 0).pluck(:id)).to contain_exactly(15, 19)
+      expect(test_table.where("fk = #{temporary_column}").pluck(:id)).to contain_exactly(11, 12)
+      expect(test_table.where(temporary_column => 0).pluck(:id)).to contain_exactly(15, 19)
       expect(test_table.all.count).to eq(4)
     end
 
@@ -68,18 +74,20 @@ RSpec.describe Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJo
 
     it 'copies multiple columns when given' do
       columns_to_copy_from = %w[id fk]
-      columns_to_copy_to = %w[id_convert_to_bigint fk_convert_to_bigint]
+      id_tmp_column = helpers.convert_to_bigint_column('id')
+      fk_tmp_column = helpers.convert_to_bigint_column('fk')
+      columns_to_copy_to = [id_tmp_column, fk_tmp_column]
 
       subject.perform(10, 15, table_name, 'id', sub_batch_size, pause_ms, columns_to_copy_from, columns_to_copy_to)
 
-      expect(test_table.where('id = id_convert_to_bigint AND fk = fk_convert_to_bigint').pluck(:id)).to contain_exactly(11, 12, 15)
-      expect(test_table.where(id_convert_to_bigint: 0).where(fk_convert_to_bigint: 0).pluck(:id)).to contain_exactly(19)
+      expect(test_table.where("id = #{id_tmp_column} AND fk = #{fk_tmp_column}").pluck(:id)).to contain_exactly(11, 12, 15)
+      expect(test_table.where(id_tmp_column => 0).where(fk_tmp_column => 0).pluck(:id)).to contain_exactly(19)
       expect(test_table.all.count).to eq(4)
     end
 
     it 'raises error when number of source and target columns does not match' do
       columns_to_copy_from = %w[id fk]
-      columns_to_copy_to = %w[id_convert_to_bigint]
+      columns_to_copy_to = [helpers.convert_to_bigint_column(:id)]
 
       expect do
         subject.perform(10, 15, table_name, 'id', sub_batch_size, pause_ms, columns_to_copy_from, columns_to_copy_to)

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Database::MigrationHelpers do
   include Database::TableSchemaHelpers
+  include Database::TriggerHelpers
 
   let(:model) do
     ActiveRecord::Migration.new.extend(described_class)
@@ -1702,10 +1703,16 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
     end
   end
 
+  describe '#convert_to_bigint_column' do
+    it 'returns the name of the temporary column used to convert to bigint' do
+      expect(model.convert_to_bigint_column(:id)).to eq('id_convert_to_bigint')
+    end
+  end
+
   describe '#initialize_conversion_of_integer_to_bigint' do
     let(:table) { :test_table }
     let(:column) { :id }
-    let(:tmp_column) { "#{column}_convert_to_bigint" }
+    let(:tmp_column) { model.convert_to_bigint_column(column) }
 
     before do
       model.create_table table, id: false do |t|
@@ -1774,11 +1781,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
     context 'when multiple columns are given' do
       it 'creates the correct columns and installs the trigger' do
         columns_to_convert = %i[id non_nullable_column nullable_column]
-        temporary_columns = %w[
-          id_convert_to_bigint
-          non_nullable_column_convert_to_bigint
-          nullable_column_convert_to_bigint
-        ]
+        temporary_columns = columns_to_convert.map { |column| model.convert_to_bigint_column(column) }
 
         expect(model).to receive(:add_column).with(table, temporary_columns[0], :bigint, default: 0, null: false)
         expect(model).to receive(:add_column).with(table, temporary_columns[1], :bigint, default: 0, null: false)
@@ -1791,10 +1794,55 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
     end
   end
 
+  describe '#revert_initialize_conversion_of_integer_to_bigint' do
+    let(:table) { :test_table }
+
+    before do
+      model.create_table table, id: false do |t|
+        t.integer :id, primary_key: true
+        t.integer :other_id
+      end
+
+      model.initialize_conversion_of_integer_to_bigint(table, columns)
+    end
+
+    context 'when single column is given' do
+      let(:columns) { :id }
+
+      it 'removes column, trigger, and function' do
+        temporary_column = model.convert_to_bigint_column(:id)
+        trigger_name = model.rename_trigger_name(table, :id, temporary_column)
+
+        model.revert_initialize_conversion_of_integer_to_bigint(table, columns)
+
+        expect(model.column_exists?(table, temporary_column)).to eq(false)
+        expect_trigger_not_to_exist(table, trigger_name)
+        expect_function_not_to_exist(trigger_name)
+      end
+    end
+
+    context 'when multiple columns are given' do
+      let(:columns) { [:id, :other_id] }
+
+      it 'removes column, trigger, and function' do
+        temporary_columns = columns.map { |column| model.convert_to_bigint_column(column) }
+        trigger_name = model.rename_trigger_name(table, columns, temporary_columns)
+
+        model.revert_initialize_conversion_of_integer_to_bigint(table, columns)
+
+        temporary_columns.each do |column|
+          expect(model.column_exists?(table, column)).to eq(false)
+        end
+        expect_trigger_not_to_exist(table, trigger_name)
+        expect_function_not_to_exist(trigger_name)
+      end
+    end
+  end
+
   describe '#backfill_conversion_of_integer_to_bigint' do
     let(:table) { :_test_backfill_table }
     let(:column) { :id }
-    let(:tmp_column) { "#{column}_convert_to_bigint" }
+    let(:tmp_column) { model.convert_to_bigint_column(column) }
 
     before do
       model.create_table table, id: false do |t|
@@ -1872,14 +1920,14 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
             interval: 120,
             batch_size: 2,
             sub_batch_size: 1,
-            job_arguments: [[column.to_s], ["#{column}_convert_to_bigint"]]
+            job_arguments: [[column.to_s], [model.convert_to_bigint_column(column)]]
           )
         end
       end
 
       context 'when multiple columns are being converted' do
         let(:other_column) { :other_id }
-        let(:other_tmp_column) { "#{other_column}_convert_to_bigint" }
+        let(:other_tmp_column) { model.convert_to_bigint_column(other_column) }
         let(:columns) { [column, other_column] }
 
         it 'creates the batched migration tracking record' do
@@ -1905,6 +1953,54 @@ RSpec.describe Gitlab::Database::MigrationHelpers do
     end
   end
 
+  describe '#revert_backfill_conversion_of_integer_to_bigint' do
+    let(:table) { :_test_backfill_table }
+    let(:primary_key) { :id }
+
+    before do
+      model.create_table table, id: false do |t|
+        t.integer primary_key, primary_key: true
+        t.text :message, null: false
+        t.integer :other_id
+        t.timestamps
+      end
+
+      model.initialize_conversion_of_integer_to_bigint(table, columns, primary_key: primary_key)
+      model.backfill_conversion_of_integer_to_bigint(table, columns, primary_key: primary_key)
+    end
+
+    context 'when a single column is being converted' do
+      let(:columns) { :id }
+
+      it 'deletes the batched migration tracking record' do
+        expect do
+          model.revert_backfill_conversion_of_integer_to_bigint(table, columns)
+        end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(-1)
+      end
+    end
+
+    context 'when a multiple columns are being converted' do
+      let(:columns) { [:id, :other_id] }
+
+      it 'deletes the batched migration tracking record' do
+        expect do
+          model.revert_backfill_conversion_of_integer_to_bigint(table, columns)
+        end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(-1)
+      end
+    end
+
+    context 'when primary key column has custom name' do
+      let(:primary_key) { :other_pk }
+      let(:columns) { :other_id }
+
+      it 'deletes the batched migration tracking record' do
+        expect do
+          model.revert_backfill_conversion_of_integer_to_bigint(table, columns, primary_key: primary_key)
+        end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(-1)
+      end
+    end
+  end
+
   describe '#index_exists_by_name?' do
     it 'returns true if an index exists' do
       ActiveRecord::Base.connection.execute(