Skip to content

Commit

Permalink
Introduce raw methods for direct dispatch
Browse files Browse the repository at this point in the history
  • Loading branch information
rkrage committed May 31, 2024
1 parent 3eab753 commit 61ecf84
Show file tree
Hide file tree
Showing 6 changed files with 318 additions and 82 deletions.
15 changes: 8 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,18 @@ There are two major classes of concerns we try to handle in the API:
We rename migration methods with prefixes denoting their safety level:

- `safe_*`: These methods check for both application and database safety concerns, prefer concurrent operations where available, set low lock timeouts where appropriate, and decompose operations into multiple safe steps.
- `unsafe_*`: These methods are generally a direct dispatch to the native ActiveRecord migration method.
- `unsafe_*`: These methods are generally a direct dispatch to the native ActiveRecord migration method, but will sometimes include basic safety features.
- `raw_*`: These methods are a direct dispatch to the native ActiveRecord migration method

Calling the original migration methods without a prefix will raise an error.

The API is designed to be explicit yet remain flexible. There may be situations where invoking the `unsafe_*` method is preferred (or the only option available for definitionally unsafe operations).

While `unsafe_*` methods were historically (through 1.0) pure wrappers for invoking the native ActiveRecord migration method, there is a class of problems that we can't handle easily without breaking that design rule a bit. For example, dropping a column is unsafe from an application perspective, so we make the application safety concerns explicit by using an `unsafe_` prefix. Using `unsafe_remove_column` calls out the need to audit the application to confirm the migration won't break the application. Because there are no safe alternatives we don't define a `safe_remove_column` analogue. However there are still conditions we'd like to assert before dropping a column. For example, dropping an unused column that's used in one or more indexes may be safe from an application perspective, but the cascading drop of the index won't use a `CONCURRENT` operation to drop the dependent indexes and is therefore unsafe from a database perspective.

When `unsafe_*` migration methods support checks of this type you can bypass the checks by passing an `:allow_dependent_objects` key in the method's `options` hash containing an array of dependent object types you'd like to allow. Until 2.0 none of these checks will run by default, but you can opt-in by setting `config.check_for_dependent_objects = true` [in your configuration initializer](#configuration).
When `unsafe_*` migration methods support checks of this type you can bypass the checks by passing an `:allow_dependent_objects` key in the method's `options` hash containing an array of dependent object types you'd like to allow. These checks will run by default, but you can opt-out by setting `config.check_for_dependent_objects = false` [in your configuration initializer](#configuration).

Similarly we believe the `force: true` option to ActiveRecord's `create_table` method is always unsafe, and therefore we disallow it even when calling `unsafe_create_table`. This option won't be enabled by default until 2.0, but you can opt-in by setting `config.allow_force_create_table = false` [in your configuration initializer](#configuration).
Similarly we believe the `force: true` option to ActiveRecord's `create_table` method is always unsafe, and therefore we disallow it even when calling `unsafe_create_table`. This option is enabled by default, but you can opt-out by setting `config.allow_force_create_table = true` [in your configuration initializer](#configuration).

[Running multiple DDL statements inside a transaction acquires exclusive locks on all of the modified objects](https://medium.com/paypal-tech/postgresql-at-scale-database-schema-changes-without-downtime-20d3749ed680#cc22). For that reason, this gem [disables DDL transactions](./lib/pg_ha_migrations/hacks/disable_ddl_transaction.rb) by default. You can change this by resetting `ActiveRecord::Migration.disable_ddl_transaction` in your application.

Expand Down Expand Up @@ -153,7 +154,7 @@ safe_change_column_default :table, :column, -> { "NOW()" }
safe_change_column_default :table, :column, -> { "'NOW()'" }
```

Note: On Postgres 11+ adding a column with a constant default value does not rewrite or scan the table (under a lock or otherwise). In that case a migration adding a column with a default should do so in a single operation rather than the two-step `safe_add_column` followed by `safe_change_column_default`. We enforce this best practice with the error `PgHaMigrations::BestPracticeError`, but if your prefer otherwise (or are running in a mixed Postgres version environment), you may opt out by setting `config.prefer_single_step_column_addition_with_default = true` [in your configuration initializer](#configuration).
Note: On Postgres 11+ adding a column with a constant default value does not rewrite or scan the table (under a lock or otherwise). In that case a migration adding a column with a default should do so in a single operation rather than the two-step `safe_add_column` followed by `safe_change_column_default`. We enforce this best practice with the error `PgHaMigrations::BestPracticeError`, but if your prefer otherwise (or are running in a mixed Postgres version environment), you may opt out by setting `config.prefer_single_step_column_addition_with_default = false` [in your configuration initializer](#configuration).

#### safe\_make\_column\_nullable

Expand Down Expand Up @@ -513,9 +514,9 @@ end
#### Available options

- `disable_default_migration_methods`: If true, the default implementations of DDL changes in `ActiveRecord::Migration` and the PostgreSQL adapter will be overridden by implementations that raise a `PgHaMigrations::UnsafeMigrationError`. Default: `true`
- `check_for_dependent_objects`: If true, some `unsafe_*` migration methods will raise a `PgHaMigrations::UnsafeMigrationError` if any dependent objects exist. Default: `false`
- `prefer_single_step_column_addition_with_default`: If true, raise an error when adding a column and separately setting a constant default value for that column in the same migration. Default: `false`
- `allow_force_create_table`: If false, the `force: true` option to ActiveRecord's `create_table` method is disallowed. Default: `true`
- `check_for_dependent_objects`: If true, some `unsafe_*` migration methods will raise a `PgHaMigrations::UnsafeMigrationError` if any dependent objects exist. Default: `true`
- `prefer_single_step_column_addition_with_default`: If true, raise an error when adding a column and separately setting a constant default value for that column in the same migration. Default: `true`
- `allow_force_create_table`: If false, the `force: true` option to ActiveRecord's `create_table` method is disallowed. Default: `false`
- `infer_primary_key_on_partitioned_tables`: If true, the primary key for partitioned tables will be inferred on PostgreSQL 11+ databases (identifier column + partition key columns). Default: `true`

### Rake Tasks
Expand Down
2 changes: 1 addition & 1 deletion lib/pg_ha_migrations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ module PgHaMigrations
def self.config
@config ||= Config.new(
true,
false,
true,
false,
true,
true
)
end
Expand Down
75 changes: 50 additions & 25 deletions lib/pg_ha_migrations/unsafe_statements.rb
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
module PgHaMigrations::UnsafeStatements
def self.disable_or_delegate_default_method(method_name, error_message, allow_reentry_from_compatibility_module: false)
define_method(method_name) do |*args, &block|
if PgHaMigrations.config.check_for_dependent_objects
disallow_migration_method_if_dependent_objects!(method_name, arguments: args)
end

if PgHaMigrations.config.disable_default_migration_methods
# Most migration methods are only ever called by a migration and
# therefore aren't re-entrant or callable from another migration
# method, but `execute` is called directly by at least one of the
# implementations in `ActiveRecord::Migration::Compatibility` so
# we have to explicitly handle that case by allowing execution of
# the original implementation by its original name.
unless allow_reentry_from_compatibility_module && caller[0] =~ /lib\/active_record\/migration\/compatibility.rb/
unless allow_reentry_from_compatibility_module && caller[0] =~ /lib\/active_record\/migration\/compatibility.rb/
raise PgHaMigrations::UnsafeMigrationError, error_message
end
end
Expand All @@ -33,38 +29,67 @@ def self.delegate_unsafe_method_to_migration_base_class(method_name)
ruby2_keywords "unsafe_#{method_name}"
end

def self.delegate_raw_method_to_migration_base_class(method_name)
define_method("raw_#{method_name}") do |*args, &block|
execute_ancestor_statement(method_name, *args, &block)
end
ruby2_keywords "raw_#{method_name}"
end

# Direct dispatch to underlying Rails method as unsafe_<method_name> with dependent object check
delegate_unsafe_method_to_migration_base_class :add_check_constraint
delegate_unsafe_method_to_migration_base_class :add_column
delegate_unsafe_method_to_migration_base_class :change_table
delegate_unsafe_method_to_migration_base_class :drop_table
delegate_unsafe_method_to_migration_base_class :rename_table
delegate_unsafe_method_to_migration_base_class :rename_column
delegate_unsafe_method_to_migration_base_class :add_foreign_key
delegate_unsafe_method_to_migration_base_class :change_column
delegate_unsafe_method_to_migration_base_class :change_column_default
delegate_unsafe_method_to_migration_base_class :remove_column
delegate_unsafe_method_to_migration_base_class :change_table
delegate_unsafe_method_to_migration_base_class :drop_table
delegate_unsafe_method_to_migration_base_class :execute
delegate_unsafe_method_to_migration_base_class :remove_index
delegate_unsafe_method_to_migration_base_class :add_foreign_key
delegate_unsafe_method_to_migration_base_class :remove_foreign_key
delegate_unsafe_method_to_migration_base_class :add_check_constraint
delegate_unsafe_method_to_migration_base_class :remove_check_constraint
delegate_unsafe_method_to_migration_base_class :remove_column
delegate_unsafe_method_to_migration_base_class :remove_foreign_key
delegate_unsafe_method_to_migration_base_class :remove_index
delegate_unsafe_method_to_migration_base_class :rename_column
delegate_unsafe_method_to_migration_base_class :rename_table

disable_or_delegate_default_method :create_table, ":create_table is NOT SAFE! Use safe_create_table instead"
# Direct dispatch to underlying Rails method as raw_<method_name> without dependent object check
delegate_raw_method_to_migration_base_class :add_check_constraint
delegate_raw_method_to_migration_base_class :add_column
delegate_raw_method_to_migration_base_class :add_foreign_key
delegate_raw_method_to_migration_base_class :add_index
delegate_raw_method_to_migration_base_class :change_column
delegate_raw_method_to_migration_base_class :change_column_default
delegate_raw_method_to_migration_base_class :change_column_null
delegate_raw_method_to_migration_base_class :change_table
delegate_raw_method_to_migration_base_class :create_table
delegate_raw_method_to_migration_base_class :drop_table
delegate_raw_method_to_migration_base_class :execute
delegate_raw_method_to_migration_base_class :remove_check_constraint
delegate_raw_method_to_migration_base_class :remove_column
delegate_raw_method_to_migration_base_class :remove_foreign_key
delegate_raw_method_to_migration_base_class :remove_index
delegate_raw_method_to_migration_base_class :rename_column
delegate_raw_method_to_migration_base_class :rename_table

# Raises error if disable_default_migration_methods is true
# Otherwise, direct dispatch to underlying Rails method without dependent object check
disable_or_delegate_default_method :add_check_constraint, ":add_check_constraint is NOT SAFE! Use :safe_add_unvalidated_check_constraint and then :safe_validate_check_constraint instead"
disable_or_delegate_default_method :add_column, ":add_column is NOT SAFE! Use safe_add_column instead"
disable_or_delegate_default_method :change_table, ":change_table is NOT SAFE! Use a combination of safe and explicit unsafe migration methods instead"
disable_or_delegate_default_method :drop_table, ":drop_table is NOT SAFE! Explicitly call :unsafe_drop_table to proceed"
disable_or_delegate_default_method :rename_table, ":rename_table is NOT SAFE! Explicitly call :unsafe_rename_table to proceed"
disable_or_delegate_default_method :rename_column, ":rename_column is NOT SAFE! Explicitly call :unsafe_rename_column to proceed"
disable_or_delegate_default_method :add_foreign_key, ":add_foreign_key is NOT SAFE! Explicitly call :unsafe_add_foreign_key"
disable_or_delegate_default_method :add_index, ":add_index is NOT SAFE! Use safe_add_concurrent_index instead"
disable_or_delegate_default_method :change_column, ":change_column is NOT SAFE! Use a combination of safe and explicit unsafe migration methods instead"
disable_or_delegate_default_method :change_column_default, ":change_column_default is NOT SAFE! Use safe_change_column_default instead"
disable_or_delegate_default_method :change_column_null, ":change_column_null is NOT (guaranteed to be) SAFE! Either use :safe_make_column_nullable or explicitly call :unsafe_make_column_not_nullable to proceed"
disable_or_delegate_default_method :remove_column, ":remove_column is NOT SAFE! Explicitly call :unsafe_remove_column to proceed"
disable_or_delegate_default_method :add_index, ":add_index is NOT SAFE! Use safe_add_concurrent_index instead"
disable_or_delegate_default_method :change_table, ":change_table is NOT SAFE! Use a combination of safe and explicit unsafe migration methods instead"
disable_or_delegate_default_method :create_table, ":create_table is NOT SAFE! Use safe_create_table instead"
disable_or_delegate_default_method :drop_table, ":drop_table is NOT SAFE! Explicitly call :unsafe_drop_table to proceed"
disable_or_delegate_default_method :execute, ":execute is NOT SAFE! Explicitly call :unsafe_execute to proceed", allow_reentry_from_compatibility_module: true
disable_or_delegate_default_method :remove_index, ":remove_index is NOT SAFE! Use safe_remove_concurrent_index instead for Postgres 9.6 databases; Explicitly call :unsafe_remove_index to proceed on Postgres 9.1"
disable_or_delegate_default_method :add_foreign_key, ":add_foreign_key is NOT SAFE! Explicitly call :unsafe_add_foreign_key"
disable_or_delegate_default_method :remove_foreign_key, ":remove_foreign_key is NOT SAFE! Explicitly call :unsafe_remove_foreign_key"
disable_or_delegate_default_method :add_check_constraint, ":add_check_constraint is NOT SAFE! Use :safe_add_unvalidated_check_constraint and then :safe_validate_check_constraint instead"
disable_or_delegate_default_method :remove_check_constraint, ":remove_check_constraint is NOT SAFE! Explicitly call :unsafe_remove_check_constraint to proceed"
disable_or_delegate_default_method :remove_column, ":remove_column is NOT SAFE! Explicitly call :unsafe_remove_column to proceed"
disable_or_delegate_default_method :remove_foreign_key, ":remove_foreign_key is NOT SAFE! Explicitly call :unsafe_remove_foreign_key"
disable_or_delegate_default_method :remove_index, ":remove_index is NOT SAFE! Use safe_remove_concurrent_index instead for Postgres 9.6 databases; Explicitly call :unsafe_remove_index to proceed on Postgres 9.1"
disable_or_delegate_default_method :rename_column, ":rename_column is NOT SAFE! Explicitly call :unsafe_rename_column to proceed"
disable_or_delegate_default_method :rename_table, ":rename_table is NOT SAFE! Explicitly call :unsafe_rename_table to proceed"

def unsafe_create_table(table, options={}, &block)
if options[:force] && !PgHaMigrations.config.allow_force_create_table
Expand Down
Loading

0 comments on commit 61ecf84

Please sign in to comment.