Add add_column_safely and backfill_column_safely migration helpers by fatkodima · Pull Request #105 · ankane/strong_migrations

fatkodima · 2019-12-21T01:54:00Z

No description provided.

jturkel · 2019-12-31T14:19:11Z

lib/strong_migrations/migration_helpers.rb

+      start_pk = result.first[primary_key]
+
+      loop do
+        finish_arel = table


Does this also need to add .where(table[column_name].not_eq(value)) to avoid reprocessing rows during a retry? Same question for the actual update a few lines down.

I don't think so. This method updates rows in primary key order, so when restarting, the whole process will start from first (start_pk) unmatched row, which was last when update crashes, and move forward.

That's true that the process will pickup where it left off before the restart but won't it unnecessarily update rows that were created after the column default was set but before the backfill process completes (i.e. rows were the DB filled in the column default on insert)? Might not be much of an issue in practice though.

Might not be much of an issue in practice though.

Actually, this.

ankane

Hey @fatkodima, thanks for this and the other PRs that follow it. Will hold off on reviewing dependent ones until we get this merged.

Overall, looks great! I still need to dig into the Arel. I was hoping there'd to be a good way to dynamically create a model and use the existing code from the readme, but I'm not sure there's a good way to do this while reusing the existing connection.

ankane · 2020-01-04T21:46:02Z

lib/strong_migrations/util.rb

+module StrongMigrations
+  module Util
+    def connection
+      ActiveRecord::Base.connection


Needs to use the migration connection rather than the ActiveRecord::Base connection.

ankane · 2020-01-04T21:49:15Z

test/migration_helpers_test.rb


+class AddColumnSafely < TestMigration
+  def change
+    add_column_safely :users, :nice, :boolean, default: true, null: false


Let's use a different name for the new column to avoid needing to change the AddColumnDefaultSafe migration (keeping the changeset minimal)

ankane · 2020-01-04T21:50:41Z

lib/strong_migrations/migration_helpers.rb

+      end
+    end
+
+    def backfill_column_safely(table_name, column_name, value, batch_size: 1000)


Let's remove batch size to keep things simple. Also, default should probably be 10,000 like with existing instructions.

ankane · 2020-01-04T21:54:46Z

lib/strong_migrations/migration_helpers.rb

    end

+    def add_column_safely(table_name, column_name, type, **options)
+      ensure_postgresql(__method__)


Think we can run the default add_column method when it's not Postgres.

ankane · 2020-01-04T21:55:26Z

lib/strong_migrations/migration_helpers.rb

+        reversible do |dir|
+          dir.up do
+            transaction do
+              add_column(table_name, column_name, type, default: nil, **options) unless connection.column_exists?(table_name, column_name, type)


Don't think we should check for column_exists?, as the migration should fail if a user tries to add an existing column.

ankane · 2020-01-04T21:56:29Z

lib/strong_migrations/migration_helpers.rb

+              change_column_default(table_name, column_name, default)
+            end
+
+            default_after_type_cast = connection.type_cast(default)


What's the reason for typecasting?

ankane · 2020-01-04T22:00:02Z

lib/strong_migrations/migration_helpers.rb

+
+      start_arel = table
+        .project(table[primary_key])
+        .where(table[column_name].not_eq(value))


Backfilling should only affect NULL columns. Would be good to include a test case for this situation (can do after initial PR if that's easier)

ankane · 2020-01-04T22:07:56Z

lib/strong_migrations.rb

    add_column_default:
 "Adding a column with a non-null default causes the entire table to be rewritten.
-Instead, add the column without a default value, then change the default.
+Instead, add the column without a default value, backfill and then change the default.


The default gets changed before backfilling.

ankane · 2020-01-04T22:12:02Z

lib/strong_migrations/migration_helpers.rb

+        end
+
+        safety_assured { execute(update_arel.to_sql) }
+


Need to throttle - sleep(0.01) (may need a global option for this later since more powerful databases may need less throttling, but let's deal with that later)

fatkodima · 2020-01-05T00:00:52Z

@ankane Thanks for review! Updated with your suggestions.

fatkodima · 2020-01-05T13:45:42Z

Updated with master.

fatkodima · 2020-02-20T22:27:42Z

@ankane would you like making another round of reviews on this?

ankane · 2020-03-02T04:27:15Z

Hey @fatkodima, sorry for the delay. Still thinking on #111.

ankane · 2020-05-12T09:12:48Z

Decided helper methods are better as a separate gem for now - see #111

fatkodima force-pushed the add_column_with_default_safely branch 2 times, most recently from e50e382 to 8bb6bd3 Compare December 22, 2019 11:55

fatkodima changed the title ~~Add add_column_with_default_safely and update_column_in_batches migration helpers~~ Add add_column_safely and backfill_column_safely migration helpers Dec 24, 2019

fatkodima mentioned this pull request Dec 27, 2019

Add rename_column_safely and rename_column_safely_cleanup migration helpers #106

Closed

1 task

fatkodima force-pushed the add_column_with_default_safely branch from 8bb6bd3 to 724bafa Compare December 30, 2019 23:45

fatkodima mentioned this pull request Dec 30, 2019

Add change_column_safely migration helper #107

Closed

jturkel reviewed Dec 31, 2019

View reviewed changes

ankane requested changes Jan 4, 2020

View reviewed changes

fatkodima force-pushed the add_column_with_default_safely branch from 724bafa to d41728c Compare January 4, 2020 23:58

fatkodima force-pushed the add_column_with_default_safely branch 2 times, most recently from 0cb518e to 63da732 Compare January 5, 2020 13:42

Add add_column_safely and backfill_column_safely migration helpers

bc50b5a

fatkodima force-pushed the add_column_with_default_safely branch from 63da732 to bc50b5a Compare February 16, 2020 11:52

ankane added the waiting label May 12, 2020

ankane closed this May 12, 2020

Conversation

fatkodima commented Dec 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankane Jan 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatkodima commented Jan 5, 2020

Uh oh!

fatkodima commented Jan 5, 2020

Uh oh!

fatkodima commented Feb 20, 2020

Uh oh!

ankane commented Mar 2, 2020

Uh oh!

ankane commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ankane Jan 4, 2020 •

edited

Loading