From 43d33ef82421314b1ae0fb3bbe48b0def3084adb Mon Sep 17 00:00:00 2001 From: Reuben Pereira Date: Thu, 5 Sep 2019 12:15:16 +0000 Subject: Refactor new undo_* methods - Move code for creating a new column from old into a function so that it can be reused. - Also add comments above the methods. --- doc/development/what_requires_downtime.md | 4 +- lib/gitlab/database/migration_helpers.rb | 81 +++++++++++++++++-------------- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/doc/development/what_requires_downtime.md b/doc/development/what_requires_downtime.md index f4cee410066..00371057d3c 100644 --- a/doc/development/what_requires_downtime.md +++ b/doc/development/what_requires_downtime.md @@ -88,7 +88,7 @@ class RenameUsersUpdatedAtToUpdatedAtTimestamp < ActiveRecord::Migration[4.2] end def down - cleanup_concurrent_column_rename :users, :updated_at_timestamp, :updated_at + undo_rename_column_concurrently :users, :updated_at, :updated_at_timestamp end end ``` @@ -118,7 +118,7 @@ class CleanupUsersUpdatedAtRename < ActiveRecord::Migration[4.2] end def down - rename_column_concurrently :users, :updated_at_timestamp, :updated_at + undo_cleanup_concurrent_column_rename :users, :updated_at, :updated_at_timestamp end end ``` diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 57a413f8e04..5a42952796c 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -459,29 +459,19 @@ module Gitlab check_trigger_permissions!(table) - old_col = column_for(table, old) - new_type = type || old_col.type - - add_column(table, new, new_type, - limit: old_col.limit, - precision: old_col.precision, - scale: old_col.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, new, old_col.default) unless old_col.default.nil? + create_column_from(table, old, new, type: type) install_rename_triggers(table, old, new) - - update_column_in_batches(table, new, Arel::Table.new(table)[old]) - - change_column_null(table, new, false) unless old_col.null - - copy_indexes(table, old, new) - copy_foreign_keys(table, old, new) end + # Reverses operations performed by rename_column_concurrently. + # + # This method takes care of removing previously installed triggers as well + # as removing the new column. + # + # table - The name of the database table. + # old - The name of the old column. + # new - The name of the new column. def undo_rename_column_concurrently(table, old, new) trigger_name = rename_trigger_name(table, old, new) @@ -557,6 +547,18 @@ module Gitlab remove_column(table, old) end + # Reverses the operations performed by cleanup_concurrent_column_rename. + # + # This method adds back the old_column removed + # by cleanup_concurrent_column_rename. + # It also adds back the (old_column > new_column) trigger that is removed + # by cleanup_concurrent_column_rename. + # + # table - The name of the database table containing the column. + # old - The old column name. + # new - The new column name. + # type - The type of the old column. If no type is given the new column's + # type is used. 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' @@ -564,26 +566,9 @@ module Gitlab 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? + create_column_from(table, new, old, type: type) 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. @@ -1076,6 +1061,28 @@ into similar problems in the future (e.g. when new tables are created). private + def create_column_from(table, old, new, type: nil) + old_col = column_for(table, old) + new_type = type || old_col.type + + add_column(table, new, new_type, + limit: old_col.limit, + precision: old_col.precision, + scale: old_col.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, new, old_col.default) unless old_col.default.nil? + + update_column_in_batches(table, new, Arel::Table.new(table)[old]) + + change_column_null(table, new, false) unless old_col.null + + copy_indexes(table, old, new) + copy_foreign_keys(table, old, new) + end + def validate_timestamp_column_name!(column_name) return if PERMITTED_TIMESTAMP_COLUMNS.member?(column_name) -- cgit v1.2.3