1249657 - onUpgrade is already run in a transaction - don't do our own transaction management

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Andrzej Hunt :ahunt]

According to the Android docs, SQLiteOpenHelper.onUpgrade will be run within a transaction:
http://developer.android.com/reference/android/database/sqlite/SQLiteOpenHelper.html#onUpgrade%28android.database.sqlite.SQLiteDatabase,%20int,%20int%29

However we're also doing our own transaction management in our 17to18 upgrade method:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#824

I'm guessing we just want to remove all the begin.../set...Successful/end... calls there?

Comment 1

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #0)
> According to the Android docs, SQLiteOpenHelper.onUpgrade will be run within
> a transaction:
> http://developer.android.com/reference/android/database/sqlite/
> SQLiteOpenHelper.html#onUpgrade%28android.database.sqlite.SQLiteDatabase,
> %20int,%20int%29
> 
> However we're also doing our own transaction management in our 17to18
> upgrade method:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/db/BrowserDatabaseHelper.java#824
> 
> I'm guessing we just want to remove all the begin.../set...Successful/end...
> calls there?

Does this sound sane, or should I just leave DB migration code (that seems to work) as is?

Comment 2

[Richard Newman [:rnewman]]

Nested SQLiteDatabase.beginTransaction-style transactions are no-ops.

http://stackoverflow.com/a/26107723/22003

The Android code doesn't fill me with confidence:

http://androidxref.com/2.3.6/xref/frameworks/base/core/java/android/database/sqlite/SQLiteDatabase.java#522

I'm happy either way. Keep working code, or remove code that shouldn't be relevant. Just make sure we don't suddenly start doing partial writes if we remove that transaction and fail half-way through!

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1249657 - Remove no-op transaction management, and fail directly in DB upgrades r?mcomella

Database upgrades are already run in a transaction, we shouldn't be trying
to do our own DB management on top of this. If the upgrade logic fails
we should propagate the Exception and let the framework deal with it.

See:
http://developer.android.com/reference/android/database/sqlite/SQLiteOpenHelper.html#onUpgrade%28android.database.sqlite.SQLiteDatabase,%20int,%20int%29

Review commit: https://reviewboard.mozilla.org/r/40265/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40265/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8730954 [details]
MozReview Request: Bug 1249657 - Remove no-op transaction management, and fail directly in DB upgrades r?mcomella

https://reviewboard.mozilla.org/r/40265/#review38653

This looks reasonable to me, but unlike Richard, I'd rather not change what's working in DB land because the risk of failure is high.

I'd be more comfortable if we could test this upgrade somehow (e.g. download a build with this DB version from archive.mozilla.org or check-out a revision), but I don't think it's worth the effort to do that and thus I don't think it's worth the risk to change this.