1125113 - Change the keywords schema associating them with uris

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

we need to change the current schema so that keywords are directly associated with uris and they increase foreign_count (so the uri is not expired)

this means changing from multiple uris for one keyword to 1 uri - 1 keyword, that will allow us to simplify management and caching.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

More or less, this is the patch.

The main change is that now we can only have multiple keywords per uri (provided the post data changes), but not multiple uris per keyword.
I could also remove the keywords cache from the bookmarks service, since I figured we were already running a query for the post data, so at that point I just replaced the query.

This means I can now remove keywords support from Bookmarks.jsm and we can use the old keywords APIs until I introduce the new one. The 2 will already be able to work in parallel. So this speeds up the conversion.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8560453 [details] [diff] [review]
patch v1

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

Sorry again for the delay... Can you please post updates to this patch as interdiffs? Thanks :)

::: toolkit/components/places/Database.cpp
@@ +1529,5 @@
> +  // bookmarks uris, or multiple keywords could be associated to the same uri.
> +  // The new system only allows multiple uris per keyword, provided they have
> +  // a different post_data value.
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT OR REPLACE INTO moz_keywords (id, keyword, place_id, post_data) " 

Nit: trailing white space

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +663,5 @@
>      let deleteUnusedKeywords = DBConn.createAsyncStatement(
>        `DELETE FROM moz_keywords WHERE id IN (
>           SELECT id FROM moz_keywords k
>           WHERE NOT EXISTS
> +           (SELECT 1 FROM moz_places h WHERE k.place_id = h.id)

Could we also do this with a trigger? When deleting a row from moz_places, couldn't we also remove all keywords for that place_id?

::: toolkit/components/places/PlacesUtils.jsm
@@ +815,5 @@
> +  setPostDataForBookmark(aBookmarkId, aPostData) {
> +    // For now we don't have a unified API to create a keyword with postData,
> +    // thus here we can just try to complete a keyword that should already exist
> +    // without any post data.
> +    let nullPostDataFragment = aPostData ? "AND post_data ISNULL" : "";

This doesn't quite seem to match the previous behavior as it doesn't allow us updating a bookmark's post data. Without the keyword to update we don't really know which one of the keywords we should update. Instead we seem to use a random matching keyword with post_data=NULL.

This probably confuses me because the old API doesn't at all fit in with the new DB scheme, like you hinted above in the comment. Did you choose to update keywords with post_data=null because we usually only use this function right after creating a new keyword?

@@ +819,5 @@
> +    let nullPostDataFragment = aPostData ? "AND post_data ISNULL" : "";
> +    let stmt = PlacesUtils.history.DBConnection.createStatement(
> +      `UPDATE moz_keywords SET post_data = :post_data
> +       WHERE place_id = (SELECT fk FROM moz_bookmarks WHERE id = :item_id)
> +      ${nullPostDataFragment}`);

With multiple keywords having post_data=null with the same place_id, will this try to update all of them and try to invalidate the (placed_id,post_data) unique constraint and fail? Maybe it doesn't, maybe we should add a LIMIT 1?

@@ +821,5 @@
> +      `UPDATE moz_keywords SET post_data = :post_data
> +       WHERE place_id = (SELECT fk FROM moz_bookmarks WHERE id = :item_id)
> +      ${nullPostDataFragment}`);
> +    stmt.params.item_id = aBookmarkId;
> +    stmt.params.post_data = aPostData;

Is aPostData=null correctly cast to SQL's NULL here? I'm not too familiar with that.

@@ +860,5 @@
>     * @returns an array containing a string URL and a string of POST data
>     */
> +  getURLAndPostDataForKeyword(aKeyword) {
> +    // In case of multiple urls associated to this keyword, we return the most
> +    // recently created keyword.

That comment seems stale? We have multiple keywords per URI only, right? So we'll only ever find a single URL for a given keyword.

@@ +872,5 @@
> +      if (!stmt.executeStep())
> +        return [ null, null ];
> +      return [ stmt.row.url, stmt.row.post_data ];
> +     }
> +     finally {

Nit: the indentation seems off a little.

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +2243,2 @@
>  {
> +  // If there are no keywords for this URI, there's nothing to do. 

Nit: trailing white space

@@ +2274,5 @@
> +  nsTArray<BookmarkData> bookmarks;
> +  rv = GetBookmarksForURI(uri, bookmarks);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (bookmarks.Length() == 0) {
> +    for (uint32_t i = 0; i < keywords.Length(); ++i) {

Nit: int32_t

@@ +2402,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +
> +    bool hasMore;
> +    while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {

We only expect a single result here, no?

@@ +2422,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    for (uint32_t i = 0; i < bookmarks.Length(); ++i) {
> +      NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,

Don't we notify at the bottom of the function already?

@@ +2501,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  bool hasMore;
> +  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {

Maybe if () ?

@@ +2540,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    bool hasMore;
>    while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {

if () ? Or is there a reason we're using while everywhere?

::: toolkit/components/places/tests/bookmarks/test_keywords.js
@@ +25,5 @@
>  }
>  
> +function check_orphans() {
> + let db = yield PlacesUtils.promiseDBConnection();
> +  let rows = yield db.executeCached(

Nit: indentation is off.

@@ +45,4 @@
>  
> +      if (name.startsWith("onItemChanged")) {
> +        return () => {
> +          if (arguments[1] != "keyword")

Maybe declare the argument explicitly?

@@ +59,5 @@
> +      }
> +
> +      if (name in target)
> +        return target[name];
> +      return undefined;

return target[name]?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> ::: toolkit/components/places/PlacesDBUtils.jsm
> @@ +663,5 @@
> >      let deleteUnusedKeywords = DBConn.createAsyncStatement(
> >        `DELETE FROM moz_keywords WHERE id IN (
> >           SELECT id FROM moz_keywords k
> >           WHERE NOT EXISTS
> > +           (SELECT 1 FROM moz_places h WHERE k.place_id = h.id)
> 
> Could we also do this with a trigger? When deleting a row from moz_places,
> couldn't we also remove all keywords for that place_id?

Since keywords increase foreign_count and a page should never be removed unless it has foreign_count to zero, that would not be useful.
PlacesDBUtils exists only to cleanup mess caused by corruption (And soon or later we should rewrite it).

> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +815,5 @@
> > +  setPostDataForBookmark(aBookmarkId, aPostData) {
> > +    // For now we don't have a unified API to create a keyword with postData,
> > +    // thus here we can just try to complete a keyword that should already exist
> > +    // without any post data.
> > +    let nullPostDataFragment = aPostData ? "AND post_data ISNULL" : "";
> 
> This doesn't quite seem to match the previous behavior as it doesn't allow
> us updating a bookmark's post data.

we don't have a ui to update post data. what we do currently is creating a bookmark, creating a keyword and then adding post data, all from a single ui.

Without the keyword to update we don't
> really know which one of the keywords we should update. Instead we seem to
> use a random matching keyword with post_data=NULL.

Yes, we assume we created a keyword without post data, and now we are setting post data for it.
Note that all of this code is going away, the new API will set a keyword with or without post data, and won't allow changing that.

> This probably confuses me because the old API doesn't at all fit in with the
> new DB scheme, like you hinted above in the comment. Did you choose to
> update keywords with post_data=null because we usually only use this
> function right after creating a new keyword?

That's right, post data was added after keywords feature, as an annotation, so they had separate APIs and one had to create a keyword without post data and then add it.

> @@ +819,5 @@
> > +    let nullPostDataFragment = aPostData ? "AND post_data ISNULL" : "";
> > +    let stmt = PlacesUtils.history.DBConnection.createStatement(
> > +      `UPDATE moz_keywords SET post_data = :post_data
> > +       WHERE place_id = (SELECT fk FROM moz_bookmarks WHERE id = :item_id)
> > +      ${nullPostDataFragment}`);
> 
> With multiple keywords having post_data=null with the same place_id, will
> this try to update all of them and try to invalidate the
> (placed_id,post_data) unique constraint and fail? Maybe it doesn't, maybe we
> should add a LIMIT 1?

We don't support LIMIT in UPDATE queries.
But you are right, if multiple keywords to the same page exist without post data, this will throw.
The new API won't have this issue cause it creates keywords with post data already (clearly it will still throw if one tries to create a duped keyword/pd tuple).
I will see what I can do, I guess I can probably limit 1 in a subquery, and filter by id.

> @@ +821,5 @@
> > +      `UPDATE moz_keywords SET post_data = :post_data
> > +       WHERE place_id = (SELECT fk FROM moz_bookmarks WHERE id = :item_id)
> > +      ${nullPostDataFragment}`);
> > +    stmt.params.item_id = aBookmarkId;
> > +    stmt.params.post_data = aPostData;
> 
> Is aPostData=null correctly cast to SQL's NULL here? I'm not too familiar
> with that.

yes, null is properly converted to NULL.

> @@ +860,5 @@
> >     * @returns an array containing a string URL and a string of POST data
> >     */
> > +  getURLAndPostDataForKeyword(aKeyword) {
> > +    // In case of multiple urls associated to this keyword, we return the most
> > +    // recently created keyword.
> 
> That comment seems stale? We have multiple keywords per URI only, right? So
> we'll only ever find a single URL for a given keyword.

oh yeah.

> ::: toolkit/components/places/nsNavBookmarks.cpp
> @@ +2274,5 @@
> > +  nsTArray<BookmarkData> bookmarks;
> > +  rv = GetBookmarksForURI(uri, bookmarks);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  if (bookmarks.Length() == 0) {
> > +    for (uint32_t i = 0; i < keywords.Length(); ++i) {
> 
> Nit: int32_t

nsTArray Length is unsigned, iirc, using int32_t would warn and fail on warning.

> @@ +2402,4 @@
> >      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    bool hasMore;
> > +    while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {
> 
> We only expect a single result here, no?

yes

> @@ +2422,3 @@
> >      NS_ENSURE_SUCCESS(rv, rv);
> > +    for (uint32_t i = 0; i < bookmarks.Length(); ++i) {
> > +      NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
> 
> Don't we notify at the bottom of the function already?

here we are notifying that a keyword is being removed (notice EmptyCString()) while below we notify an addition.

> @@ +2540,4 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    bool hasMore;
> >    while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {
> 
> if () ? Or is there a reason we're using while everywhere?

I think it was just bad habits...

Comment 4

[Marco Bonardo [:mak]]

Attachment: interdiff

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v2

merged patch

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8573468 [details] [diff] [review]
patch v2

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

r+ because v1 + interdiff lgtm.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/687b6a735b34