Closed
Bug 736348
Opened 12 years ago
Closed 12 years ago
Child positioning hits sqlite limits
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(blocking-fennec1.0 beta+)
RESOLVED
FIXED
Firefox 14
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | beta+ |
People
(Reporter: rnewman, Assigned: rnewman)
Details
(Whiteboard: [sync])
Attachments
(1 file, 1 obsolete file)
14.83 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
android.database.sqlite.SQLiteException: too many SQL variables: , while compiling: UPDATE bookmarks SET position = CASE guid WHEN ? THEN 0 WHEN ? THEN 1 WHEN ? … We should batch these. This means that positioning of folders with more than 333 children will fail.
Assignee | ||
Comment 1•12 years ago
|
||
This needs to be fixed in the CP.
Component: Android Sync → General
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
Assignee | ||
Comment 2•12 years ago
|
||
The alternative is for callers to know the limit, make multiple calls, and for us to add an ?offset parameter to the CP URI to allow that. Probably best to just issue multiple queries within a transaction… If I get time tomorrow I'll address this.
blocking-fennec1.0: --- → ?
Assignee | ||
Comment 3•12 years ago
|
||
Figure I'd whiteboard this before blassey gets to it :D
Whiteboard: [sync]
Comment 4•12 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #2) > The alternative is for callers to know the limit, make multiple calls, and > for us to add an ?offset parameter to the CP URI to allow that. > > Probably best to just issue multiple queries within a transaction… I guess you can do this transparently, no? You could just change BOOKMARKS_POSITIONS update handler to split the passed guids into smaller batches and build queries from those. This way callers can just send as many guids as they need at once and the CP would just do the right/safe thing under the hood.
Comment 5•12 years ago
|
||
Before I forget: please add a test case to cover the BOOKMARKS_POSITIONS uri and this specific bug.
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #4) > I guess you can do this transparently, no? You could just change > BOOKMARKS_POSITIONS update handler to split the passed guids into smaller > batches and build queries from those. This way callers can just send as many > guids as they need at once and the CP would just do the right/safe thing > under the hood. Yes, that's exactly what I meant by "just issue multiple queries within a transaction". :) (In reply to Lucas Rocha (:lucasr) from comment #5) > Before I forget: please add a test case to cover the BOOKMARKS_POSITIONS uri > and this specific bug. I will see what I can do.
Updated•12 years ago
|
blocking-fennec1.0: ? → beta+
Assignee | ||
Comment 7•12 years ago
|
||
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Attachment #607405 -
Flags: feedback?(lucasr.at.mozilla)
Comment 8•12 years ago
|
||
Comment on attachment 607405 [details] [diff] [review] First stab, no tests. Review of attachment 607405 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: mobile/android/base/db/BrowserProvider.java.in @@ +1287,5 @@ > return 0; > > + final SQLiteDatabase db = getWritableDatabase(uri); > + int offset = 0; > + int updated = 0; nit: add empty line here. @@ +1288,5 @@ > > + final SQLiteDatabase db = getWritableDatabase(uri); > + int offset = 0; > + int updated = 0; > + db.beginTransaction(); nit: add empty line here. I want this line to be clearly visible. @@ +1302,5 @@ > + // returned update count will be smaller than the requested > + // number of records. > + db.setTransactionSuccessful(); > + db.endTransaction(); > + db.beginTransaction(); Oh man, this is so... hacky. Android's fault, right? @@ +1303,5 @@ > + // number of records. > + db.setTransactionSuccessful(); > + db.endTransaction(); > + db.beginTransaction(); > + } nit: I'd add an empty line here. @@ +1307,5 @@ > + } > + offset += MAX_POSITION_UPDATES_PER_QUERY; > + } > + db.setTransactionSuccessful(); > + db.endTransaction(); those calls should be done inside the try block, no? If they are done here, setTransactionSuccessfull() and endTransaction() will be called twice if the update throws.
Attachment #607405 -
Flags: feedback?(lucasr.at.mozilla) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #8) > @@ +1302,5 @@ > > + // returned update count will be smaller than the requested > > + // number of records. > > + db.setTransactionSuccessful(); > > + db.endTransaction(); > > + db.beginTransaction(); > > Oh man, this is so... hacky. Android's fault, right? SQLite's. Once something fails, the transaction is in the ABORT state, and every other operation will fail until we end it and start a new one. See the implementation of applyBatch later in the file. > > + db.setTransactionSuccessful(); > > + db.endTransaction(); > > those calls should be done inside the try block, no? If they are done here, > setTransactionSuccessfull() and endTransaction() will be called twice if the > update throws. I don't think so. If the update throws a SQLException, then the catch block will begin a new transaction, and these two lines will end it. If the update does not throw, then the transaction we originally began will be ended here. In other words: by the time the while loop ends, there will always be an open transaction. If anything, this should be in a try block inside a finally block, but if we get some other kind of exception we're screwed anyway, so... :D
Comment 10•12 years ago
|
||
Patch looks good just want to see tests before landing. Given this is a currently untested area (the position update URI), I'd be open to get the tests in a follow-up bug.
Assignee | ||
Comment 11•12 years ago
|
||
It -- and tests -- are still rotting in my queue :D Should get to this this week!
Assignee | ||
Comment 12•12 years ago
|
||
With tests, and fixed so it works :)
Attachment #607405 -
Attachment is obsolete: true
Attachment #612443 -
Flags: review?(lucasr.at.mozilla)
Comment 13•12 years ago
|
||
Comment on attachment 612443 [details] [diff] [review] Proposed patch. v1 Review of attachment 612443 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with the suggest fixes. ::: mobile/android/base/tests/testBrowserProvider.java.in @@ +90,5 @@ > mHistoryUri = getContentUri("History"); > mImagesUri = getContentUri("Images"); > mCombinedUri = getContentUri("Combined"); > > + mBookmarksPositionUri = getPositionContentUri(); Maybe add a getUriColumn() to ContentProviderTest and use it here? @@ +283,5 @@ > public void setUp() throws Exception { > super.setUp("@ANDROID_PACKAGE_NAME@.db.BrowserProvider", "AUTHORITY"); > loadContractInfo(); > > + mTests.add(new TestPositionBookmarks()); Move this together with the bookmark-related tests. @@ +686,5 @@ > + * Verify that the reordering worked by querying. > + */ > + class TestPositionBookmarks extends Test { > + > + // Utilities. No need for this comment I guess. @@ +717,5 @@ > + */ > + public long getIdFromUri(Uri uri) { > + String path = uri.getPath(); > + int lastSlash = path.lastIndexOf('/'); > + return Long.parseLong(path.substring(lastSlash + 1)); I think you can just use ContentUris.parseId() here. See: http://developer.android.com/reference/android/content/ContentUris.html#parseId%28android.net.Uri%29 @@ +735,5 @@ > + > + // Reuse the same ContentValues. > + ContentValues item = createBookmark("Test Bookmark", "http://example.com", folderId, > + mBookmarksTypeFolder, 0, "", > + "description", "keyword"); nit: add empty line here.
Comment 14•12 years ago
|
||
Comment on attachment 612443 [details] [diff] [review] Proposed patch. v1 Review of attachment 612443 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with the suggest fixes. ::: mobile/android/base/tests/testBrowserProvider.java.in @@ +90,5 @@ > mHistoryUri = getContentUri("History"); > mImagesUri = getContentUri("Images"); > mCombinedUri = getContentUri("Combined"); > > + mBookmarksPositionUri = getPositionContentUri(); Maybe add a getUriColumn() to ContentProviderTest and use it here? @@ +283,5 @@ > public void setUp() throws Exception { > super.setUp("@ANDROID_PACKAGE_NAME@.db.BrowserProvider", "AUTHORITY"); > loadContractInfo(); > > + mTests.add(new TestPositionBookmarks()); Move this together with the bookmark-related tests. @@ +686,5 @@ > + * Verify that the reordering worked by querying. > + */ > + class TestPositionBookmarks extends Test { > + > + // Utilities. No need for this comment I guess. @@ +717,5 @@ > + */ > + public long getIdFromUri(Uri uri) { > + String path = uri.getPath(); > + int lastSlash = path.lastIndexOf('/'); > + return Long.parseLong(path.substring(lastSlash + 1)); I think you can just use ContentUris.parseId() here. See: http://developer.android.com/reference/android/content/ContentUris.html#parseId%28android.net.Uri%29 @@ +735,5 @@ > + > + // Reuse the same ContentValues. > + ContentValues item = createBookmark("Test Bookmark", "http://example.com", folderId, > + mBookmarksTypeFolder, 0, "", > + "description", "keyword"); nit: add empty line here.
Attachment #612443 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Pushed with nits: https://hg.mozilla.org/integration/mozilla-inbound/rev/c527278f647a All browser tests pass, and Sync's do, too. Thanks for the review, Lucas, and have a good long weekend!
Target Milestone: --- → Firefox 14
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c527278f647a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•