443068 - Move triggers into migration code

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Shawn Wilsher :sdwilsh]

We do not need test if triggers exists and then add them every time we start the places service.  These should be done in migration code for the proper schema versions (and for ease of seeing all the triggers in one place, they should be moved out to their own file).

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

This fails test_savedsearches.js, oddly enough.  I plan on looking into that more tomorrow.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

The only change between the previous version is that I had a NEW instead of OLD in the deletion trigger.  Luckily, we have unit test coverage that caught that (we actually have coverage for all the triggers being moved).

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Renaming the constants from PLACES_ to CREATE_ to be more inline with bug 443433's patch.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 327973 [details] [diff] [review]
v1.1

r=me. please test migration of <=v5, if you have not already done so.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 327973 [details] [diff] [review]
v1.1

nope, this won't work. there's a massive window between when dbs were upgraded to v6, and when the triggers were first checked in.

v6 migration was about 1 year ago: https://bugzilla.mozilla.org/show_bug.cgi?id=319455#c41

Anyone upgrading from a pre-trigger build to a build with this patch will not get the triggers added.

you could instead re-version the db to v7. and in the v7 upgrade only migrate if the triggers aren't detected (as in CreateTriggers in status quo). this would mean that migration would run for all builds once, but would effectively do nothing but bump the version number for builds that already have the triggers.

or you could do nothing, since the net result of this fix is mostly cosmetic. the perf improvement from a single db query for trigger existence will be minimal - there's likely opportunity for bigger wins elsewhere.

Comment 6

[Shawn Wilsher :sdwilsh]

I think it's worthwhile for code clarity, and the less time spent during startup is just a bonus.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 329507 [details] [diff] [review]
v1.2


>@@ -692,16 +693,22 @@ nsNavHistory::InitDB(PRInt16 *aMadeChang
>       }
> 
>       // Migrate anno tables up to V6
>       if (DBSchemaVersion < 6) {
>         rv = MigrateV6Up(mDBConn);
>         NS_ENSURE_SUCCESS(rv, rv);
>       }
> 
>+      // Migrate anno tables up to V7
>+      if (DBSchemaVersion < 7) {
>+        rv = MigrateV7Up(mDBConn);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+

s/anno//

>@@ -1392,17 +1293,69 @@ nsNavHistory::MigrateV6Up(mozIStorageCon
>   // see bug #386303 for more details
>   rv = aDBConn->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING("DROP INDEX IF EXISTS moz_favicons_url"));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = aDBConn->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING("DROP INDEX IF EXISTS moz_anno_attributes_nameindex"));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  return NS_OK;
>+  return transaction.Commit();
>+}
>+
>+// nsNavHistory::MigrateV7Up
>+nsresult
>+nsNavHistory::MigrateV7Up(mozIStorageConnection* aDBConn) 
>+{
>+  mozStorageTransaction transaction(aDBConn, PR_FALSE);
>+  nsresult rv;
>+
>+  // We need to create two triggers on moz_historyvists to maintain the
>+  // accuracy of moz_places.visit_count.  For this to work, we must ensure that
>+  // all moz_places.visit_count values are correct.
>+  // See bug 416313 for details.
>+  {
>+    // First, we do a one-time reset of all the moz_places.visit_count values.
>+    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "UPDATE moz_places SET visit_count = "
>+        "(SELECT count(*) FROM moz_historyvisits "
>+         "WHERE place_id = moz_places.id "
>+         "AND visit_type NOT IN (0, 4, 7))") /* invalid, EMBED, DOWNLOAD */
>+    );
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Now we create our two triggers
>+    rv = aDBConn->ExecuteSimpleSQL(CREATE_VISIT_COUNT_INSERT_TRIGGER);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = aDBConn->ExecuteSimpleSQL(CREATE_VISIT_COUNT_DELETE_TRIGGER);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  // We need to create one trigger on moz_bookmarks to remove unused keywords.
>+  // See bug 421180 for details.
>+  {
>+    // First, remove any existing dangling keywords
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "DELETE FROM moz_keywords "
>+      "WHERE id IN ("
>+        "SELECT k.id "
>+        "FROM moz_keywords k "
>+        "LEFT OUTER JOIN moz_bookmarks b "
>+        "ON b.keyword_id = k.id "
>+        "WHERE b.id IS NULL"
>+      ")")
>+    );
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Now we create our trigger
>+    rv = aDBConn->ExecuteSimpleSQL(CREATE_KEYWORD_VALIDITY_TRIGGER);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  return transaction.Commit();
> }

per my previous review: you need to leave the trigger-detection code in place, otherwise already-trigger-fied databases will do the visit-count and keyword migrations again.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

not sure how I missed that comment last time.

This restores the trigger check during migration.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.3.1

it helps to save the file before generating the diff :(

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 329528 [details] [diff] [review]
v1.3.1


>+// nsNavHistory::MigrateV7Up
>+nsresult
>+nsNavHistory::MigrateV7Up(mozIStorageConnection* aDBConn) 
>+{
>+  mozStorageTransaction transaction(aDBConn, PR_FALSE);
>+
>+  // Create a statement to test for trigger creation
>+  nsCOMPtr<mozIStorageStatement> triggerDetection;
>+  nsresult rv = aDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    "SELECT name "
>+    "FROM sqlite_master "
>+    "WHERE type = 'trigger' "
>+    "AND name = ?"
>+  ), getter_AddRefs(triggerDetection));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Check for exisitance

s/exisitance/existence/

>+  PRBool triggerExists;
>+  rv = triggerDetection->BindUTF8StringParameter(
>+    0, NS_LITERAL_CSTRING("moz_historyvisits_afterinsert_v1_trigger")
>+  );
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = triggerDetection->ExecuteStep(&triggerExists);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = triggerDetection->Reset();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // We need to create two triggers on moz_historyvists to maintain the
>+  // accuracy of moz_places.visit_count.  For this to work, we must ensure that
>+  // all moz_places.visit_count values are correct.
>+  // See bug 416313 for details.
>+  if (!triggerExists) {
>+    // First, we do a one-time reset of all the moz_places.visit_count values.
>+    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "UPDATE moz_places SET visit_count = "
>+        "(SELECT count(*) FROM moz_historyvisits "
>+         "WHERE place_id = moz_places.id "
>+         "AND visit_type NOT IN (0, 4, 7))") /* invalid, EMBED, DOWNLOAD */
>+    );
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Now we create our two triggers
>+    rv = aDBConn->ExecuteSimpleSQL(CREATE_VISIT_COUNT_INSERT_TRIGGER);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = aDBConn->ExecuteSimpleSQL(CREATE_VISIT_COUNT_DELETE_TRIGGER);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  // Check for exisitance

ditto

r=me

Comment 12

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/438cea3432bf

Comment 13

[Shawn Wilsher :sdwilsh]

And addressed the spelling mistake:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/cba48512577a

Comment 14

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv