diff --git a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb index ac65e8d745c..cce8942128c 100644 --- a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb +++ b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb @@ -12,6 +12,6 @@ class RenameAllowLocalRequestsFromHooksAndServicesApplicationSetting < ActiveRec end def down - cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + undo_rename_column_concurrently :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services end end diff --git a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb index 127e44254ac..cb86f843f9c 100644 --- a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb +++ b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb @@ -12,6 +12,6 @@ class CleanupAllowLocalRequestsFromHooksAndServicesApplicationSettingRename < Ac end def down - rename_column_concurrently :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + undo_cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services end end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 8bc45f6e78c..57a413f8e04 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -470,7 +470,7 @@ module Gitlab # We set the default value _after_ adding the column so we don't end up # updating any existing data with the default value. This isn't # necessary since we copy over old values further down. - change_column_default(table, new, old_col.default) if old_col.default + change_column_default(table, new, old_col.default) unless old_col.default.nil? install_rename_triggers(table, old, new) @@ -482,6 +482,16 @@ module Gitlab copy_foreign_keys(table, old, new) end + def undo_rename_column_concurrently(table, old, new) + trigger_name = rename_trigger_name(table, old, new) + + check_trigger_permissions!(table) + + remove_rename_triggers_for_postgresql(table, trigger_name) + + remove_column(table, new) + end + # Installs triggers in a table that keep a new column in sync with an old # one. # @@ -547,6 +557,35 @@ module Gitlab remove_column(table, old) end + def undo_cleanup_concurrent_column_rename(table, old, new, type: nil) + if transaction_open? + raise 'undo_cleanup_concurrent_column_rename can not be run inside a transaction' + end + + check_trigger_permissions!(table) + + new_column = column_for(table, new) + + add_column(table, old, type || new_column.type, + limit: new_column.limit, + precision: new_column.precision, + scale: new_column.scale) + + # We set the default value _after_ adding the column so we don't end up + # updating any existing data with the default value. This isn't + # necessary since we copy over old values further down. + change_column_default(table, old, new_column.default) unless new_column.default.nil? + + install_rename_triggers(table, old, new) + + update_column_in_batches(table, old, Arel::Table.new(table)[new]) + + change_column_null(table, old, false) unless new_column.null + + copy_indexes(table, new, old) + copy_foreign_keys(table, new, old) + end + # Changes the column type of a table using a background migration. # # Because this method uses a background migration it's more suitable for diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index fe44b09aa24..cff4eb398bf 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -576,6 +576,38 @@ describe Gitlab::Database::MigrationHelpers do model.rename_column_concurrently(:users, :old, :new) end + + context 'when default is false' do + let(:old_column) do + double(:column, + type: :boolean, + limit: nil, + default: false, + null: false, + precision: nil, + scale: nil) + end + + it 'copies the default to the new column' do + expect(model).to receive(:change_column_default) + .with(:users, :new, old_column.default) + + model.rename_column_concurrently(:users, :old, :new) + end + end + end + end + + describe '#undo_rename_column_concurrently' do + it 'reverses the operations of rename_column_concurrently' do + expect(model).to receive(:check_trigger_permissions!).with(:users) + + expect(model).to receive(:remove_rename_triggers_for_postgresql) + .with(:users, /trigger_.{12}/) + + expect(model).to receive(:remove_column).with(:users, :new) + + model.undo_rename_column_concurrently(:users, :old, :new) end end @@ -592,6 +624,80 @@ describe Gitlab::Database::MigrationHelpers do end end + describe '#undo_cleanup_concurrent_column_rename' do + context 'in a transaction' do + it 'raises RuntimeError' do + allow(model).to receive(:transaction_open?).and_return(true) + + expect { model.undo_cleanup_concurrent_column_rename(:users, :old, :new) } + .to raise_error(RuntimeError) + end + end + + context 'outside a transaction' do + let(:new_column) do + double(:column, + type: :integer, + limit: 8, + default: 0, + null: false, + precision: 5, + scale: 1) + end + + let(:trigger_name) { model.rename_trigger_name(:users, :old, :new) } + + before do + allow(model).to receive(:transaction_open?).and_return(false) + allow(model).to receive(:column_for).and_return(new_column) + end + + it 'reverses the operations of cleanup_concurrent_column_rename' do + expect(model).to receive(:check_trigger_permissions!).with(:users) + + expect(model).to receive(:install_rename_triggers_for_postgresql) + .with(trigger_name, '"users"', '"old"', '"new"') + + expect(model).to receive(:add_column) + .with(:users, :old, :integer, + limit: new_column.limit, + precision: new_column.precision, + scale: new_column.scale) + + expect(model).to receive(:change_column_default) + .with(:users, :old, new_column.default) + + expect(model).to receive(:update_column_in_batches) + + expect(model).to receive(:change_column_null).with(:users, :old, false) + + expect(model).to receive(:copy_indexes).with(:users, :new, :old) + expect(model).to receive(:copy_foreign_keys).with(:users, :new, :old) + + model.undo_cleanup_concurrent_column_rename(:users, :old, :new) + end + + context 'when default is false' do + let(:new_column) do + double(:column, + type: :boolean, + limit: nil, + default: false, + null: false, + precision: nil, + scale: nil) + end + + it 'copies the default to the old column' do + expect(model).to receive(:change_column_default) + .with(:users, :old, new_column.default) + + model.undo_cleanup_concurrent_column_rename(:users, :old, :new) + end + end + end + end + describe '#change_column_type_concurrently' do it 'changes the column type' do expect(model).to receive(:rename_column_concurrently)