If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Stop using mozIStorageConnection::GetLastInsertRowID

RESOLVED FIXED in mozilla1.9.1a2

Status

()

Toolkit
Places
P1
blocker
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.1a2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
and no, storage can't fix this.  It's a limitation of sqlite.
(Assignee)

Comment 2

9 years ago
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.
Assignee: nobody → sdwilsh
Priority: -- → P1
(Assignee)

Comment 3

9 years ago
Created attachment 333046 [details] [diff] [review]
v1.0

Not really a whole lot of change, thankfully.
Attachment #333046 - Flags: review?(dietrich)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review dietrich]
(Assignee)

Updated

9 years ago
Blocks: 449640
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
Attachment #333046 - Flags: review?(dietrich) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review dietrich] → [has patch][needs perf analysis]
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
(Assignee)

Comment 6

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

Comment 7

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
(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.
(Assignee)

Comment 9

9 years ago
Created attachment 333662 [details] [diff] [review]
v1.1

Addresses review comments
Attachment #333046 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs perf analysis] → [has patch][has review][can land]
(Assignee)

Comment 10

9 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/5649accdc89b
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]

Updated

9 years ago
Depends on: 452777
You need to log in before you can comment on or make changes to this bug.