1017502 - Add a foreign_count column to moz_places

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

We want to add a foreign_count colum to moz_places that will track whether there is a non-history consumer keeping a reference to the given place.
It will work more or less like a classical refcounting, when it's 0 means there is only volatile history, while number counts number of references.
References we expect so far are Bookmarks or Tags (currently both are the same but this is going to change), in future Descriptors may be added to this list.

This will be useful to allow tagging any uri without having to create a bookmark for it.

1. we need a migration function in Database.cpp that on upgrade sets the count to the right value
2. we need a maintenance task in PlacesDBUtils that corrects wrong counts
3. there are some consumers who currently do manual isBookmarked queries (or subqueries), would be very nice and welcome to make them use this new column instead (we may avoid some joins with moz_bookmarks and make the queries faster). We'll have to search for them, but should be enough to filter all queries on moz_bookmarks and see where it makes sense to change.

Comment 1

[Marco Bonardo [:mak]]

4. we need to change nsPlacesExpiration so that it won't expire counted entries

Comment 2

[Althaf Hameez [:ahameez]]

Attachment: bug1017502_foreign_count_moz_places.diff feedback v1

Just looking for some early feedback before I proceed on to the next parts.

::: toolkit/components/places/Database.cpp
@@ -731,16 +731,21 @@
> // Firefox 25 uses schema version 24.

Not sure what's the correct comment that should go in here, so that's just a placeholder.

::: toolkit/components/places/nsPlacesTables.h
> #define MOZ_PLACES_COLUMNS \
>   "id, url, title, rev_host, visit_count, hidden, typed, favicon_id, " \
> -  "frecency, last_visit_date"
> +  "frecency, last_visit_date, guid, foreign_count"

For some reason guid wasn't in MOZ_PLACES_COLUMNS was this missed out from before somehow or is it intentionally not there?


For setting the counts to the right values on upgrade would it be preferred in a separate function like Database::CheckAndUpdateGUIDs() or just done within the migration function itself?

Comment 3

[Althaf Hameez [:ahameez]]

also should we create an index for foreign_count ?

Comment 4

[Marco Bonardo [:mak]]

(In reply to Althaf Hameez [:ahameez] from comment #2)
> Created attachment 8436307 [details] [diff] [review]
> bug1017502_foreign_count_moz_places.diff feedback v1
> 
> Just looking for some early feedback before I proceed on to the next parts.
> 
> ::: toolkit/components/places/Database.cpp
> @@ -731,16 +731,21 @@
> > // Firefox 25 uses schema version 24.
> 
> Not sure what's the correct comment that should go in here, so that's just a
> placeholder.

it should be Firefox 33 (Nightly is 32 right now but will move to 33 along this week iirc)

> ::: toolkit/components/places/nsPlacesTables.h
> > #define MOZ_PLACES_COLUMNS \
> >   "id, url, title, rev_host, visit_count, hidden, typed, favicon_id, " \
> > -  "frecency, last_visit_date"
> > +  "frecency, last_visit_date, guid, foreign_count"
> 
> For some reason guid wasn't in MOZ_PLACES_COLUMNS was this missed out from
> before somehow or is it intentionally not there?

Looks like this is completely unused, we probably leftover it, please remove.

> For setting the counts to the right values on upgrade would it be preferred
> in a separate function like Database::CheckAndUpdateGUIDs() or just done
> within the migration function itself?

If the migrator function is not too large it's fine to keep everything into it, we used separate functions only for very complex migrations.

Comment 5

[Althaf Hameez [:ahameez]]

Attachment: bug1017502_foreign_count_moz_places.diff feedback v2

Updated patch for further feedback

Includes
1) Migration function - which I think has a mistake
2) Maintenance Task to fix wrong counts
3) adjusted nsPlacesExpiration
4) Triggers on addition and removal of bookmarks to adjust foreign_count
5) foreign_count index (if required)

Still to be done
1) Automated Tests on foreign_count
2) Adjusting manual isbookmarked queries

So I ran all the automated tests in toolkit/components/places/tests and I had 6 failures of which 5 were tests found in migration. I also couldn't manually verify is the migration works so I think there is something wrong in my migration code.

In the maintenance task there are 2 maintenance tasks t fix foreign_counts. I think there must be a better way than this

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8437490 [details] [diff] [review]
bug1017502_foreign_count_moz_places.diff feedback v2

Review of attachment 8437490 [details] [diff] [review]:
-----------------------------------------------------------------

yes, apart from missing pieces this one looks good already!

::: toolkit/components/places/Database.cpp
@@ +735,5 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
>  
>        // Firefox 24 uses schema version 23.
> +      if (currentSchemaVersion < 24) {

newline before

@@ +1940,5 @@
> +      "ALTER TABLE moz_places ADD COLUMN foreign_count INTEGER DEFAULT 0 NOT NULL"));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +  // Add index
> +  rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_PLACES_FOREIGNCOUNT);

ok, regarding the index,it depends on the queries we run against this column.
For the queries I see so far it's not needed (also cause a single query can only use one index, and most of the queries you changed already use one).

I'd say to leave this in for now, when we are closer to the final patch, we'll check which queries make use of it and decide on an index. options may be:
1. no index at all
2. the current index
3. a compound index (with some other column) to cover a frequently used query doing stull like foreign_count = X AND some_other_column = Y

@@ +1941,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +  // Add index
> +  rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_PLACES_FOREIGNCOUNT);
> +  NS_ENSURE_SUCCESS(rv, rv);

IIRC it's faster to create the index after having filled up values, so I'd move this to the end of the migrating function for now.

@@ +1946,5 @@
> +
> +  // Adjust counts for all the rows
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT h.id FROM moz_places h"
> +    "INNER JOIN moz_bookmarks b"

we just use JOIN or LEFT JOIN, we usually don't need to be more verbose than that.

@@ +1967,5 @@
> +    rv = updateStmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), itemId);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = updateStmt->Execute();
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

So, there's an issue here, we should set the foreign_count to the number of bookmarks a given uri entry has, not just to 1. it will work more or less like refcounting.

I think you can coalesce these 2 queries into a single update

UPDATE moz_places SET foreign_count = 
(SELECT count(*) FROM moz_bookmarks WHERE fk = moz_places.id)

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +698,5 @@
> +      "WHERE id NOT IN ( " +
> +        "SELECT h.id FROM moz_places h " +
> +        "INNER JOIN moz_bookmarks ON fk = h.id " +
> +        ")");
> +    cleanupStatements.push(fixForeignCountSet);

jusr re-run the update query we have in the migration

::: toolkit/components/places/nsPlacesTriggers.h
@@ +179,5 @@
> +  "CREATE TEMP TRIGGER moz_places_foreign_count_afterdelete_trigger " \
> +  "AFTER DELETE ON moz_bookmarks FOR EACH ROW " \
> +  "BEGIN " \
> +    "UPDATE moz_places " \
> +    "SET foreign_count = 0 " \

we can't do this, cause it may have 2 bookmarks, and after a single removal we'd set it to 0 instead of one.
instead you should decrement it (IIRC you can just set foreign_count = foreign_count - 1)

@@ +190,5 @@
> +  "AFTER INSERT ON moz_bookmarks FOR EACH ROW " \
> +  "BEGIN " \
> +    "UPDATE moz_places " \
> +    "SET foreign_count = 1 " \
> +    "WHERE id = NEW.fk;" \

and you should increment here

Comment 7

[Althaf Hameez [:ahameez]]

Attachment: bug1017502_foreign_count_moz_places.diff feedback v3

Updated with automated test, testing the triggers and maintenance task.

Currently 1 failing test, which is > unit/test_hosts_triggers.js
Failing on test_bookmark_changes line 118 
Not too sure what's going wrong here.

Now I'll look into the manual isBookmarked queries :)

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8440298 [details] [diff] [review]
bug1017502_foreign_count_moz_places.diff feedback v3

Review of attachment 8440298 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what's up with test_hosts_triggers.js, do you see any warnings while the test runs? The only thing I may think of is that your trigger is updating moz_places, and that causes us to activate other triggers, but that shouldn't happen cause the other triggers should activate only for frecency or typed changes.
So, no ideas off-hand.
you may use dump_table("table_name"); to print out the sqlite table in the test output and check what's up.

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +684,5 @@
>  
> +    // L.4 recalculate foreign_count.
> +    let fixForeignCount = DBConn.createAsyncStatement(
> +      "UPDATE moz_places SET foreign_count = " +
> +      "( SELECT count(*) FROM moz_bookmarks WHERE fk = moz_places.id ) " );

nit": remove whitespace before 'SELECT' and after ')'

::: toolkit/components/places/nsPlacesTriggers.h
@@ +175,5 @@
>    "END" \
>  )
>  
> +#define CREATE_FOREIGNCOUNT_AFTERDELETE_TRIGGER NS_LITERAL_CSTRING( \
> +  "CREATE TEMP TRIGGER moz_places_foreign_count_afterdelete_trigger " \

the triggers are named after the table they listen to, so in this case moz_bookmarks_foreign_count...

@@ +185,5 @@
> +  "END" \
> +)
> +
> +#define CREATE_FOREIGNCOUNT_AFTERINSERT_TRIGGER NS_LITERAL_CSTRING( \
> +  "CREATE TEMP TRIGGER moz_places_foreign_count_afterinsert_trigger " \

ditto

::: toolkit/components/places/tests/bookmarks/test_1017502-bookmarks_foreign_count.js
@@ +13,5 @@
> +  run_next_test();
> +}
> +
> +T_URL = "https://www.mozilla.org/firefox/nightly/firstrun/";
> +T_URI = NetUtil.newURI(T_URL);

please "const" these

@@ +36,5 @@
> +  let id2 = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.bookmarksMenuFolderId,
> +                      T_URI, PlacesUtils.bookmarks.DEFAULT_INDEX, "First Run");
> +  rows = yield conn.executeCached(
> +    "SELECT foreign_count FROM moz_places WHERE url = :t_url ", { t_url: T_URL });
> +  Assert.equal(rows[0].getResultByName("foreign_count"), 2);

may you please move this to an helper like foreignCountForURI and use the helper all around? (fwiw you may use URI.spec and avoid the other const, but whatever, not important)

@@ +45,5 @@
> +    "SELECT foreign_count FROM moz_places WHERE url = :t_url ", { t_url: T_URL });
> +  Assert.equal(rows[0].getResultByName("foreign_count"), 1);
> +
> +  // Cleanup - Remove 1st bookmark
> +  PlacesUtils.bookmarks.removeItem(id1);

please also check we go back to zero here.

Also, even if currently it's the same, I'd like the test to also verify we manage the counter for Tags. in future the tags backend will change and the test should fetch eventual issues in counting tags.

A separate test doing similar to this one but using tags API would be fine.

@@ +60,5 @@
> +  let stmt = DBConn().createStatement(
> +    "UPDATE moz_places SET foreign_count = 10 WHERE url = :t_url ");
> +  stmt.params.t_url = T_URL;
> +  stmt.executeStep();
> +  stmt.finalize();

please use Sqlite.jsm API here as well, like you did in the rest of the test.

Comment 9

[Althaf Hameez [:ahameez]]

Thanks for the feedback.

On using Sqlite.jsm on the UPDATE statement this is what I've been trying to do but it always gives a statement exception when running the test. Not sure what I'm doing wrong here 

> yield conn.execute("UPDATE moz_places SET foreign_count = 10 WHERE url = :t_url ", { t_url: T_URL });
gives the error
> Unexpected exception Error: Error(s) encountered during statement execution. at resource://gre/modules/Sqlite.jsm:596 - See following stack:

Secondly for the helper method, I'm using a function like this
> function checkForeignCount(conn, num){
>   let rows = yield conn.executeCached(
>     "SELECT foreign_count FROM moz_places WHERE url = :t_url ", { t_url: T_URL });
>   Assert.equal(rows[0].getResultByName("foreign_count"), num);
> }

and therefore called as > yield checkForeignCount(conn, 0);
Is that OK?

Finally with regards to test_hosts_trigger failing test, I did a dump table and the results are below.

3 table dumps were taken. After the insertion of the bookmark, after changeBookmarkURI and after promiseClearHistory

BEFORE PATCH

 0:13.00 id host  frecency  typed prefix  
 0:13.00 1  test.mozilla.org  140 0 NULL  
 0:13.00 *** There were a total of 1 rows of data.

 0:13.14 id host  frecency  typed prefix  
 0:13.14 1  test.mozilla.org  0 0 NULL  
 0:13.14 2  different.mozilla.org 140 0 NULL  
 0:13.14 *** There were a total of 2 rows of data.

 0:13.29 *** Printing data from moz_hosts
 0:13.29 id host  frecency  typed prefix  
 0:13.29 2  different.mozilla.org 140 0 NULL  
 0:13.29 *** There were a total of 1 rows of data.

AFTER PATCH

 0:12.82 id host  frecency  typed prefix  
 0:12.82 1  test.mozilla.org  140 0 NULL  
 0:12.82 *** There were a total of 1 rows of data.

 0:12.97 id host  frecency  typed prefix  
 0:12.97 1  test.mozilla.org  0 0 NULL  
 0:12.97 2  different.mozilla.org 140 0 NULL  
 0:12.97 *** There were a total of 2 rows of data.

 0:13.08 *** Printing data from moz_hosts
 0:13.08 id host  frecency  typed prefix  
 0:13.08 1  test.mozilla.org  0 0 NULL  
 0:13.08 *** There were a total of 1 rows of data.

As can be seen clearly promiseClearHistory removed test.mozilla.org but now is removing different.mozilla.org which is causing the error. Still not too sure what is exactly causing it but will look further.

Will also begin working on the test for tags as well.

Comment 10

[Marco Bonardo [:mak]]

Replied on IRC, the helper is fine, even if maybe you could make it slightly "better" by having it just return the value and keep the check outside, like

Assert.equal(yield getForeignCountForPage(url, conn), value);

the test would be a little bit more explicit this way.
The trigger test issue is solved (will need a dedicated test for changeBookmarkURI)
for the update failure you might print out the error message you get (it's mozIStorageError)

Comment 11

[Marco Bonardo [:mak]]

for promiseDBConnection() returning a read-only connection... I see.

Please keep using DBConn() as in the current patch but with the async API, don't use createStatement and executeStep. We should stop using the sync API.

Comment 12

[Althaf Hameez [:ahameez]]

Attachment: bug1017502_foreign_count_moz_places.diff v4

Updated patch with requested changes.

The only change that I could not implement was to make the helper return the value and keep the check outside. It would complain about a generator function attempting to return a value. 
Everything else is OK

all places tests passed on my local build.

Comment 13

[Althaf Hameez [:ahameez]]

Attachment: bug1017502_foreign_count_moz_places.diff v5

Figured out how to return values from generator functions.

Comment 14

[Althaf Hameez [:ahameez]]

So I forgot to rename the helper function. checkForeignCountForURL should become getForeignCountForURL
I'll change it following feedback from you on the rest of the patch.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8454874 [details] [diff] [review]
bug1017502_foreign_count_moz_places.diff v5

Review of attachment 8454874 [details] [diff] [review]:
-----------------------------------------------------------------

Just to be clear, on Tuesday there's the 33->34 merge, since this is a schema change I'd say to wait for the merge before landing.

::: toolkit/components/places/Database.cpp
@@ +742,5 @@
> +      if (currentSchemaVersion < 24) {
> +        rv = MigrateV24Up();
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +      // Firefox 33 uses schema version 24.

should become 34

@@ +1958,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Add index
> +  rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_PLACES_FOREIGNCOUNT);
> +  NS_ENSURE_SUCCESS(rv, rv);

ok, based on the patch this index is not yet useful, we might introduce it in future, but please remove it for now.

::: toolkit/components/places/nsPlacesTriggers.h
@@ +196,5 @@
> +)
> +
> +#define CREATE_FOREIGNCOUNT_AFTERUPDATE_TRIGGER NS_LITERAL_CSTRING( \
> +  "CREATE TEMP TRIGGER moz_bookmarks_foreign_count_afterupdate_trigger " \
> +  "AFTER UPDATE ON moz_bookmarks FOR EACH ROW " \

this one should be limited to fk changes, it should not trigger on other changes.

so AFTER UPDATE OF fk ON moz_bookmarks...

::: toolkit/components/places/tests/bookmarks/test_1017502-bookmarks_foreign_count.js
@@ +11,5 @@
> +
> +const T_URI = NetUtil.newURI("https://www.mozilla.org/firefox/nightly/firstrun/");
> +
> +function checkForeignCountForURL(conn, url){
> +  let url = url || T_URI.spec

this should rather be let url = url instanceof Ci.nsIURI ? url.spec : url; and you should pass the nsIURI into

@@ +15,5 @@
> +  let url = url || T_URI.spec
> +  let rows = yield conn.executeCached(
> +      "SELECT foreign_count FROM moz_places WHERE url = :t_url ", { t_url: url });
> +  if (rows.length == 0)
> +    throw new Task.Result(0);

instead of this, use a generator "function*" so you can just return from it.

function* somefunc() {
  yield something;
  return something_else;
}

throw new Task.Result is the old method used before we had generator functions

@@ +17,5 @@
> +      "SELECT foreign_count FROM moz_places WHERE url = :t_url ", { t_url: url });
> +  if (rows.length == 0)
> +    throw new Task.Result(0);
> +  else
> +    throw new Task.Result(rows[0].getResultByName("foreign_count"));

How can this select return nothing or something? I would expect no rows to be an error, since it would mean the page doesn't exist in moz_places, not that foreign_count is zero

@@ +29,5 @@
> +  let conn = yield PlacesUtils.promiseDBConnection();
> +
> +  // Simulate a visit to the url
> +  yield promiseAddVisits(T_URI);
> +  Assert.equal((yield checkForeignCountForURL(conn)), 0);

pass T_URI

@@ +34,5 @@
> +
> +  // Add 1st bookmark which should increment foreign_count by 1
> +  let id1 = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
> +                    T_URI, PlacesUtils.bookmarks.DEFAULT_INDEX, "First Run");
> +  Assert.equal((yield checkForeignCountForURL(conn)), 1);

ditto (not repeating further)

@@ +46,5 @@
> +  PlacesUtils.bookmarks.removeItem(id2);
> +  Assert.equal((yield checkForeignCountForURL(conn)), 1);
> +
> +  // Change first bookmark's URI
> +  let URI2 = NetUtil.newURI("http://www.mozilla.org");

nit: const

@@ +51,5 @@
> +  PlacesUtils.bookmarks.changeBookmarkURI(id1, URI2);
> +  // Check foreign count for original URI
> +  Assert.equal((yield checkForeignCountForURL(conn)), 0);
> +  // Check foreign count for new URI
> +  Assert.equal((yield checkForeignCountForURL(conn, URI2.spec)), 1);

once the function supports nsIURI there's no more need to pass the spec

Comment 16

[Marco Bonardo [:mak]]

Regarding the async update problem, you must wait for its completion, so register a mozIStorageStatementCallback when you call asyncExecute, and wait for it (through a deferred)

Comment 17

[Althaf Hameez [:ahameez]]

Attachment: bug1017502_foreign_count_moz_places.diff v6

With all required fixes. 

Currently put a try run at: https://tbpl.mozilla.org/?tree=Try&rev=e08d3ee22e0b

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8464722 [details] [diff] [review]
bug1017502_foreign_count_moz_places.diff v6

Review of attachment 8464722 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good, thank you!

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/a8708a05018e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8708a05018e