feat(store): improve SQL migrations - #2358
Conversation
igamigo
changed the title
There was a problem hiding this comment.
Looks good so far. One thing we should do before closing the issue is this point from the issue:
- Back up the database before migration.
We should look into backing up the store, migrating it, and once we know it has passed (by checking the hashes), delete the backup. If anything errors in the middle, we roll back the upgrade and then report the error. We could decide to do this in a separate PR.
Another thing that would be good to validate is whether the first migration is applied on a fresh file and that there is no way for a sqlite database to be applied the 0001 migration
| /// Renders a migration failure without reproducing the migration script. | ||
| pub fn describe_migration_error(err: &MigrationError) -> String { | ||
| match err { | ||
| MigrationError::RusqliteError { err, .. } => describe_sqlite_error(err), | ||
| MigrationError::ForeignKeyCheck(violations) => { | ||
| format!("{} foreign key violation(s) after applying the migration", violations.len()) | ||
| }, | ||
| other => other.to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Let's just inline this function where it's called (it's only one place AFAICT)
| /// Rewrites the SQL text stored for a schema object into a form that ignores differences `SQLite` | ||
| /// itself ignores, so cosmetic edits do not change the fingerprint. | ||
| fn normalize_sql(sql: &str) -> String { |
There was a problem hiding this comment.
Let's revert these changes. I don't think it's code worth maintaining as it becomes instantly quite more complicated. A solution could be to introduce a SQL linter so the normalization is even more trivial.
There was a problem hiding this comment.
Reverted
There was a problem hiding this comment.
One thing I think we can do is verify that the hash and check that it's indeed as we expect
There was a problem hiding this comment.
We now follow node's procedure of checking migrations. At startup, the MigrationBuilder derives hashes by replaying the migrations, and checks them.
This is now handled as of 0e10af0 |
| exit 0 | ||
| fi | ||
|
|
||
| >&2 echo "The following released migrations were modified, renamed or deleted:" |
There was a problem hiding this comment.
If the diff is compared against the base branch then a migration the script is comparing to might not be released yet:
| >&2 echo "The following released migrations were modified, renamed or deleted:" | |
| >&2 echo "The following merged migrations were modified, renamed or deleted:" |
|
If I am correct, merging this PR would cause any future change in SQL store database to force the addition of a migration script. Do we want this once we merge it to |
Let's leave this as it is right now and skip the migration check job in another PR with a new label (analogous to what |
|
3 participants
Closes #2346
store.sqlasmigrations/0001_init.sql. Schema changes are appended as new files, no longer patched into an existing one.PINNED_SCHEMA_HASHESand made it the runtime check. The previous check is now test only.normalize_sqlas a quote-aware scanner. Inline column comments are now free to edit, and three cases where different schemas shared a digest no longer do.apply_migrationshandles everySchemaVersionarm. A store from a newer client reportsSchemaTooNewrather than an opaqueDatabaseTooFarAhead.scripts/check-migrations.shand a CI job that rejects any edit to a released migration.Notes
#2304 already moved the hash from the script's bytes to the materialized schema.