Convert todos.note_id to bigint for self-managed

For self-managed instances:
1. Ensure the `note_id` to `note_id_convert_to_bigint`
conversion is completed
2. Copy indexes and foreign keys
3. Swap columns

Also take into account the possible states of the database schema,
as explained in
!125837.

See #417402.

Changelog: other

db/post_migrate/20230710024518_ensure_todos_bigint_backfill_completed_for_self_managed.rb

+# frozen_string_literal: true
+
+class EnsureTodosBigintBackfillCompletedForSelfManaged < Gitlab::Database::Migration[2.1]
+  include Gitlab::Database::MigrationHelpers::ConvertToBigint
+
+  restrict_gitlab_migration gitlab_schema: :gitlab_main
+  disable_ddl_transaction!
+
+  def up
+    return if should_skip?
+
+    ensure_batched_background_migration_is_finished(
+      job_class_name: 'CopyColumnUsingBackgroundMigrationJob',
+      table_name: 'todos',
+      column_name: 'id',
+      job_arguments: [['note_id'], ['note_id_convert_to_bigint']]
+    )
+  end
+
+  def down
+    # no-op
+  end
+
+  private
+
+  def should_skip?
+    com_or_dev_or_test_but_not_jh?
+  end
+end

db/post_migrate/20230710024903_swap_todos_note_id_to_bigint_for_self_managed.rb

+# frozen_string_literal: true
+
+class SwapTodosNoteIdToBigintForSelfManaged < Gitlab::Database::Migration[2.1]
+  include Gitlab::Database::MigrationHelpers::ConvertToBigint
+
+  disable_ddl_transaction!
+
+  TABLE_NAME = 'todos'
+
+  def up
+    return if should_skip?
+    return if temp_column_removed?(TABLE_NAME, :note_id)
+    return if columns_swapped?(TABLE_NAME, :note_id)
+
+    swap
+  end
+
+  def down
+    return if should_skip?
+    return if temp_column_removed?(TABLE_NAME, :note_id)
+    return unless columns_swapped?(TABLE_NAME, :note_id)
+
+    swap
+  end
+
+  def swap
+    # This will replace the existing index_todos_on_note_id
+    add_concurrent_index TABLE_NAME, :note_id_convert_to_bigint,
+      name: 'index_todos_on_note_id_convert_to_bigint'
+
+    # This will replace the existing fk_91d1f47b13
+    add_concurrent_foreign_key TABLE_NAME, :notes, column: :note_id_convert_to_bigint,
+      name: 'fk_todos_note_id_convert_to_bigint',
+      on_delete: :cascade
+
+    with_lock_retries(raise_on_exhaustion: true) do
+      execute "LOCK TABLE notes, #{TABLE_NAME} IN ACCESS EXCLUSIVE MODE"
+
+      execute "ALTER TABLE #{TABLE_NAME} RENAME COLUMN note_id TO note_id_tmp"
+      execute "ALTER TABLE #{TABLE_NAME} RENAME COLUMN note_id_convert_to_bigint TO note_id"
+      execute "ALTER TABLE #{TABLE_NAME} RENAME COLUMN note_id_tmp TO note_id_convert_to_bigint"
+
+      function_name = Gitlab::Database::UnidirectionalCopyTrigger
+        .on_table(TABLE_NAME, connection: connection)
+        .name(:note_id, :note_id_convert_to_bigint)
+      execute "ALTER FUNCTION #{quote_table_name(function_name)} RESET ALL"
+
+      execute 'DROP INDEX IF EXISTS index_todos_on_note_id'
+      rename_index TABLE_NAME, 'index_todos_on_note_id_convert_to_bigint',
+        'index_todos_on_note_id'
+
+      execute "ALTER TABLE #{TABLE_NAME} DROP CONSTRAINT IF EXISTS fk_91d1f47b13"
+      rename_constraint(TABLE_NAME, 'fk_todos_note_id_convert_to_bigint', 'fk_91d1f47b13')
+    end
+  end
+
+  private
+
+  def should_skip?
+    com_or_dev_or_test_but_not_jh?
+  end
+end

db/schema_migrations/20230710024518

+11c031dc6fe6cf48b4268adf64f147d5447f79e53543b3fcb57196f99092af8b
\ No newline at end of file

db/schema_migrations/20230710024903

+23d1b7e65b4e2742a25063dc277e73e9b48151e038d1079316209aed6d9c8199
\ No newline at end of file

lib/gitlab/database/migration_helpers/convert_to_bigint.rb

@@ -15,6 +15,20 @@ def com_or_dev_or_test_but_not_jh?
 
           Gitlab.com? && !Gitlab.jh?
         end
+
+        def temp_column_removed?(table_name, column_name)
+          !column_exists?(table_name.to_s, convert_to_bigint_column(column_name))
+        end
+
+        def columns_swapped?(table_name, column_name)
+          table_columns = columns(table_name.to_s)
+          temp_column_name = convert_to_bigint_column(column_name)
+
+          column = table_columns.find { |c| c.name == column_name.to_s }
+          temp_column = table_columns.find { |c| c.name == temp_column_name }
+
+          column.sql_type == 'bigint' && temp_column.sql_type == 'integer'
+        end
       end
     end
   end

spec/lib/gitlab/database/migration_helpers/convert_to_bigint_spec.rb

@@ -3,7 +3,15 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::MigrationHelpers::ConvertToBigint, feature_category: :database do
-  describe 'com_or_dev_or_test_but_not_jh?' do
+  let(:migration) do
+    Class
+      .new
+      .include(described_class)
+      .include(Gitlab::Database::MigrationHelpers)
+      .new
+  end
+
+  describe '#com_or_dev_or_test_but_not_jh?' do
     using RSpec::Parameterized::TableSyntax
 
     where(:dot_com, :dev_or_test, :jh, :expectation) do
@@ -23,13 +31,46 @@
         allow(Gitlab).to receive(:dev_or_test_env?).and_return(dev_or_test)
         allow(Gitlab).to receive(:jh?).and_return(jh)
 
-        migration = Class
-          .new
-          .include(Gitlab::Database::MigrationHelpers::ConvertToBigint)
-          .new
-
         expect(migration.com_or_dev_or_test_but_not_jh?).to eq(expectation)
       end
     end
   end
+
+  describe '#temp_column_removed?' do
+    it 'return true when column is not present' do
+      expect(migration).to receive(:column_exists?).with('test_table', 'id_convert_to_bigint').and_return(false)
+
+      expect(migration.temp_column_removed?(:test_table, :id)).to eq(true)
+    end
+
+    it 'return false when column present' do
+      expect(migration).to receive(:column_exists?).with('test_table', 'id_convert_to_bigint').and_return(true)
+
+      expect(migration.temp_column_removed?(:test_table, :id)).to eq(false)
+    end
+  end
+
+  describe '#columns_swapped?' do
+    it 'returns true if columns are already swapped' do
+      columns = [
+        Struct.new(:name, :sql_type).new('id', 'bigint'),
+        Struct.new(:name, :sql_type).new('id_convert_to_bigint', 'integer')
+      ]
+
+      expect(migration).to receive(:columns).with('test_table').and_return(columns)
+
+      expect(migration.columns_swapped?(:test_table, :id)).to eq(true)
+    end
+
+    it 'returns false if columns are not yet swapped' do
+      columns = [
+        Struct.new(:name, :sql_type).new('id', 'integer'),
+        Struct.new(:name, :sql_type).new('id_convert_to_bigint', 'bigint')
+      ]
+
+      expect(migration).to receive(:columns).with('test_table').and_return(columns)
+
+      expect(migration.columns_swapped?(:test_table, :id)).to eq(false)
+    end
+  end
 end

spec/migrations/ensure_todos_bigint_backfill_completed_for_self_managed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe EnsureTodosBigintBackfillCompletedForSelfManaged, feature_category: :database do
+  describe '#up' do
+    let(:migration_arguments) do
+      {
+        job_class_name: 'CopyColumnUsingBackgroundMigrationJob',
+        table_name: 'todos',
+        column_name: 'id',
+        job_arguments: [['note_id'], ['note_id_convert_to_bigint']]
+      }
+    end
+
+    it 'ensures the migration is completed for self-managed instances' do
+      expect_next_instance_of(described_class) do |instance|
+        expect(instance).to receive(:com_or_dev_or_test_but_not_jh?).and_return(false)
+        expect(instance).to receive(:ensure_batched_background_migration_is_finished).with(migration_arguments)
+      end
+
+      migrate!
+    end
+
+    it 'skips the check for GitLab.com, dev, or test' do
+      expect_next_instance_of(described_class) do |instance|
+        expect(instance).to receive(:com_or_dev_or_test_but_not_jh?).and_return(true)
+        expect(instance).not_to receive(:ensure_batched_background_migration_is_finished)
+      end
+
+      migrate!
+    end
+  end
+end

spec/migrations/swap_todos_note_id_to_bigint_for_self_managed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe SwapTodosNoteIdToBigintForSelfManaged, feature_category: :database do
+  describe '#up' do
+    context 'when GitLab.com, dev, or test' do
+      before do
+        # As we call `schema_migrate_down!` before each example, and for this migration
+        # `#down` is same as `#up`, we need to ensure we start from the expected state.
+        connection = described_class.new.connection
+        connection.execute('ALTER TABLE todos ALTER COLUMN note_id TYPE bigint')
+        connection.execute('ALTER TABLE todos DROP COLUMN IF EXISTS note_id_convert_to_bigint')
+      end
+
+      it 'does not swap the columns' do
+        # rubocop: disable RSpec/AnyInstanceOf
+        allow_any_instance_of(described_class).to receive(:com_or_dev_or_test_but_not_jh?).and_return(true)
+        # rubocop: enable RSpec/AnyInstanceOf
+
+        todos = table(:todos)
+
+        disable_migrations_output do
+          reversible_migration do |migration|
+            migration.before -> {
+              todos.reset_column_information
+
+              expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('bigint')
+              expect(todos.columns.find { |c| c.name == 'note_id_convert_to_bigint' }).to be nil
+            }
+
+            migration.after -> {
+              todos.reset_column_information
+
+              expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('bigint')
+              expect(todos.columns.find { |c| c.name == 'note_id_convert_to_bigint' }).to be nil
+            }
+          end
+        end
+      end
+    end
+
+    context 'when self-managed instance with the columns already swapped' do
+      before do
+        # As we call `schema_migrate_down!` before each example, and for this migration
+        # `#down` is same as `#up`, we need to ensure we start from the expected state.
+        connection = described_class.new.connection
+        connection.execute('ALTER TABLE todos ALTER COLUMN note_id TYPE bigint')
+        connection.execute('ALTER TABLE todos ADD COLUMN IF NOT EXISTS note_id_convert_to_bigint integer')
+      end
+
+      it 'does not swap the columns' do
+        # rubocop: disable RSpec/AnyInstanceOf
+        allow_any_instance_of(described_class).to receive(:com_or_dev_or_test_but_not_jh?).and_return(false)
+        # rubocop: enable RSpec/AnyInstanceOf
+
+        todos = table(:todos)
+
+        migrate!
+
+        expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('bigint')
+        expect(todos.columns.find do |c|
+          c.name == 'note_id_convert_to_bigint'
+        end.sql_type).to eq('integer')
+      end
+    end
+
+    context 'when self-managed instance with the `note_id_convert_to_bigint` column already dropped ' do
+      before do
+        # As we call `schema_migrate_down!` before each example, and for this migration
+        # `#down` is same as `#up`, we need to ensure we start from the expected state.
+        connection = described_class.new.connection
+        connection.execute('ALTER TABLE todos ALTER COLUMN note_id TYPE bigint')
+        connection.execute('ALTER TABLE todos DROP COLUMN IF EXISTS note_id_convert_to_bigint')
+      end
+
+      it 'does not swap the columns' do
+        # rubocop: disable RSpec/AnyInstanceOf
+        allow_any_instance_of(described_class).to receive(:com_or_dev_or_test_but_not_jh?).and_return(false)
+        # rubocop: enable RSpec/AnyInstanceOf
+
+        todos = table(:todos)
+
+        disable_migrations_output do
+          reversible_migration do |migration|
+            migration.before -> {
+              todos.reset_column_information
+
+              expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('bigint')
+              expect(todos.columns.find { |c| c.name == 'note_id_convert_to_bigint' }).to be nil
+            }
+
+            migration.after -> {
+              todos.reset_column_information
+
+              expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('bigint')
+              expect(todos.columns.find { |c| c.name == 'note_id_convert_to_bigint' }).to be nil
+            }
+          end
+        end
+      end
+    end
+
+    context 'when self-managed instance' do
+      before do
+        # As we call `schema_migrate_down!` before each example, and for this migration
+        # `#down` is same as `#up`, we need to ensure we start from the expected state.
+        connection = described_class.new.connection
+        connection.execute('ALTER TABLE todos ALTER COLUMN note_id TYPE integer')
+        connection.execute('ALTER TABLE todos ADD COLUMN IF NOT EXISTS note_id_convert_to_bigint bigint')
+        connection.execute('ALTER TABLE todos ALTER COLUMN note_id_convert_to_bigint TYPE bigint')
+        connection.execute('DROP INDEX IF EXISTS index_todos_on_note_id_convert_to_bigint')
+        connection.execute('CREATE OR REPLACE FUNCTION trigger_dca935e3a712() RETURNS trigger LANGUAGE plpgsql AS $$
+          BEGIN NEW."note_id_convert_to_bigint" := NEW."note_id"; RETURN NEW; END; $$;')
+      end
+
+      it 'swaps the columns' do
+        # rubocop: disable RSpec/AnyInstanceOf
+        allow_any_instance_of(described_class).to receive(:com_or_dev_or_test_but_not_jh?).and_return(false)
+        # rubocop: enable RSpec/AnyInstanceOf
+
+        todos = table(:todos)
+
+        disable_migrations_output do
+          reversible_migration do |migration|
+            migration.before -> {
+              todos.reset_column_information
+
+              expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('integer')
+              expect(todos.columns.find do |c|
+                       c.name == 'note_id_convert_to_bigint'
+                     end.sql_type).to eq('bigint')
+            }
+
+            migration.after -> {
+              todos.reset_column_information
+
+              expect(todos.columns.find { |c| c.name == 'note_id' }.sql_type).to eq('bigint')
+              expect(todos.columns.find do |c|
+                       c.name == 'note_id_convert_to_bigint'
+                     end.sql_type).to eq('integer')
+            }
+          end
+        end
+      end
+    end
+  end
+end