Feat/backward migrations #1072
Feat/backward migrations #1072
Conversation
patriotyk
force-pushed
the
feat/backward_migrations
branch
from
c9d0296 to
9dff1ce
Compare
patriotyk
force-pushed
the
feat/backward_migrations
branch
from
9dff1ce to
c49d330
Compare
Codecov Report
@@ Coverage Diff @@
## master #1072 +/- ##
==========================================
- Coverage 75.29% 75.28% -0.01%
==========================================
Files 157 157
Lines 9036 9065 +29
==========================================
+ Hits 6804 6825 +21
- Misses 2232 2240 +8
Continue to review full report at Codecov.
|
Signed-off-by: Serhiy Stetskovych <[email protected]>
patriotyk
force-pushed
the
feat/backward_migrations
branch
from
c49d330 to
178f588
Compare
Signed-off-by: Serhiy Stetskovych <[email protected]>
patriotyk
force-pushed
the
feat/backward_migrations
branch
from
178f588 to
8bb6858
Compare
| bool SQLStorageBase::dbMigrateForward(int version_from) { | ||
| SQLite3Guard db = dbConnection(); | ||
| for (int32_t k = version_from + 1; k <= current_schema_version_; k++) { | ||
| if (db.exec(schema_migrations_.at(static_cast<size_t>(k)), nullptr, nullptr) != SQLITE_OK) { |
There was a problem hiding this comment.
Thoughts:
- for safety, first insert the rollback migrations, then execute the forward migration. This way, there is no way to lose the backward migrations if the migration process was interrupted. Even cleaner would be to use sqlite save points (https://www.sqlite.org/lang_savepoint.html) to nest all this migration process in a big transaction. Not sure if we can still do that and keep compatibility. It might be fine though, need to think a bit more about it
- use
INSERT OR REPLACE, as we'll probably want to have the more recent versions of these
There was a problem hiding this comment.
I tried to swap, but it doesn't work. Doesn't work because first we need to migrate to create backward_migrations table, then we can insert data in to it. I also tried to add save point but PrepareStatement always fails when I add it.
There was a problem hiding this comment.
Good point for swapping, I missed that detail!
I took the liberty to push a version which should work with save points on top of your PR. Please check if it doesn't break your work.
| LOG_ERROR << "Can't migrate db from version " << (ver) << " to version " << ver - 1 << ": " << db.errmsg(); | ||
| return false; | ||
| } | ||
| if (db.exec(std::string("DELETE FROM migrations WHERE version_from=") + std::to_string(ver) + ";", nullptr, |
There was a problem hiding this comment.
Even though it shouldn't hurt, do you think it's actually needed?
There was a problem hiding this comment.
Why not? We don't need this anymore so this data is garbage.
There was a problem hiding this comment.
Ok fair enough, I won't fight on this.
|
Thanks for the update, this is looking good. Sorry for all the nitpick comments, but they should be easy fixes. |
Signed-off-by: Serhiy Stetskovych <[email protected]>
patriotyk
force-pushed
the
feat/backward_migrations
branch
from
6af330a to
bafaaaa
Compare
With save points inside a transaction Signed-off-by: Laurent Bonnans <[email protected]>
|
I've added a commit to use save points for forward migration. It changes our existing migrations scripts which could be controversial but I don't think it has a negative impact. |
Thank you, I was trying to accomplish this without migration scripts modification. |
|
Looks fine to me after a precursory glance, and after discussion with @lbonn, I think it's fine if both of you are happy. It sounds like Laurent wants to do a manual test first, though. |
| return false; | ||
| } | ||
| if (db.exec(std::string("DELETE FROM rollback_migrations WHERE version_from=") + std::to_string(ver) + ";", nullptr, | ||
| nullptr) != SQLITE_OK) { |
There was a problem hiding this comment.
DELETE probably won't hurt, but shouldn't be necessary if we add with INSERT OR REPLACE.
There was a problem hiding this comment.
Here is the original discussion: #1072 (comment). Now I don't feel too strongly about it.
There was a problem hiding this comment.
Sorry, missed that conversation. And I agree, I don't feel strongly, I just wanted to leave the thought.
|
Ok I've added the test I wanted in #1041 (hard to do it here in a convincing fashion) and it passes. So let's merge! |
No description provided.