DB migration and thisAndFuture addition #45
Conversation
dcaliste
force-pushed
the
thisAndFuture
branch
from
51ab8b3
to
2b37ff9
Compare
There was a problem hiding this comment.
The second commit "An empty timezone is a no-op in SqliteFormat::loadTimezones()." should be SqliteStorage::Private::loadTimezones(), I suppose.
Good that we got back to discussing this. Indeed need to remember to add new columns to the end.
Commented on renaming the extra1, but could be also eventually get rid of the unused extras. In retrospect suppose the extra3 could have been renamed to thisAndFuture. But the renamings/drops will also require a version bump so maybe do that separately, independent change or together with some later adjustement.
So far looking good, I'll do some more test round
|
Created JB#59982 for fixing the column placement. Can use that for reference. |
…JB#59982 The INSERT and UPDATE sharing the same code in sqliteformat.cpp, and INSERT relying on column order, new column should be added at the end in the CREATE statement. Otherwise, there is a difference between migrated DBs where the new column is at the end and new DBs where the column has been added in the middle of other columns. Correct this issue for the new thisAndFuture column.
dcaliste
force-pushed
the
thisAndFuture
branch
from
2b37ff9
to
9b0f5db
Compare
An empty timezone is a no-op in SqliteFormat::Private::loadTimezones(). This allows to remove the constraint exception on error check in macro SL3_exec().
Commit changes immediately after migration to the next version and bump user_version as soon as possible to avoid the database being migrated but the pragma not being in sync with the DB status. Don't fail on migration error when adding a new column, assuming that the column may already exists.
Operations like addEvent(KCalendarCore::Event) requires a default notebook to exist. Change a bit the initialisation to create a default notebook if none exists, instead of doing it only when the DB is empty.
Indeed, corrected in the commit message.
Thanks, bug id added in the corresponding commit.
Indeed, but since you introduced a way to migrate the database, I was tempted to use it since it avoids to get things behind names like extra3 and make the commit and the code in general more clear. But finally it ended up in two separated commits. Not sure it is clearer ! |
|
Alright, let's have this. |
It's a follow-up of the discussion from #43 .
Trying to make the migration to version with thisAndFuture column, I found that the initial implementation of the thisAndFuture column was wrong. It created different table schema for migrated DB and new DB, thus the INSERT, the UPDATE and the SELECT were wrong, or let say working only in one case... This should be fixed with the first commit, creating the new column always at the end for migrated or new DBs.
Then, as discussed in #43, I try to make the migration steps as atomic as possible, making the write immediate and setting the pragma there, immediately after the DB changes. This is the third commit.
The second commit is a minor simplification when I noticed that the
SL3_execmacro was filtering out the constraint errors for the sole purpose of inserting at each opening an existing empty timezone.When I tested, I noticed that if you delete the default notebook, then the DB has no default notebook anymore (even if it still contains various other notebooks) and this breaks a lot of assertions in the tests. I think it's not a big deal to always create a default notebook on opening. On device, there is already a default notebook (the "private" one), so it should not create additional notebooks.
I tested these changes with an old user_version = 0 DB and with a DB created on the fly by the tests. @pvuorela tell me what do you think about these changes.