do not change db schema in an incompatible way#5254
Conversation
|
Where this column is used? Does this only happen if you import a new backup into old Delta Chat core and then it tries to use the column? |
|
I don't think changing existing migration is a good idea. If needed we can add a migration restoring the column on top of this, but otherwise it will break backups of anyone who imported them into nightly. Also I run the latest core and @hpk42 probably runs the latest core on desktop, if we change existing migration this means database should be manually modified by anyone running core this way. |
But in this case it should be modified only if you are going to import the db to the older version, no? Which you are probably not going to do. We just should keep in mind that there are some db-s with these columns absent. |
|
Also some databases will have old_keypairs table and some will not?
These changes have been released in core 1.133.0 and there are bots running with this core. |
|
I already released a DeltaLab version yesterday with core from yesterday, just overriding the migration will cause issues I am afraid? |
+1000 |
it is used in cores v1.131.9 and below, so on virtually all released versions, that will break eg. when they get a second-device-database or backup from the upcoming version. iirc, in comparable previous column-removals, we removed only columns that are not in use. so, eg. using add-second-device from and andorid-1.44 to an desktop-1.42 (or the other way round) won't work. same for backups or just downgrading as 1.44 has issues that will be fixed in 1.44.1 or so. for changing the existing migration: i was not aware that it is that widely in use, i thought more about some devs, i am also running current cores on many devices :) bots usually do not do add-second-device or downgrades. the only difference to the changed-107 would be that these devs/bots already have the three columns removed. in a later migration, we could remove the columns for all. however, i am also fine to close/change this pr and go for another migration, just thought it is not worth the effort (and potentially added bugs :) (/me afk for now, so if someone wants to take over, feel free to commit/close, otherwise i can have a look tomorrow) |
|
I know of at least one user who is running core v1.134.0 on desktop and the most recent DeltaLab. They wanted to add an account to their Ubuntu Touch device, and got the error in the screenshot above. They thought their Ubuntu Touch setup was broken and wiped their entire DeltaTouch config on Ubuntu Touch. All of their accounts are completely on the v1.1.34.0 scheme by now, older backups may not be available. |
|
We also cannot just restore I would suggest that we solve this by adding a new migration that adds empty |
PR #5099 removed some columns in the database that were actually in use. usually, to not worsen UX unnecessarily (releases take time - in between, "Add Second Device", "Backup" etc. would fail), we try to avoid such schema changes, see discussion at #2294. there as still some situations imaginable, where things cause problems, however, the vast majority of real-life cases should be fine. as no official releases with newer cores are rolled out yet, i think, it is fine to change the "107" migration and not copy things a second time in a newer migration.
r10s
force-pushed
the
r10s/keep-compatibility-with-previous-versions
branch
from
970db1b to
8088333
Compare
…ts, deltalab and deltatouch
|
thinking it over, the sometimes missing columns would cause troubles also when keypairs are added on these systems. so, i added a commit that re-adds the columns in a new migration, filling it the same way as it was used for migration 107 - maybe i've overseen sth, but to me it looks as if housekeeping is not needed. also did some little tests using the when this is merged, we should do a new core so a 1.43.1 can be done for android/ios-testflight |
| // otherwise "add second device" or "backup" may break. | ||
| // moreover, this allows downgrades to the previous version. | ||
| // writing of `addr` and `is_default` can be removed ~ 2024-08 | ||
| let addr = keypair.addr.to_string(); |
There was a problem hiding this comment.
Is to_string() really needed here?
There was a problem hiding this comment.
idk - i did not wanted to optimise code but just do what was done before to protect against further side effects :)
the code is subject to be removed in some months anyways
| sql.execute_migration( | ||
| "ALTER TABLE keypairs ADD COLUMN addr TEXT DEFAULT '' COLLATE NOCASE; | ||
| ALTER TABLE keypairs ADD COLUMN is_default INTEGER DEFAULT 0; | ||
| ALTER TABLE keypairs ADD COLUMN created INTEGER DEFAULT 0; |
There was a problem hiding this comment.
created column is not used even by old versions?
There was a problem hiding this comment.
i am not sure, therefore just played safe :)
There was a problem hiding this comment.
i just checked, it was never read but written in store_self_keypair() - therefore it is needed
5 participants
PR #5099 removed some columns in the database that were actually in use.
usually, to not worsen UX unnecessarily
(releases take time - in between, "Add Second Device", "Backup" etc. would fail), we try to avoid such schema changes (checking for db-version would avoid import etc. but would still worse UX),
see discussion at #2294.
these are the errors, the user will be confronted with otherwise:
it is not great to maintain the old columns, but well :)
as no official releases with newer cores are rolled out yet, i think, it is fine to change the "107" migration
and not copy things a second time in a newer migration.
(this issue happens to me during testing, and is probably also the issue reported by @lk108 for ubuntu-touch)