Closed Bug 450290 Opened 12 years ago Closed 11 years ago

Sync the temp tables to the permanent tables

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 12 obsolete files)

I'm thinking of two functions:
1) sync moz_places
2) sync moz_historyvisits

(1) should be done whenever we add a bookmark, and before moz_historyvisits is sync'd.  Probably should be done with a bookmark observer (or done in C++ when the bookmark is added).
(2) should be done every X time (TBD)

We need tests for the following:
(1) Ensuring we sync after bookmark add
?
Blocks: 437244
No longer blocks: 437244
Depends on: 429988
No longer depends on: 429998
Blocks: 442967
Attached patch v0.1 (obsolete) — Splinter Review
Initial implementation - completely untested.  Asking for review to make sure that this approach seems sound.
Attachment #334363 - Flags: review?(dietrich)
(In reply to comment #0)
> (2) should be done every X time (TBD)

if we had a pref to control the interval, testing this would be reasonably do-able as well

Comment on attachment 334363 [details] [diff] [review]
v0.1

i'd prefer nsPlacesDBFlush or something like that. "sync" is an ambiguous and overloaded term. also, maybe nsP* to vaguely indicate that it's private.
Comment on attachment 334363 [details] [diff] [review]
v0.1

yeah, approach looks fine.
Attachment #334363 - Flags: review?(dietrich)
Depends on: 451244
Attached patch v0.2 (obsolete) — Splinter Review
This has one test that just adds a bookmark and ensures a sync happened.  I had to post an extra empty event the the background thread to get the test to pass, which I don't full understand.  I also trigger an assertion in the JS engine, which I will file a bug for shortly.
Attachment #334363 - Attachment is obsolete: true
Depends on: 451299
Depends on: 451463
Attached patch v0.3 (obsolete) — Splinter Review
We just need one more test that ensures that we sync historyvisits.  That one is arguably harder since it uses timers, but hey :)
Attachment #334602 - Attachment is obsolete: true
Depends on: 451590
Attached patch v1.0 (obsolete) — Splinter Review
Full test coverage now.  Yay!
Attachment #334807 - Attachment is obsolete: true
Attachment #334927 - Flags: review?(dietrich)
Whiteboard: [has patch][needs review dietrich]
Comment on attachment 334927 [details] [diff] [review]
v1.0


>+//// Constants
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;
>+
>+const kQuitApplication = "quit-application";
>+
>+const kSyncPrefName = "syncDBTableInterval";

please append "InSecs" or something like that so that it is self-documenting.

>+  try {
>+    // We want to silently fail if the preference does not exist, and use a
>+    // default to fallback to.
>+    this._syncInterval = this._prefs.getIntPref(kSyncPrefName);
>+  }
>+  catch (e) {
>+    // The preference did not exist, so default to two minutes.
>+    this._syncInterval = 120;
>+  }

make the default a const

>+    else if (aTopic == "nsPref:changed" && aData == kSyncPrefName) {
>+      // Get the new pref value, and then update our timer
>+      this._syncInterval = aSubject.getIntPref(kSyncPrefName);
>+

here and above, should validate the pref value is a usable number

//////////////////////////////////////////////////////////////////////////////
>+  //// nsISupports
>+
>+  classDescription: "Used to synchronize the temporary and permanent tables of Places",
>+  classID: Components.ID("c1751cfc-e8f1-4ade-b0bb-f74edfb8ef6a"),
>+  contractID: "@mozilla.org/places/sync;1",
>+  _xpcom_categories: [{
>+    category: "app-startup",
>+    service: true
>+  }],

the tryserver build will tell us how hard this hits Ts. if it's bad, we'll need to initiate it later, likely in delayedStartup.

also, this file needs to be added to packages-static.
Attachment #334927 - Flags: review?(dietrich) → review-
(In reply to comment #8)
> here and above, should validate the pref value is a usable number
Er, how could it ever not be a usable number?  I'm not sure what I'd be validating against here...

> the tryserver build will tell us how hard this hits Ts. if it's bad, we'll need
> to initiate it later, likely in delayedStartup.
> 
> also, this file needs to be added to packages-static.
You can't start a service with the category manager in delayed startup, AFAIK.  Besides, the only thing we do at startup is register a bookmark observer, which will init history, which is already in the Ts hotpath.  The later we wait, the more likely we are to miss a new bookmark being added, and opens up a period of time for us to not have a database that has the proper foreign key requirements.
Whiteboard: [has patch][needs review dietrich] → [needs new patch]
(In reply to comment #9)
> (In reply to comment #8)
> > here and above, should validate the pref value is a usable number
> Er, how could it ever not be a usable number?  I'm not sure what I'd be
> validating against here...
Nevermind - negative or zero would be bad.
Attached patch v1.1 (obsolete) — Splinter Review
Addresses review comments.
Attachment #334927 - Attachment is obsolete: true
Attachment #334953 - Flags: review?(dietrich)
Whiteboard: [needs new patch] → [has patch][needs review dietrich]
Comment on attachment 334953 [details] [diff] [review]
v1.1


>+  /**
>+   * Synchronizes the moz_{aName} and moz_{aName}_temp by copying all the data
>+   * from the temporary table into the permanent one.  It then deletes the data
>+   * in the temporary table.  All of this is done in a transaction that is
>+   * rolled back upon failure at any point.
>+   */
>+  _doSyncMozX: function PlacesBackground_doSyncX(aName)

nit: s/doSyncX/doSyncMozX/

>+  {
>+    // We try to get a transaction, but if we can't don't worry
>+    let ourTransaction = false;
>+    try {
>+      this._db.beginTransaction();
>+      ourTransaction = true;
>+    }
>+    catch (e) { }

why would we not be able to get a transaction? do we want to do this w/o one, since at one point some ids will be duplicated in both tables? also, should we use TRANSACTION_IMMEDIATE here to be safe?
(In reply to comment #12)
> why would we not be able to get a transaction? do we want to do this w/o one,
> since at one point some ids will be duplicated in both tables? also, should we
> use TRANSACTION_IMMEDIATE here to be safe?
There could already be one open (as in the case when we sync both tables), or sqlite could return busy (we should arguably retry in that case).  I'm also not so sure on what happens with two different threads and transactions.  I've sent a question to sqlite-users to get a clarification so I can figure out exactly what we should be doing in this situation:
http://www.mail-archive.com/sqlite-users@sqlite.org/msg36424.html
So here's the deal.  Transactions are per connection, so we have a few options here:
1) wait in while until we do not have an open transaction, try to get one (we'd still have to wrap in a try catch because someone else on another thread could open one up on us)
2) Continue to do what we do and try to get the transaction, but basically say "oh well" if we cannot get it

The problem with those two is that we can end up executing the first query in one of those, and then a transaction closes, and then we execute the second one, which would leave us in an inconsistent state (ugh).  On top of that, someone could come and add something to one of the temporary tables after we INSERT OR REPLACE, but before we DELETE from, so at that point, the data is lost completely :(.  As a result, I think both of those options are off the table.

So, what we really need is some way to do a few operations on the database atomically and make sure that *no* other consumers can read or write from the database on other threads during this time.  I can't think of a solution immediately to that problem though :(
Whiteboard: [has patch][needs review dietrich] → [needs new patch]
I'll note that this would likely be a much easier problem to solve if sqlite supported nested named transactions, but I don't think they plan on adding that any time soon.
how about a delete trigger on the temp table, that inserts that row into the permanent table? that way, deleting everything from the temp table in one query would also insert it all into the permanent one in a single step.
I think that approach has merit.  As far as I can tell, that would make the each row's deletion atomic.  Hurray!

I'll cook up a patch for that momentarily.
Attached patch v1.2 (obsolete) — Splinter Review
OK.  That suggestion has been implemented, and it passes the tests (I had the trigger wrong at first, and it failed the tests spectacularly!).
Attachment #334953 - Attachment is obsolete: true
Attachment #334983 - Flags: review?(dietrich)
Attachment #334953 - Flags: review?(dietrich)
Whiteboard: [needs new patch] → [has patch][needs review dietrich]
Comment on attachment 334983 [details] [diff] [review]
v1.2

>+  //// nsINavBookmarkObserver
>+
>+  onBeginUpdateBatch: function() this._inBatchMode = true,
>+  onEndUpdateBatch: function() this._inBatchMode = false,

i think we might want to pay attention to batch state, instead of just blindly syncing every time an item is added. having to sync between each item could severely slow down first-run upgraders from Fx2, as well as bookmark import and restore.
Comment on attachment 334983 [details] [diff] [review]
v1.2

another question: a single write query is implicitly a transaction. given that, is the explicit transaction code in _doSyncMozX() necessary?
(In reply to comment #19)
> i think we might want to pay attention to batch state, instead of just blindly
> syncing every time an item is added. having to sync between each item could
> severely slow down first-run upgraders from Fx2, as well as bookmark import and
> restore.
Ugh - I was worrying about state, but not using it when syncing.  CRY!
OK, that's an easy fix, and I'll make a test for that too.

(In reply to comment #20)
> (From update of attachment 334983 [details] [diff] [review])
> another question: a single write query is implicitly a transaction. given that,
> is the explicit transaction code in _doSyncMozX() necessary?
It might not be, but I'm not 100% how triggers work there with regards to the implicit transactions.  I'd say it doesn't hurt since we want that to be atomic.
Whiteboard: [has patch][needs review dietrich] → [has patch]
Comment on attachment 334983 [details] [diff] [review]
v1.2

per previous comments
Attachment #334983 - Flags: review?(dietrich)
not to self - sync before shutdown :(
Attached patch v1.3 (obsolete) — Splinter Review
This is another iteration, but I'm not requesting review yet.  What this does:
Properly handles batching.
Cancels the timer during batch mode so it doesn't update during a batch.
Adds tests to test that adding a visit or a bookmark does not get synchronized until batch mode is completed

What still needs to be done:
Have the timer dispatched to the background thread so it doesn't do anything on the main thread.
Test that the timer does not run during batch mode (this test is going to be a bit crazy, but it's doable).
Flush on quit.  This is a bit harder, and may mean that I am going to add an observer topic that will be dispatched just before we close down the background thread so we know to post any last events to it.

I fully expect to get 100% test coverage on all of our behaviors added in this bug.
Attachment #334983 - Attachment is obsolete: true
Blocks: 451781
> Have the timer dispatched to the background thread so it doesn't do anything on
> the main thread.
Turns out this will trigger an assertion (one that we shouldn't be triggering), so I'm punting this to bug 451781.  It's not terribly important anyway.

> Test that the timer does not run during batch mode (this test is going to be a
> bit crazy, but it's doable).
Consequentially, this isn't important now either.
(In reply to comment #24)
> Flush on quit.  This is a bit harder, and may mean that I am going to add an
> observer topic that will be dispatched just before we close down the background
> thread so we know to post any last events to it.
We actually do this now, but not in a way that I consider safe.  I'm going to add an observer topic for places thread shutdown, and add a test to make sure we sync.
Attached patch v1.4 (obsolete) — Splinter Review
Addresses review comments, and accomplishes everything I said I needed to still.
Attachment #335103 - Attachment is obsolete: true
Attachment #335121 - Flags: review?(dietrich)
Whiteboard: [has patch] → [has patch][needs review dietrich]
Attached patch v1.5 (obsolete) — Splinter Review
(In reply to comment #20)
> another question: a single write query is implicitly a transaction. given that,
> is the explicit transaction code in _doSyncMozX() necessary?
I just verified that this is in fact the case.  That transaction has been removed.
Attachment #335121 - Attachment is obsolete: true
Attachment #335126 - Flags: review?(dietrich)
Attachment #335121 - Flags: review?(dietrich)
Comment on attachment 335126 [details] [diff] [review]
v1.5

looks good, r=me
Attachment #335126 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][needs review dietrich] → [has patch][has review]
No longer depends on: 451299
Flags: in-testsuite?
Flags: in-litmus?
Blocks: 451858
Depends on: 452486
Attached patch v1.6 (obsolete) — Splinter Review
This was actually being created to early in real builds, and we ended up not having a profile directory.  We now get created during profile-after-change, so we have a profile, and everything is AOK to use.  It's a small change, and I factored out a small bit of code into the head_background.js too, so I'm not requesting a new review.
Attachment #335126 - Attachment is obsolete: true
Attached patch v1.7 (obsolete) — Splinter Review
Forgot to hg add my last test...
Attachment #335776 - Attachment is obsolete: true
Depends on: 451607
i'm not sure this works fine, i'm using a build with our latest patches in and i see corruption in bookmarks, i don't yet know if that's due to a wrong query or to wrong sync though. i see wrong moz_places ids in my table, for example most visited on the toolbar has fk = 1 but the correct query in the moz_places table has id = 3 while the last site i've visited has got id = 1...
I've got the non-batched and batched code paths covered in this bug with tests to ensure that the sync happens correctly.  If you could get steps to reproduce the corruption, it'd be a big help.
Attached patch v1.8 (obsolete) — Splinter Review
This adds tests to an issue Marco and I found with the insert triggers.
Attachment #335781 - Attachment is obsolete: true
Attachment #338236 - Flags: review?(mak77)
Whiteboard: [has patch][has review] → [has patch][needs review Marco]
(In reply to comment #34)
> Created an attachment (id=338236) [details]
> v1.8
> 
> This adds tests to an issue Marco and I found with the insert triggers.

how can this be litmus tested from user's end?   Please suggest some test areas.
(In reply to comment #35)
> how can this be litmus tested from user's end?   Please suggest some test
> areas.
Normal browsing would have completely hosed the places database after just a short while.  I'm not sure we need litmus tests when we have automated coverage (better to spend human time on things that are hard to test, right?).  Please correct me if I'm wrong.
Comment on attachment 338236 [details] [diff] [review]
v1.8

+/**
+ * This trigger moves the data out of a temporary table into the permanent one
+ * before deleting from the temporary table.
+ */
+#define CREATE_TEMP_SYNC_TRIGGER_BASE(__table) NS_LITERAL_CSTRING( \
+  "CREATE TEMPORARY TRIGGER " __table "_beforedelete_trigger " \
+  "BEFORE DELETE ON " __table "_temp FOR EACH ROW " \
+  "BEGIN " \
+    "INSERT OR REPLACE INTO " __table " " \
+    "SELECT * FROM " __table "_temp " \
+    "WHERE id = OLD.id;" \
+  "END" \
+)

add a comment on the fact that in such a trigger INSERT OR REPLACE does not
create a new id

diff --git a/toolkit/components/places/tests/background/test_database_sync_after_quit_applicaiton.js b/toolkit/components/places/tests/background/test_database_sync_after_quit_applicaiton.js
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/background/test_database_sync_after_quit_applicaiton.js

typo: applicaiton

apart that, r=me
Attachment #338236 - Flags: review?(mak77) → review+
Whiteboard: [has patch][needs review Marco] → [has patch]
Attached patch v1.9Splinter Review
Please don't let there by a v1.10...
Attachment #338236 - Attachment is obsolete: true
Whiteboard: [has patch] → [has patch][has reviews]
i'm not sure if it's only my problem, but actually using patch 1.9 the build did not create head_background.js in toolkit\components\places\tests\background, looking at the patch i see a call to copy from/to, maybe my mozilla-build windows environment is missing some piece
Extremely minor in the global picture, but it seems unnecessary to have a lazy getter for _bh as it is used a few statements later.

+  this.__defineGetter__("_bh", function() {
+    delete this._bh;
+    return this._bh = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+                      getService(Ci.nsINavBookmarksService);
+  });
+

...

+  // Register observers
+  this._bh.addObserver(this, false);
(In reply to comment #40)
> Extremely minor in the global picture, but it seems unnecessary to have a lazy
> getter for _bh as it is used a few statements later.

that code has been replaced in bug 459491
(In reply to comment #41)
> that code has been replaced in bug 459491

Marco, Shawn, Dietrich, the results of the Big Places Optimization Work live in patches filed to several bugs, which makes it a bit hard to the big picture.

Is it possible to publish a link to a Mercurial repo or a Mercurial patchqueue repo with all related patches in a tracking bug like bug 442967?
We've been using a public patch queue all this time:
http://hg.mozilla.org/users/sdwilsh_shawnwilsher.com/storage-async/
http://hg.mozilla.org/mozilla-central/rev/0d1361990ec8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1b2
we have a bunch of automatic tests, and i don't see how a litmus test could be written to check this (would require the user to directly look at sqlite tables and temp tables).
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.