Closed Bug 443068 Opened 12 years ago Closed 12 years ago

Move triggers into migration code

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1a2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Details

Attachments

(1 file, 5 obsolete files)

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).
Attached patch v0.1 (obsolete) — Splinter Review
This fails test_savedsearches.js, oddly enough.  I plan on looking into that more tomorrow.
Attached patch v1.0 (obsolete) — Splinter Review
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).
Attachment #327726 - Attachment is obsolete: true
Attachment #327857 - Flags: review?(dietrich)
Whiteboard: [has patch][needs review dietrich]
Attached patch v1.1 (obsolete) — Splinter Review
Renaming the constants from PLACES_ to CREATE_ to be more inline with bug 443433's patch.
Attachment #327857 - Attachment is obsolete: true
Attachment #327973 - Flags: review?(dietrich)
Attachment #327857 - Flags: review?(dietrich)
Comment on attachment 327973 [details] [diff] [review]
v1.1

r=me. please test migration of <=v5, if you have not already done so.
Attachment #327973 - Flags: review?(dietrich) → review+
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.
Attachment #327973 - Flags: review+ → review-
I think it's worthwhile for code clarity, and the less time spent during startup is just a bonus.
Whiteboard: [has patch][needs review dietrich] → [needs new patch]
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #327973 - Attachment is obsolete: true
Attachment #329507 - Flags: review?(dietrich)
Whiteboard: [needs new patch] → [has patch][needs review 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.
Attachment #329507 - Flags: review?(dietrich) → review-
Attached patch v1.3 (obsolete) — Splinter Review
not sure how I missed that comment last time.

This restores the trigger check during migration.
Attachment #329507 - Attachment is obsolete: true
Attached patch v1.3.1Splinter Review
it helps to save the file before generating the diff :(
Attachment #329527 - Attachment is obsolete: true
Attachment #329528 - Flags: review?(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
Attachment #329528 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][needs review dietrich] → [has patch]
Whiteboard: [has patch] → [has patch][has review][can land]
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/438cea3432bf
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
Target Milestone: --- → Firefox 3.1a2
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.