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

Use transactions for migrations & fixtures #6574

Closed
ErisDS opened this issue Mar 2, 2016 · 6 comments
Closed

Use transactions for migrations & fixtures #6574

ErisDS opened this issue Mar 2, 2016 · 6 comments
Assignees
Labels
help wanted [triage] Ideal issues for contributors to help with server / core Issues relating to the server or core of Ghost

Comments

@ErisDS
Copy link
Member

ErisDS commented Mar 2, 2016

Following on from the work in #6572, I think it would be a great improvement if we switched to using transactions for migrations & fixtures.

Back in ye olde days, it wasn't possible to use transactions for schema operations (see [https://github.com//issues/586])), however knex had a lot of changes to transactions around 0.8.0 and knex migrations use transactions (see changelog), which make me believe it may be possible now, although not documented.

As much as possible, it would be a significant improvement to have data migrations and fixtures wrapped in a transaction. Any failure to complete a task should then result in the whole transaction failing, with a clear error message.

This would help to prevent blogs from ending up in weird half-way states.

@ErisDS ErisDS added data server / core Issues relating to the server or core of Ghost labels Mar 2, 2016
@ErisDS
Copy link
Member Author

ErisDS commented Mar 20, 2016

Here is how to do transactions for schema operations: knex/knex#148 (comment)

@ErisDS ErisDS added the help wanted [triage] Ideal issues for contributors to help with label Mar 20, 2016
@ErisDS
Copy link
Member Author

ErisDS commented Mar 21, 2016

Note: Implementing this needs to be done in a way that is sensitive to the FORCE_MIGRATION flag.

This flag is intended for use when having to force a migration to complete. The main use case for this is if we add a migration to master, and then add extra things to it. Anyone who uses Ghost after the migration is added but before the new stuff is added would be stuck halfway through the upgrade.

The basic idea is that FORCE_MIGRATION ensures that the bottom version number we run the migrations from is as low as possible (currently 003). We should then attempt to run all migrations, and any which can run should be successful. #6621 ensures that every single fixture migration does a check to ensure it needs to run.

The upshot is, if we add a transaction so that a failure would result in a rollback, we only do this for actual error situations.

E.g. If you try to add a column that already exists, that's should probably output a warning, but not fail and rollback. It's likely this particular migration already ran.

E.g. If you try to add a column to a table that doesn't exist, that should probably fail and rollback because something is horribly wring.

E.g. If you try to update sort_order on posts_tags and get values > 0, that should probably output a warning, but not fail and rollback. Again it looks like the migration already ran.

E.g. If you try to update sort_order on posts_tags but get an error because the sort_order column doesn't exist, that should probably fail and rollback cos something is irreparably wrong.

@ErisDS
Copy link
Member Author

ErisDS commented Jun 10, 2016

In Ghost master, we now have a single fixture migration which uses a transaction:

return models.Base.transaction(function (transaction) {

This works really, really nicely.

I'd love to see this abstracted out into a transaction that automatically lives around migrations. I think it makes sense to have nested transactions, one for each individual task and an outer wrapping transaction for the process, but that might be overkill.

I'm starting to think that we need some small of "migration runner" script a bit like a test runner, that could then let us do things like node core/server/data/migrations/runner.js --version=004. To force running a particular version, instead of the FORCE_MIGRATION env variable

cc @acburdine on this last part because I think this would be a great thing to cli-ify 😉

@kirrg001
Copy link
Contributor

@ErisDS @acburdine
I will build the first basic version of the migration runner.
I don't think we need to flag --version for the first version or?
If you start the migration runner as separate shell command, it will create a db connection, reads your current db version and updates everything in one transaction. Tell me if i miss a detail.

@ErisDS
Copy link
Member Author

ErisDS commented Jun 13, 2016

So, IMO the first, most important thing, is getting migrations to run in a transaction. That will solve 99% of the issues we currently have with migrations.

Second thing is adding a runner to make it possible to run it separately to Ghost - and for this, no we do not need the --version flag to start, that can come later.

Everything else sounds spot on.

@kirrg001 kirrg001 mentioned this issue Jun 16, 2016
6 tasks
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Jun 20, 2016
closes TryGhost#6972, TryGhost#6574

- run each database version as top level transaction
- run migrations in correct order
ErisDS pushed a commit that referenced this issue Jul 14, 2016
closes #6972, #6574

- run each database version as top level transaction
- run migrations in correct order
chris-brown pushed a commit to chris-brown/Ghost that referenced this issue Aug 14, 2016
closes TryGhost#6972, TryGhost#6574

- run each database version as top level transaction
- run migrations in correct order
@ErisDS
Copy link
Member Author

ErisDS commented Sep 19, 2016

This was closed by #7000 / 0866aea

@ErisDS ErisDS closed this as completed Sep 19, 2016
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Sep 29, 2016
refs TryGhost#6574, refs TryGhost#7432

- create transaction for creating tables
- if an error occurs or a container get's destroyed before population finishes, transaction is rolled back
ErisDS pushed a commit that referenced this issue Sep 30, 2016
* 🎨  run database population in transaction

refs #6574, refs #7432

- create transaction for creating tables
- if an error occurs or a container get's destroyed before population finishes, transaction is rolled back

* 🎨  simplify transaction creation and test
ErisDS pushed a commit that referenced this issue Sep 30, 2016
* 🎨  run database population in transaction

refs #6574, refs #7432

- create transaction for creating tables
- if an error occurs or a container get's destroyed before population finishes, transaction is rolled back

* 🎨  simplify transaction creation and test
yo1dog pushed a commit to yo1dog/Ghost that referenced this issue Oct 14, 2016
* 🎨  run database population in transaction

refs TryGhost#6574, refs TryGhost#7432

- create transaction for creating tables
- if an error occurs or a container get's destroyed before population finishes, transaction is rolled back

* 🎨  simplify transaction creation and test
mixonic pushed a commit to mixonic/Ghost that referenced this issue Oct 28, 2016
* 🎨  run database population in transaction

refs TryGhost#6574, refs TryGhost#7432

- create transaction for creating tables
- if an error occurs or a container get's destroyed before population finishes, transaction is rolled back

* 🎨  simplify transaction creation and test
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this issue Nov 20, 2016
closes TryGhost#6972, TryGhost#6574

- run each database version as top level transaction
- run migrations in correct order
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this issue Nov 20, 2016
* 🎨  run database population in transaction

refs TryGhost#6574, refs TryGhost#7432

- create transaction for creating tables
- if an error occurs or a container get's destroyed before population finishes, transaction is rolled back

* 🎨  simplify transaction creation and test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted [triage] Ideal issues for contributors to help with server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

2 participants