Closed Bug 736348 Opened 12 years ago Closed 12 years ago

Child positioning hits sqlite limits

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android
defect

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)

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.
This needs to be fixed in the CP.
Component: Android Sync → General
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
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: --- → ?
Figure I'd whiteboard this before blassey gets to it :D
Whiteboard: [sync]
(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.
Before I forget: please add a test case to cover the BOOKMARKS_POSITIONS uri and this specific bug.
(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.
blocking-fennec1.0: ? → beta+
Attached patch First stab, no tests. (obsolete) — Splinter Review
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #607405 - Flags: feedback?(lucasr.at.mozilla)
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+
(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
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.
It -- and tests -- are still rotting in my queue :D

Should get to this this week!
With tests, and fixed so it works :)
Attachment #607405 - Attachment is obsolete: true
Attachment #612443 - Flags: review?(lucasr.at.mozilla)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/c527278f647a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: