482346 - mDBFindURIBookmarks can't use indices to sort

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

with bug 481261 this query will warn due to the fact it cannot use an index to sort.

current query is

214       "SELECT b.id "
215       "FROM moz_bookmarks b "
216       "JOIN ( "
217         "SELECT id FROM moz_places_temp "
218         "WHERE url = ?1 "
219         "UNION ALL "
220         "SELECT id FROM moz_places "
221         "WHERE url = ?1 "
222         "AND +id NOT IN (SELECT id FROM moz_places_temp) "
223       ") AS h ON b.fk = h.id "
224       "WHERE b.type = ?2 "
225       "ORDER BY MAX(IFNULL(b.lastModified, 0), b.dateAdded) DESC, b.id DESC"),

if we suppose that lastModified > dateAdded and dateAdded can't be null we could do

SELECT b.id
FROM moz_bookmarks b
WHERE b.type = 1 AND b.fk = (
    SELECT id FROM moz_places_temp
    WHERE url = "chrome://mcd/content/s3.xul"
    UNION
    SELECT id FROM moz_places
    WHERE url = "chrome://mcd/content/s3.xul"
    LIMIT 1
)
ORDER BY IFNULL(b.lastModified, b.dateAdded) DESC, b.id DESC

this has a better explain, but won't stop warn, since we can't have an index on the first order by clause

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

and eventually, that's it

Comment 2

[Marco Bonardo [:mak]]

Mardak suggests setting lastModified to dateAdded on items' creation.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Mardak suggests setting lastModified to dateAdded on items' creation.
That seems sensible to me.  Does doing this get rid of the warning?  If so, we should totally do this!

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 366425 [details] [diff] [review]
patch v1.0

canceling request until comment 3 is addressed

Comment 5

[Marco Bonardo [:mak]]

that will require a schema update to fix all empty dates, we wanted already to do this in the past, i can't recall why we didn't atm.

Comment 6

[Shawn Wilsher :sdwilsh]

disabled the warning for this query for now:
http://hg.mozilla.org/mozilla-central/rev/8f59f15f01a0

Comment 7

[Marco Bonardo [:mak]]

i just notice actually most bookmarks have lastModified filled, because after adding the bookmark we set description, that is an annotation, and that causes lastModified to be set.

Comment 8

[Marco Bonardo [:mak]]

Attachment: wip

1. set lastModified = dateAdded
2. update treeView to avoid showing lastModified if it's the same as dateAdded
3. try to use mDBInsertBookmark globally, to use prepared statement
4. try to set lastModified immediately, without having to run 2 queries.

needs cleanup and a small test.

Comment 9

[Marco Bonardo [:mak]]

I think i'm not going to apply any "trick" to hide lastModified if the item has not been modified.
This was a design choice, but has not real benefits imo. Hiding could be done at 2 levels:
- at a query level with an IFNULL we could return 0 if the 2 dates are equal. This would add complication to a major number of queries, is worth doing it?
- at a result level, we could set an empty value on nodes, but this would be out of sync with real value
- at a view level, but then when sorting we would use the node's value, so sorting results would appear "strange"

I think there's no point in hiding lastModified because:
- thinking to lastModified use cases, it's quite clear user expects to see last changes he did to bookmarks, and adding a bookmark is a change.
- when we add a bookmark for a page we usually set also description taking it from the page if it's available, this means the main creation point for bookmarks will already set lastModified just some ms after dateAdded.

Thoughts?

Comment 10

[Marco Bonardo [:mak]]

Notice on Windows for example you see both dateAdded and lastModified, where lastMod is set to dateAdded if no further changes have been made.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v2.0

asking code review to Shawn. btw i'll ask an additional review to Dietrich for the approach change (having lastModified always filled)

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 383941 [details] [diff] [review]
patch v2.0

>+++ b/toolkit/components/places/public/nsINavBookmarksService.idl
>+   * @note: when an item is added lastModified is set to the same value as
>+   *        dateAdded.  To know if an item has been really modified is possible
>+   *        to compare these 2 attributes.
two things:
1) I'm not really sure what the second sentence is trying to say.
2) spell out numbers <= 10 please

>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>   // mDBSetItemDateAdded
>+  // Notice we want to set dateAdded and lastModified to the same value, we do
>+  // this for performance reasons since will allow to use an index.
>+  // Moreover having (lastModified < dateAdded) would not make sense.
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>-      "UPDATE moz_bookmarks SET dateAdded = ?1 WHERE id = ?2"),
>+      "UPDATE moz_bookmarks SET dateAdded = ?1, lastModified = ?1 "
>+      "WHERE id = ?2"),
This comment is also not clear

>+  PRTime dateAdded = PR_Now();
>   {
>     mozStorageStatementScoper scope(mDBInsertBookmark);
>-    rv = mDBInsertBookmark->BindInt64Parameter(0, childID);
>+    rv = mDBInsertBookmark->BindNullParameter(kInsertBookmarkIndex_Id);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBInsertBookmark->BindInt32Parameter(1, TYPE_BOOKMARK);
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_PlaceId, childID);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBInsertBookmark->BindInt64Parameter(2, aFolder);
>+    rv = mDBInsertBookmark->BindInt32Parameter(kInsertBookmarkIndex_Type, TYPE_BOOKMARK);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBInsertBookmark->BindInt32Parameter(3, index);
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_Parent, aFolder);
>     NS_ENSURE_SUCCESS(rv, rv);
>-  
>+    rv = mDBInsertBookmark->BindInt32Parameter(kInsertBookmarkIndex_Position, index);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Support NULL titles.
>     if (aTitle.IsVoid())
>-      rv = mDBInsertBookmark->BindNullParameter(4);
>+      rv = mDBInsertBookmark->BindNullParameter(kInsertBookmarkIndex_Title);
>     else
>-      rv = mDBInsertBookmark->BindUTF8StringParameter(4, aTitle);
>+      rv = mDBInsertBookmark->BindUTF8StringParameter(kInsertBookmarkIndex_Title, aTitle);
>     NS_ENSURE_SUCCESS(rv, rv);
>-  
>-    rv = mDBInsertBookmark->BindInt64Parameter(5, PR_Now());
>+    rv = mDBInsertBookmark->BindNullParameter(kInsertBookmarkIndex_ServiceContractId);
>     NS_ENSURE_SUCCESS(rv, rv);
>-  
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_DateAdded, dateAdded);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_LastModified, dateAdded);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>     rv = mDBInsertBookmark->Execute();
>     NS_ENSURE_SUCCESS(rv, rv);
Can we pull all this out into a helper method that takes all the arguments we'll bind?  We use this logic in at least two places :/

>+  // XXX: do we really need to notify lastModified has changed? since the item
>+  // has changed, it's quite clear that lastModified has changed.
I feel like we shouldn't notify here

>+  // XXX: do we really need to notify lastModified has changed? since the item
>+  // has changed, it's quite clear that lastModified has changed.
>+  NOTIFY_DATE_CHANGED(aBookmarkId, "lastModified", lastModified);
likewise here

>+  // XXX: Do we really need to notify that lastModified has changed? since the
>+  // item has changed, it's quite clear that lastModified has changed.
>+  NOTIFY_DATE_CHANGED(aBookmarkId, "lastModified", lastModified);
ditto

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v2.1

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 384034 [details] [diff] [review]
patch v2.1

>+++ b/toolkit/components/places/public/nsINavBookmarksService.idl
>+   * @note: This is the only method that will send an itemChanged notification
nit: no : after note.

>+   *        for property "lastModified".  lastModified will still be updated in
nit: "for the property".

>+   * @note: when an item is added lastModified is set to the same value as
>+   *        dateAdded.
ditto, and capitalize the 'w' please.  also, comma after "is added"

>+++ b/toolkit/components/places/public/nsINavHistoryService.idl
>+   * @note: when an item is added lastModified is set to the same value as
>+   *        dateAdded.
ditto here

>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>   // mDBSetItemDateAdded
>+  // lastModified is set to the same value as dateAdded, we do this for
I think we want a period, and not a comma after dataAdded.

>+  // performance reasons, since will allow to use an index to sort items by
nit: "since it will allow us to use"


>+nsresult
>+nsNavBookmarks::InsertBookmarkInDB(PRInt64 aItemId,
>+                                   PRInt64 aPlaceId,
>+                                   PRUint16 aItemType,
>+                                   PRInt64 aParentId,
>+                                   PRInt32 aIndex,
>+                                   const nsACString &aTitle,
>+                                   const nsAString &aServiceContractId,
>+                                   PRTime aDateAdded,
>+                                   PRTime aLastModified,
>+                                   PRInt64 *_retval)
_retval should be something more descriptive like _itemId

>+{
>+  NS_ENSURE_ARG_POINTER(_retval);
This isn't a public API, so it should be an assertion (we don't need to check it)

>+    NS_ASSERTION(hasResult, "hasResult is false but the call succeeded?");
We actually want to return an error code here.  I know I've argued the opposite in the past, and something would be really wrong if that happened.

>-  return CreateContainerWithID(-1, aParent, aName, EmptyString(), PR_TRUE,
>+  nsString voidString;
>+	voidString.SetIsVoid(PR_TRUE);
nit: tab

>+++ b/toolkit/components/places/src/nsNavBookmarks.h
>+   * @param aItemId
>+   *        itemId to insert, pass -1 or nsnull to generate a new one.
nit: "The itemId..."
I feel like we should only allow -1 here actually.

>+   * @param aPlaceId
>+   *        placeId to which this bookmark refers to, pass nsnull for
>+   *        items that don't refer to an URI (eg. folders, separators, ...).
nit: "The palceId..."
again  -1

>+   * @param aItemType
>+   *        type of the new bookmark, see TYPE_* constants.
nit: "The type..." I'm going to stop saying this one, but it applies for this whole method.
I kind want an enum here just for extra type checking and making sure we handle possible new ones
>+   * @param aTitle
>+   *        title for the new bookmark, pass a void string to set a NULL title.
nit: Use a new sentance for the second part.

>+   * @param aServiceContractId
>+   *        the contract id for a dynamic container, pass a void string for
>+   *        other types of items.
>+   * @param aDateAdded
>+   *        date for the insertion.
>+   * @param aLastModified
>+   *        last modified date for the insertion, usually equal to aDateAdded.
I think we should re-order these last three, and make aLastModified and aServiceContractId optional.  The order should then be aDataAdded, aLastModified, and then aServiceContractId.  aServiceContractId would just check for IsEmpty instead of IsVoid, and the default value can be EmptyCString();

>+   * @note: This will also update last modified date of the parent folder.
nit: no colon

r=sdwilsh with these changes

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2.2

asking for a quick code check, and approval for the behavior change (always show last modified)

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v2.3

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 384462 [details] [diff] [review]
patch v2.3

r=me

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/efbde7c87cdf