449884 - Stop using mozIStorageConnection::GetLastInsertRowID

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

It's not reliable when you start using more than one thead, and it doesn't work when you have a trigger managing inserts.

It'd be really awesome if someone other than me could take this.

Comment 1

[Shawn Wilsher :sdwilsh]

and no, storage can't fix this.  It's a limitation of sqlite.

Comment 2

[Shawn Wilsher :sdwilsh]

On second thought, I can't tell if failures are because of this or not (I've already fixed one case which got me a lot further along in the unit tests).  I'm just gonna fix this now.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Not really a whole lot of change, thankfully.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 333046 [details] [diff] [review]
v1.0


>+  // mDBGetLastBookmarkID
>+  rv = DBConn()->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT id "
>+      "FROM moz_bookmarks "
>+      "ORDER BY ROWID DESC "
>+      "LIMIT 1"),
>+    getter_AddRefs(mDBGetLastBookmarkID));

how does perform vs max(rowid) on large tables?

>+  {
>+    mozStorageStatementScoper scoper(mDBGetLastBookmarkID);
>+    
>+    PRBool hasResults;
>+    rv = mDBGetLastBookmarkID->ExecuteStep(&hasResults);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rowId = *aNewBookmarkId = mDBGetLastBookmarkID->AsInt64(0);
>+  }

sanity check hasResults as you did with mDBGetURLPageInfo?

>@@ -1188,18 +1204,24 @@ nsNavBookmarks::CreateContainerWithID(PR
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = statement->BindInt64Parameter(5, PR_Now());
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = statement->Execute();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRInt64 id;
>-  rv = dbConn->GetLastInsertRowID(&id);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  {
>+    mozStorageStatementScoper scoper(mDBGetLastBookmarkID);
>+    
>+    PRBool hasResults;
>+    rv = mDBGetLastBookmarkID->ExecuteStep(&hasResults);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    id = mDBGetLastBookmarkID->AsInt64(0);
>+  }

ditto

>@@ -1246,19 +1268,24 @@ nsNavBookmarks::InsertSeparator(PRInt64 
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = statement->BindInt64Parameter(3, PR_Now()); 
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = statement->Execute();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRInt64 rowId;
>-  rv = dbConn->GetLastInsertRowID(&rowId);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  *aNewItemId = rowId;
>+  {
>+    mozStorageStatementScoper scoper(mDBGetLastBookmarkID);
>+    
>+    PRBool hasResults;
>+    rv = mDBGetLastBookmarkID->ExecuteStep(&hasResults);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rowId = *aNewItemId = mDBGetLastBookmarkID->AsInt64(0);
>+  }

ditto

>+
>+      nsCOMPtr<mozIStorageStatement> idStmt;
>+      rv = DBConn()->CreateStatement(NS_LITERAL_CSTRING(
>+          "SELECT id "
>+          "FROM moz_keywords "
>+          "ORDER BY ROWID DESC "
>+          "LIMIT 1"),
>+        getter_AddRefs(idStmt));
>       NS_ENSURE_SUCCESS(rv, rv);
>+
>+      PRBool hasResults;
>+      rv = idStmt->ExecuteStep(&hasResults);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      keywordId = idStmt->AsInt64(0);

ditto for both perf and sanity check comments from above

r=me w/ these fixed

Comment 5

[Marco Bonardo [:mak]]

hey Shawn, couldn't you cache the lastInsertRowId every time you execute a insert (or insert or update) directly in mozStorage in some sort of thread scoped variable and return the cached value so the function will become thread safe? Clearly you should ensure that insert and caching are happening sequentially with some locking

Comment 6

[Shawn Wilsher :sdwilsh]

Sure, but there's a few problems with that:
1) Getting that locking correct is going to be difficult
2) Most consumers don't care about the last inserted id, so doing that work just slows them down for no added value
3) The locking is going to add more overhead to every consumer as well.

Additionally, it doesn't solve the issue of using a trigger to do the inserts, like we do.  See http://www.sqlite.org/c3ref/last_insert_rowid.html

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> how does perform vs max(rowid) on large tables?
MAX(ROWID) appears to be about 40-50% slower, but we are talking less then 40k ticks either way for my places.sqlite file on moz_historyvisits.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> sanity check hasResults as you did with mDBGetURLPageInfo?
I went ahead and add NS_ASSERTION for all of those - the call shouldn't succeed and hasResult fail, so there's no reason to check that for release builds.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments

Comment 10

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/5649accdc89b