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

Rewrite DB update to be explicit #6609

Merged
merged 1 commit into from
Mar 21, 2016
Merged

Conversation

ErisDS
Copy link
Member

@ErisDS ErisDS commented Mar 17, 2016

This is a pretty important functional change. The work is complete, however it is marked as WIP as I have so far only tested the changes on a sqlite3 database.

I would very much like some help with testing this change, particularly on postgres, but also anyone out there who has a modified DB or a large dataset - your testing would be awesome.

What to test

  1. trying to upgrade a database that is older than data version 003 (from ghost < 0.5.0). This should now always output an error message instructing users to upgrade to 0.7.1 first.
  2. trying to upgrade a database that is on version 003 (0.5.0... 0.6.4?)
  3. trying to upgrade a database that was updated to 004, using FORCE_MIGRATION=true
  4. trying to upgrade a database that was created AT 004, using FORCE_MIGRATION=true
  5. trying to upgrade a half-upgraded database - e.g. the columns that need to be created already exist
  6. trying to create new databases (just in case)

I've already tested these things on sqlite3, and it all seems to be working ok.

Why this is changing

A little while ago, I did a PR to add the ability to drop columns in our database migrations/upgrades: #6467. This didn't work as expected, as on migrations where a table was deleted, we'd build commands to delete the table, followed by deleting all the (already deleted) columns. This prevented upgrades from 002 -> 003 from working, but they are sufficiently old (0.5.0 was released 2014-08-11) that it was safe to just remove these old migrations.

However, this got me thinking about how, although migrations have always been destructive in that they would delete any table not defined in schema.js, we'd never previously had destructive migrations for columns. This would adversely affect any users who had, for example, added an extra column to their posts tables.

Further still, because all the migration operations were automatically calculated by figuring out the differences between the existing DB structure, and what was in the schema file, there were absolutely no guarantees what might happen when a user ran a migration.

This makes it very hard to reason about upgrades, and makes them almost impossible to test - the test coverage for migrations has always been absolute zero.

All of these reasons added up together resulted in me deciding to remove the old automatic migrations in favour of explicit tasks which describe what needs to happen. These new, explicit tasks have 100% test coverage. The downside is that they're a bit harder to write, and we're now responsible for ensuring that the populate & upgrade flow result in the same DB structure, however I believe this is still an improvement particularly as we are able to test much more thoroughly now.


refs #6301

  • Replace builder & automated database upgrade with a set of explicit tasks
  • Ensure the tasks can only happen if they need to
  • Remove some duplicate code between fixture & db upgrades (more to do)
  • Add tests

refs TryGhost#6301

- Replace builder & automated database upgrade with a set of explicit tasks
- Ensure the tasks can only happen if they need to
- Remove some duplicate code between fixture & db upgrades (more to do)
- Add tests
@ErisDS
Copy link
Member Author

ErisDS commented Mar 21, 2016

I've tested this on mysql & postgres today. Have been trying out doing updates from 003, force migration updates on 004 and database creation at 004. Everything seems to be working smoothly.

The only thing I've found is a couple of messaging improvements I'd like to make across the whole db & fixture migration system, and a few things where I'd like to change behaviour slightly. IMO these things can go in a second PR as they update/change what's already there and working.

@ErisDS ErisDS changed the title [WIP] Rewrite DB update to be explicit Rewrite DB update to be explicit Mar 21, 2016
sebgie added a commit that referenced this pull request Mar 21, 2016
Rewrite DB update to be explicit
@sebgie sebgie merged commit ea9c823 into TryGhost:master Mar 21, 2016
@sebgie sebgie deleted the new-db-upgrade branch March 21, 2016 15:15
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