fix for bug #392399: use multi-column index on moz_historyvisits to improve performance, remove unused code, and fix the queries where we attempt to find the max visit date for a place to always exclude transitions of type 0 and 4. patch=Marco Bonardo <mak77@supereva.it> analysis, suggestions and review=Ondrej Brablc <ondrej@allpeers.com> r=sspitzer a=blocking-firefox-3-+

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -157,8 +157,8 @@ nsNavBookmarks::Init()
   nsCAutoString selectChildren(
     NS_LITERAL_CSTRING("SELECT h.id, h.url, COALESCE(a.title, h.title), "
       "h.rev_host, h.visit_count, "
-      "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id), "
+      SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
-      "f.url, null, a.id, "
+      ", f.url, null, a.id, "
       "a.dateAdded, a.lastModified, "
       "a.position, a.type, a.fk "
      "FROM moz_bookmarks a "

toolkit/components/places/src/nsNavHistory.cpp

@@ -643,6 +643,10 @@ nsNavHistory::InitDB(PRBool *aDoImport)
     rv = UpdateSchemaVersion();
     NS_ENSURE_SUCCESS(rv, rv);
   }
+  else {
+    rv = EnsureCurrentSchema(mDBConn);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
   // Get the page size. This may be different than was set above if the database
   // file already existed and has a different page size.
@@ -730,7 +734,8 @@ nsNavHistory::InitDB(PRBool *aDoImport)
     NS_ENSURE_SUCCESS(rv, rv);
 
     rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
-        "CREATE INDEX moz_historyvisits_pageindex ON moz_historyvisits (place_id)"));
+        "CREATE INDEX moz_historyvisits_placedateindex "
+        "ON moz_historyvisits (place_id, visit_date)"));
     NS_ENSURE_SUCCESS(rv, rv);
 
     // This makes a big difference in startup time for large profiles because of
@@ -783,17 +788,6 @@ nsNavHistory::InitStatements()
     getter_AddRefs(mDBGetURLPageInfo));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // mDBGetURLPageInfoFull
-  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
-        "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id), "
-        "f.url "
-      "FROM moz_places h "
-      "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
-      "WHERE h.url = ?1 "),
-    getter_AddRefs(mDBGetURLPageInfoFull));
-  NS_ENSURE_SUCCESS(rv, rv);
-
   // mDBGetIdPageInfo
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count "
@@ -801,17 +795,6 @@ nsNavHistory::InitStatements()
                                 getter_AddRefs(mDBGetIdPageInfo));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // mDBGetIdPageInfoFull
-  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
-        "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id), "
-        "f.url "
-      "FROM moz_places h "
-      "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
-      "WHERE h.id = ?1"),
-    getter_AddRefs(mDBGetIdPageInfoFull));
-  NS_ENSURE_SUCCESS(rv, rv);
-
   // mDBRecentVisitOfURL
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT v.id, v.session "
@@ -857,8 +840,8 @@ nsNavHistory::InitStatements()
   // mDBVisitToURLResult, should match kGetInfoIndex_* (see GetQueryResults)
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
-        "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id), "
+        SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
-        "f.url, null, null "
+        ", f.url, null, null "
       "FROM moz_places h "
       "JOIN moz_historyvisits v ON h.id = v.place_id "
       "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
@@ -880,8 +863,8 @@ nsNavHistory::InitStatements()
   // mDBUrlToURLResult, should match kGetInfoIndex_*
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
-        "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id), "
+        SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
-        "f.url, null, null "
+        ", f.url, null, null "
       "FROM moz_places h "
       "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
       "WHERE h.url = ?1"),
@@ -891,8 +874,8 @@ nsNavHistory::InitStatements()
   // mDBBookmarkToUrlResult, should match kGetInfoIndex_*
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "SELECT b.fk, h.url, b.title, h.rev_host, h.visit_count, "
-        "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = b.fk), "
+        SQL_STR_FRAGMENT_MAX_VISIT_DATE( "b.fk" )
-        "f.url, null, null, b.dateAdded, b.lastModified "
+        ", f.url, null, null, b.dateAdded, b.lastModified "
       "FROM moz_bookmarks b "
       "JOIN moz_places h ON b.fk = h.id "
       "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
@@ -1030,6 +1013,38 @@ nsNavHistory::MigrateV6Up(mozIStorageConnection* aDBConn)
   return NS_OK;
 }
 
+nsresult
+nsNavHistory::EnsureCurrentSchema(mozIStorageConnection* aDBConn)
+{
+  // We need to do a one-time change of the moz_historyvisits.pageindex
+  // to speed up finding last visit date when joinin with moz_places.
+  // See bug 392399 for more details.
+  PRBool oldIndexExists = PR_FALSE;
+  nsresult rv = aDBConn->IndexExists(
+    NS_LITERAL_CSTRING("moz_historyvisits_pageindex"), &oldIndexExists);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  if (oldIndexExists) {
+    // wrap in a transaction for safety and performance
+    mozStorageTransaction pageindexTransaction(aDBConn, PR_FALSE);
+
+    // drop old index
+    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+        "DROP INDEX IF EXISTS moz_historyvisits_pageindex"));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // create the new multi-column index
+    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+        "CREATE INDEX IF NOT EXISTS moz_historyvisits_placedateindex "
+        "ON moz_historyvisits (place_id, visit_date)"));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = pageindexTransaction.Commit();
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+  return NS_OK;
+}
+
 nsresult
 nsNavHistory::CleanUpOnQuit()
 {
@@ -2268,16 +2283,19 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
     // visit_type <> 0 == undefined (see bug #375777 for details)
     queryString = NS_LITERAL_CSTRING(
       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
-      "MAX(v.visit_date), f.url, null, null "
+        SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
+        ", f.url, null, null "
       "FROM moz_places h "
-      "JOIN moz_historyvisits v ON h.id = v.place_id "
+      "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
-      "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id WHERE "
+      "WHERE h.id IN ( "
-      "(h.id IN (SELECT DISTINCT h.id FROM moz_historyvisits, "
+        "SELECT DISTINCT p.id "
-      " moz_places h WHERE place_id = "
+        "FROM moz_places p "
-      " h.id AND hidden <> 1 AND visit_type <> 4 AND visit_type <> 0 "
+        "JOIN moz_historyvisits ON place_id = p.id "
-      " ORDER BY visit_date DESC LIMIT ");
+        "WHERE hidden <> 1 AND visit_type NOT IN(0,4) "
+        "ORDER BY visit_date DESC "
+        "LIMIT ");
     queryString.AppendInt(aOptions->MaxResults());
-    queryString += NS_LITERAL_CSTRING(")) GROUP BY h.id ORDER BY 6 DESC"); // v.visit_date
+    queryString += NS_LITERAL_CSTRING(") ORDER BY 6 DESC"); // v.visit_date
     return NS_OK;
   }
 
@@ -2287,9 +2305,8 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
         nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING)) {
     queryString = NS_LITERAL_CSTRING(
       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
-      "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id "
+        SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
-      " AND visit_type <> 4 AND visit_type <> 0), "
+        ", f.url, null, null "
-      "f.url, null, null "
       "FROM moz_places h "
       "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id WHERE "
       "h.id IN (SELECT id FROM moz_places WHERE hidden <> 1 "
@@ -2318,7 +2335,7 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
     // 4 == TRANSITION_EMBED
     // 0 == undefined (see bug #375777 for details)
     commonConditions.AssignLiteral(
-      "h.hidden <> 1 AND v.visit_type <> 4 AND v.visit_type <> 0 "); 
+      "h.hidden <> 1 AND v.visit_type NOT IN(0,4) ");
   }
 
   // Query string: Output parameters should be in order of kGetInfoIndex_*
@@ -2356,8 +2373,8 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
       queryString = NS_LITERAL_CSTRING("SELECT b.fk, h.url, COALESCE(b.title, h.title), ");
       queryString += NS_LITERAL_CSTRING(
         "h.rev_host, h.visit_count, "
-        "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = b.fk), "
+        SQL_STR_FRAGMENT_MAX_VISIT_DATE( "b.fk" )
-        "f.url, null, b.id, b.dateAdded, b.lastModified "
+        ", f.url, null, b.id, b.dateAdded, b.lastModified "
         "FROM moz_bookmarks b "
         "JOIN moz_places h ON b.fk = h.id "
         "LEFT OUTER JOIN moz_historyvisits v ON b.fk = v.place_id "
@@ -3469,6 +3486,8 @@ nsNavHistory::OnIdle()
     // XXX REMOVE ME AFTER BETA2.
     PRBool oldIndexExists = PR_FALSE;
     rv = mDBConn->IndexExists(NS_LITERAL_CSTRING("moz_places_urlindex"), &oldIndexExists);
+    NS_ENSURE_SUCCESS(rv, rv);
+ 
     if (oldIndexExists) {
       // wrap in a transaction for safety and performance
       mozStorageTransaction urlindexTransaction(mDBConn, PR_FALSE);
@@ -4900,35 +4919,6 @@ nsNavHistory::VisitIdToResultNode(PRInt64 visitId,
   return RowToResult(statement, aOptions, aResult);
 }
 
-
-// nsNavHistory::UriToResultNode
-//
-//    Used by the query results to create new nodes on the fly when
-//    notifications come in. This creates a URL node for the given URL.
-
-nsresult
-nsNavHistory::UriToResultNode(nsIURI* aUri, nsNavHistoryQueryOptions* aOptions,
-                              nsNavHistoryResultNode** aResult)
-{
-  // this query must be asking for URL results, because we don't have enough
-  // information to construct a visit result node here
-  NS_ASSERTION(aOptions->ResultType() == nsNavHistoryQueryOptions::RESULTS_AS_URI,
-               "Can't make visits from URIs");
-  mozStorageStatementScoper scoper(mDBUrlToUrlResult);
-  nsresult rv = BindStatementURI(mDBUrlToUrlResult, 0, aUri);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  PRBool hasMore = PR_FALSE;
-  rv = mDBUrlToUrlResult->ExecuteStep(&hasMore);
-  NS_ENSURE_SUCCESS(rv, rv);
-  if (! hasMore) {
-    NS_NOTREACHED("Trying to get a result node for an invalid URL");
-    return NS_ERROR_INVALID_ARG;
-  }
-
-  return RowToResult(mDBUrlToUrlResult, aOptions, aResult);
-}
-
 nsresult
 nsNavHistory::BookmarkIdToResultNode(PRInt64 aBookmarkId, nsNavHistoryQueryOptions* aOptions,
                                      nsNavHistoryResultNode** aResult)

toolkit/components/places/src/nsNavHistory.h

@@ -88,6 +88,13 @@
 #define QUERYUPDATE_COMPLEX 2
 #define QUERYUPDATE_COMPLEX_WITH_BOOKMARKS 3
 
+// this is a work-around for a problem with the optimizer of sqlite
+// A sub-select on MAX(visit_date) is slower than this query with our indexes
+// see Bug #392399 for more details
+#define SQL_STR_FRAGMENT_MAX_VISIT_DATE( place_relation ) \
+  "(SELECT visit_date FROM moz_historyvisits WHERE place_id = " place_relation \
+  " AND visit_type NOT IN (0,4) ORDER BY visit_date DESC LIMIT 1)"
+
 struct AutoCompleteIntermediateResult;
 class AutoCompleteResultComparator;
 class mozIAnnotationService;
@@ -221,17 +228,9 @@ public:
   static const PRInt32 kGetInfoIndex_ItemDateAdded;
   static const PRInt32 kGetInfoIndex_ItemLastModified;
 
-  // select a history row by URL, with visit date info (extra work)
-  mozIStorageStatement* DBGetURLPageInfoFull()
-  { return mDBGetURLPageInfoFull; }
-
   // select a history row by id
   mozIStorageStatement* DBGetIdPageInfo() { return mDBGetIdPageInfo; }
 
-  // select a history row by id, with visit date info (extra work)
-  mozIStorageStatement* DBGetIdPageInfoFull()
-  { return mDBGetIdPageInfoFull; }
-
   // Constants for the columns returned by the above statement
   // (in addition to the ones above).
   static const PRInt32 kGetInfoIndex_VisitDate;
@@ -261,9 +260,7 @@ public:
   nsresult VisitIdToResultNode(PRInt64 visitId,
                                nsNavHistoryQueryOptions* aOptions,
                                nsNavHistoryResultNode** aResult);
-  nsresult UriToResultNode(nsIURI* aUri,
+
-                           nsNavHistoryQueryOptions* aOptions,
-                           nsNavHistoryResultNode** aResult);
   nsresult BookmarkIdToResultNode(PRInt64 aBookmarkId,
                                   nsNavHistoryQueryOptions* aOptions,
                                   nsNavHistoryResultNode** aResult);
@@ -369,9 +366,7 @@ protected:
   nsCOMPtr<nsIFile> mDBFile;
 
   nsCOMPtr<mozIStorageStatement> mDBGetURLPageInfo;   // kGetInfoIndex_* results
-  nsCOMPtr<mozIStorageStatement> mDBGetURLPageInfoFull; // kGetInfoIndex_* results
   nsCOMPtr<mozIStorageStatement> mDBGetIdPageInfo;     // kGetInfoIndex_* results
-  nsCOMPtr<mozIStorageStatement> mDBGetIdPageInfoFull; // kGetInfoIndex_* results
 
   nsCOMPtr<mozIStorageStatement> mDBRecentVisitOfURL; // converts URL into most recent visit ID/session ID
   nsCOMPtr<mozIStorageStatement> mDBInsertVisit; // used by AddVisit
@@ -394,6 +389,7 @@ protected:
   nsresult ForceMigrateBookmarksDB(mozIStorageConnection *aDBConn);
   nsresult MigrateV3Up(mozIStorageConnection *aDBConn);
   nsresult MigrateV6Up(mozIStorageConnection *aDBConn);
+  nsresult EnsureCurrentSchema(mozIStorageConnection* aDBConn);
   nsresult CleanUpOnQuit();
 
 #ifdef IN_MEMORY_LINKS

toolkit/components/places/src/nsNavHistoryAutoComplete.cpp

@@ -106,7 +106,7 @@ nsNavHistory::CreateAutoCompleteQueries()
     "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id "
     "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
     "WHERE v.visit_date >= ?1 AND v.visit_date <= ?2 AND h.hidden <> 1 AND "
-    " v.visit_type <> 0 AND v.visit_type <> 4 AND ");
+    " v.visit_type NOT IN(0,4) AND ");
 
   if (mAutoCompleteOnlyTyped)
     sql += NS_LITERAL_CSTRING("h.typed = 1 AND ");
@@ -394,7 +394,7 @@ nsNavHistory::StartSearch(const nsAString & aSearchString,
     // determine our earliest visit
     nsCOMPtr<mozIStorageStatement> dbSelectStatement;
     rv = mDBConn->CreateStatement(
-      NS_LITERAL_CSTRING("SELECT MIN(visit_date) id FROM moz_historyvisits WHERE visit_type <> 4 AND visit_type <> 0"),
+      NS_LITERAL_CSTRING("SELECT MIN(visit_date) id FROM moz_historyvisits WHERE visit_type NOT IN(0,4)"),
       getter_AddRefs(dbSelectStatement));
     NS_ENSURE_SUCCESS(rv, rv);
     PRBool hasMinVisit;
@@ -452,7 +452,7 @@ nsresult nsNavHistory::AutoCompleteTypedSearch()
     "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
     "JOIN moz_historyvisits v ON h.id = v.place_id WHERE (h.id IN "
     "(SELECT DISTINCT h.id from moz_historyvisits v, moz_places h WHERE "
-    "v.place_id = h.id AND h.typed = 1 AND v.visit_type <> 0 AND v.visit_type <> 4 "
+    "v.place_id = h.id AND h.typed = 1 AND v.visit_type NOT IN(0,4) "
     "ORDER BY v.visit_date DESC LIMIT ");
   sql.AppendInt(AUTOCOMPLETE_MAX_PER_TYPED);
   sql += NS_LITERAL_CSTRING(")) GROUP BY h.id ORDER BY MAX(v.visit_date) DESC");