Canonical ID Schema Migration #1182
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/migration
branch
from
ed6348d to
062a6c1
Compare
Codecov Report
Continue to review full report at Codecov.
|
| @@ -530,6 +537,26 @@ private boolean tableContainsColumn(String table, String column) { | |||
| return columns; | |||
| } | |||
|
|
|||
| private void rewriteCanonicalIds() { | |||
| new SQLitePersistence.Query(db, "SELECT target_id, target_proto FROM targets") | |||
There was a problem hiding this comment.
Dumb question: this file doesn't appear to employ a transaction anywhere. Doesn't db.execSQL auto-commit if you don't? It seems like it would be bad if this partially committed.
There was a problem hiding this comment.
The transaction is opened here: https://cs.corp.google.com/aosp-master/frameworks/base/core/java/android/database/sqlite/SQLiteOpenHelper.java?q=f:SQLiteopenhelper.java&dr&l=409
It will only get committed if line 421 executes.
| TargetData targetData = serializer.decodeTargetData(targetProto); | ||
| String updatedCanonicalId = targetData.getTarget().getCanonicalId(); | ||
| db.execSQL( | ||
| "UPDATE targets SET canonical_id = ? WHERE target_id = ?", |
There was a problem hiding this comment.
This migration can't be rolled back and has the potential to cause problems because target_id must be guaranteed unique (it's the primary key of the table).
If a user does roll back, will this actually cause a problem with duplicate target ids? I think probably not because the next target id is in target_globals and we're not changing that here, right?
One alternative that makes this slightly more rollback friendly would be to put the new target id in a new column and create an additional index on that column. I can't say I'm super excited about this prospect and likely would prefer to just document that this release includes a migration that can't be rolled-back from.
There was a problem hiding this comment.
Good catch. The fix for this is actually pretty straightforward as we can always rewrite canonical IDs with the current format. The target IDs don't change and as as long the format in the schema matches the expected format at the current client version we should be safe. I modified the schema converter to support downgrades and updated the code to run this particular migration on both up- and downgrades.
There was a problem hiding this comment.
Hm. I just realized that the new version of the schema converter is pretty dumb. It assumes that my shiny new code is shipped to old SDKs. What do you think about submitting the PR in its original form and letting LRU GC delete stale QueryData if the user chooses to downgrade?
There was a problem hiding this comment.
Letting LRU GC do its thing SGTM.
There was a problem hiding this comment.
I reverted the commit that pretended to add support for downgrades.
schmidt-sebastian
force-pushed
the
mrschmidt/migration
branch
from
3ff001b to
3e64a82
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/migration
branch
from
3e64a82 to
062a6c1
Compare
No description provided.