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

Create new migration to undo adding foreign key #35088

Merged
merged 1 commit into from Jun 2, 2020

Conversation

uponthesun
Copy link

Reverting #35051 again because the migration times out in production:

https://app.honeybadger.io/projects/3240/faults/64320902

Running the SQL statement on a clone takes about 4 minutes:

mysql> ALTER TABLE `sections` ADD CONSTRAINT `fk_rails_5c2401d1cb` FOREIGN KEY (`script_id`) REFERENCES `scripts` (`id`);
Query OK, 2739553 rows affected (4 min 34.15 sec)
Records: 2739553  Duplicates: 0  Warnings: 0

Inspiration for hack for not running in production comes from https://github.com/code-dot-org/code-dot-org/blob/staging/dashboard/db/migrate/20181106201126_change_id_to_bigint_in_user_levels.rb

I'm not sure what our current best practices are for migrations on tables of this size - large but not user_levels large. Prepping this revert to unblock the pipeline.

Testing story

  • Ran migration locally and verified results with show create table sections. Relevant output:
  PRIMARY KEY (`id`),
  UNIQUE KEY `index_sections_on_code` (`code`) USING BTREE,
  KEY `fk_rails_20b1e5de46` (`course_id`) USING BTREE,
  KEY `index_sections_on_user_id` (`user_id`) USING BTREE,
  CONSTRAINT `fk_rails_20b1e5de46` FOREIGN KEY (`course_id`) REFERENCES `courses` (`id`)

compared to staging:

  PRIMARY KEY (`id`),
  UNIQUE KEY `index_sections_on_code` (`code`) USING BTREE,
  KEY `fk_rails_20b1e5de46` (`course_id`) USING BTREE,
  KEY `index_sections_on_user_id` (`user_id`) USING BTREE,
  KEY `fk_rails_5c2401d1cb` (`script_id`),
  CONSTRAINT `fk_rails_20b1e5de46` FOREIGN KEY (`course_id`) REFERENCES `courses` (`id`),
  CONSTRAINT `fk_rails_5c2401d1cb` FOREIGN KEY (`script_id`) REFERENCES `scripts` (`id`)

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

@dmcavoy dmcavoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this revert @uponthesun !

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are our thoughts on removing the original migration, rather than adding a revert migration?

@uponthesun
Copy link
Author

@Hamms we did do that for the first one, I went this way because the change to add the FK has been applied to three environments (staging/test/lb) as well as some developer desktops at this point, so it'd be more manual work to undo it in each of those. Do you feel like it's better to do that way nonetheless?

@Hamms
Copy link
Contributor

Hamms commented Jun 2, 2020

Ahh, that makes sense. I'd suggest we consider squashing these migrations in a later pass, after we're confident the revert has been applied everywhere

@uponthesun
Copy link
Author

Going to merge to start unblocking deployments, but feel free to leave comments on whether this was the right approach, and whether we should clean up these migrations somehow after they've been applied @wjordan @sureshc

@uponthesun uponthesun merged commit 1af1174 into staging Jun 2, 2020
@uponthesun uponthesun deleted the revert-script-id-fk-migration branch June 2, 2020 18:55
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.

None yet

4 participants