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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox47 affected)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

3 years ago
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?
(Assignee)

Comment 1

3 years ago
(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?
Flags: needinfo?(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!
Flags: needinfo?(rnewman)
(Assignee)

Comment 3

3 years ago
Created attachment 8730954 [details]
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/
Attachment #8730954 - Flags: review?(michael.l.comella)
Assignee: nobody → ahunt
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.
Attachment #8730954 - Flags: review?(michael.l.comella) → review+
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

2 years ago
Attachment #8730954 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.