Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add migration helper methods [skip ci] #102

Closed
wants to merge 1 commit into from

Conversation

fatkodima
Copy link
Contributor

Related #95

Currently, I'm not asking for a thorough review, just POC validation.

This adds (documentation and usage examples provided inline with method definitions):

  1. add_foreign_key_concurrently. If we can update activerecord dependency to >= 5.2, then this won't be needed as add_foreign_key ..., validate: false can be used directly
  2. update_column_in_batches
  3. add_column_with_default
  4. rename_column_concurrently, undo_rename_column_concurrently, cleanup_after_rename_column_concurrently, undo_cleanup_after_rename_column_concurrently - for working with renaming columns
  5. change_column_type_concurrently, cleanup_after_change_column_type_concurrently

While writing this, I have figured it out, that it will also be useful to add something like rename_table_concurrently. If you have other ideas of what can be useful to add, please share.

Gitlab also has some code for background migrations for extra large tables. I do not considered them here to make this PR more digestible. Those will be added separately, if needed.

Todo:

  • support mysql
  • add tests
  • update documentation

@ankane
Copy link
Owner

ankane commented Dec 18, 2019

Hey @fatkodima, thanks for another PR! Overall, I think migration helpers makes sense. A few notes:

  1. Let's add them one at a time (or in logical groups) so we can discuss them individually
  2. We shouldn't copy any code from GitLab

@fatkodima
Copy link
Contributor Author

@ankane
In point 2, you mean due to licensing?
And what about mentioned changing activerecord requirement to >= 5.2?

@ankane
Copy link
Owner

ankane commented Dec 18, 2019

Yeah, due to licensing (although it's also MIT, so we could use it), but mostly I'd rather recreate them from first principles in their simplest form to start.

Re activerecord: we shouldn't change the version requirements, but we can have some helpers only work with AR 5.2+ (and raise an error message for earlier versions) if it's significantly more work to make them work with earlier versions.

@ankane
Copy link
Owner

ankane commented Jan 5, 2020

Closing in favor of separate PRs. @fatkodima, would love to get your thoughts on #111 when you have a chance.

@ankane ankane closed this Jan 5, 2020
@ankane
Copy link
Owner

ankane commented Jan 5, 2020

Also, fwiw, I made the helpers opt-in on master to demonstrate what that would look like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants