fix: ensure migration progress is not lost for PG, mysql and sqlite #1991
Conversation
crepererum
changed the title
crepererum
force-pushed
the
crepererum/issue1966
branch
3 times, most recently
from
a2613f1 to
abb3d4d
Compare
|
ready to review |
| // execute migrations twice. See https://github.com/launchbadge/sqlx/issues/1966. | ||
| // The `execution_time` however can only be measured for the whole transaction. This value _only_ exists for | ||
| // data lineage and debugging reasons, so it is not super important if it is lost. So we initialize it to -1 | ||
| // and update it once the actual transaction completed. |
There was a problem hiding this comment.
MySQL is a problem case because any DDL statements like CREATE TABLE implicitly commit any open transaction: https://dev.mysql.com/doc/refman/5.6/en/implicit-commit.html
https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html
There's no real way we can actually get any isolation with MySQL. All the user can really do is either manually reverse the changes or restore from a backup.
What we probably should be doing is inserting into _sqlx_migrations first with success = FALSE and then update it if we successfully apply the migration.
There was a problem hiding this comment.
ah, this is why this construct was there, I was wondering why the mysql code looked so much different. I'll apply your suggestion and add some doc comment explaining it (including the links you've provided).
There was a problem hiding this comment.
Applied. However I needed a similar trick for the reverse direction and there I first toggle the flag from true to false and then delete the entry (have a look at the code), let me know if that makes sense to you.
|
Will apply the recommendations next week. |
crepererum
force-pushed
the
crepererum/issue1966
branch
from
abb3d4d to
dc45ef7
Compare
This is similar to launchbadge#1966.
This is similar to launchbadge#1966.
crepererum
force-pushed
the
crepererum/issue1966
branch
from
dc45ef7 to
fd24e8f
Compare
|
@abonander any update on this? |
Fixes #1966.
The same issue existed for other database types, so I've fixed them as well. I couldn't find any runtime tests for migrations yet, so I've extended the migrations testing code to include some.