Rollback operation on Start failure #73
Conversation
Implement the **change column type** operation. A change type migration
looks like this:
```json
{
"name": "18_change_column_type",
"operations": [
{
"change_type": {
"table": "reviews",
"column": "rating",
"type": "integer",
"up": "CAST(rating AS integer)",
"down": "CAST(rating AS text)"
}
}
]
}
```
This migration changes the type of the `rating` column from `TEXT` to
`INTEGER`.
The implementation is very similar to the **set NOT NULL** operation
(#63):
* On `Start`:
* Create a new column having the new type
* Backfill the new column with values from the existing column,
converting the types using the `up` SQL.
* Create a trigger to populate the new column when values are written to
the old column, converting types with `up`.
* Create a trigger to populate the old column when values are written to
the new column, converting types with `down`.
* On `Complete`
* Remove triggers
* Drop the old column
* Rename the new column to the old column name.
* On `Rollback`
* Remove the new column and both triggers.
The migration can fail in at least 2 ways:
* The initial backfill of existing rows on `Start` fails due to the type
conversion not being possible on one or more rows. In the above example,
any existing rows with `rating` values not representable as an `INTEGER`
will cause a failure on `Start`.
* In this case, the failure is reported and the migration rolled back
(#73)
* During the rollout period, unconvertible values are written to the old
version schema. The `up` trigger will fail to convert the values and the
`INSERT`/`UPDATE` will fail.
* Some form of data quarantine needs to be implemented here, copying the
invalid rows elsewhere and blocking completion of the migration until
those rows are handled in some way).
The PR also adds example migrations to the `/examples` directory.
andrew-farries
force-pushed
the
rollback-on-start-failure
branch
from
3d3b69c to
2785290
Compare
You mean there are some operations for which you think we shouldn't roll back? This PR adds rollbacks for all Or do you mean that we should also rollback on failed |
ah, I didn't realize we rollback the whole migration, all good! Still, I think the way we handle reporting to the user can be improved. We should clearly state that there was an error with start first, then that we are rolling back. In case of error during roll back show the error too. It feels like rollback should happen from the |
The current behavior is almost as you describe. In the case of a start + rollback failure the error output is: In the case of just a start error (and a successful rollback): We could also add a message in this case (eg 'migration successfully rolled back'), but IMO it's not worth it; users will assume that a failed start will not leave anything behind in the database.
I think the behaviour belongs in the package rather than the CLI as this is functionality that all clients will need. |
Some migration operations create new objects in the database; new columns, triggers and functions. When such a migration fails to start (eg a change column type operation where some existing rows in the database can't be converted to the new type), all database objects created by Start should be cleaned up.
This PR calls
RollbackonStarterrors to ensure that this cleanup happens.