Last Comment Bug 615413 - Clear out old weave/* annotations
: Clear out old weave/* annotations
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 9
Assigned To: Richard Newman [:rnewman]
:
Mentors:
Depends on: 614104
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-29 17:14 PST by Philipp von Weitershausen [:philikon]
Modified: 2011-09-09 16:28 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible patch. v1 (1.74 KB, patch)
2011-08-27 08:10 PDT, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 0: trailing whitespace in test file. (4.73 KB, patch)
2011-08-29 09:51 PDT, Richard Newman [:rnewman]
mak77: review+
Details | Diff | Splinter Review
Part 1: add cleanup query. (3.19 KB, patch)
2011-08-29 09:52 PDT, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 1: add cleanup queries. v2 (3.54 KB, patch)
2011-08-29 10:06 PDT, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 1: add cleanup queries. v3 (3.66 KB, patch)
2011-08-29 10:57 PDT, Richard Newman [:rnewman]
mak77: feedback+
Details | Diff | Splinter Review
Part 1: add cleanup queries. v4 (3.67 KB, patch)
2011-08-29 12:15 PDT, Richard Newman [:rnewman]
mak77: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2010-11-29 17:14:58 PST
In bug 614104 we're switching the history engine over to a new GUID format and a different GUID annotation name so that all entries get new GUIDs. We need to make sure the old ones are deleted. We can do this async on idle whenever, so long as it happens.
Comment 1 Philipp von Weitershausen [:philikon] 2010-12-06 11:02:16 PST
Changing this bug slightly to clear out more obsolete annotations:
* weave/guid history annotation (obsoleted bug 614104)
* weave/predecessor and weave/parent (obsoleted by bug 615410)
Comment 2 Philipp von Weitershausen [:philikon] 2011-01-13 21:07:34 PST
Since bug 623418 landed we can also get rid of the sync/children annotation.
Comment 3 Richard Newman [:rnewman] 2011-08-26 20:48:18 PDT
For the record, in my profile a whopping 11,193 of the 11,319 annos in places.sqlite are weave/guid. Apparently my profile is new enough to have no weave/predecessor or parent annos, or any sync/children annos.
Comment 4 Richard Newman [:rnewman] 2011-08-26 21:59:55 PDT
I seem to have accidentally started hacking on this.
Comment 5 Marco Bonardo [::mak] 2011-08-27 00:59:29 PDT
fwiw, we may do this in PlacesDBUtils, it is already setup to do cleanup work once a week.
Comment 6 Philipp von Weitershausen [:philikon] 2011-08-27 01:07:58 PDT
(In reply to Marco Bonardo [:mak] from comment #5)
> fwiw, we may do this in PlacesDBUtils, it is already setup to do cleanup
> work once a week.

I support this plan. I wouldn't want to re-invent the wheel for scheduling non-intrusive cleanup in Sync.
Comment 7 Richard Newman [:rnewman] 2011-08-27 08:10:03 PDT
Created attachment 556260 [details] [diff] [review]
Possible patch. v1

I've hand-tested the query (saved 3MB of places.sqlite on my profile, a little less than 8%!), but is this the kind of approach you were thinking of, mak?
Comment 8 Marco Bonardo [::mak] 2011-08-29 02:03:09 PDT
Comment on attachment 556260 [details] [diff] [review]
Possible patch. v1

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

I don't think this is worth a test.

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +333,5 @@
>            "(SELECT id FROM moz_places WHERE id = a.place_id LIMIT 1) " +
>        ")");
>      cleanupStatements.push(deleteOrphanAnnos);
>  
> +    // B.3 remove old weave/* and Sync annotations.

Ideally this should run before A1, since A1 removes unused attributes, so you may rename A1 to A2 and make this A1 (also in toolkit/components/places/tests/unit/test_preventive_maintenance.js). that should ensure a better cleanup

@@ +340,5 @@
> +      "WHERE anno_attribute_id IN ( "                    +
> +      "  SELECT id FROM moz_anno_attributes m_aa WHERE " +
> +      "    m_aa.name = 'weave/guid'        OR "          +
> +      "    m_aa.name = 'weave/predecessor' OR "          +
> +      "    m_aa.name = 'weave/parent'      OR "          +

I suppose you want to delete ALL weave/* annotations, then a
(aa.name > "weave/" and aa.name < "weave0") (0 is just the first char after ? in the unicode table) should be faster
Comment 9 Marco Bonardo [::mak] 2011-08-29 02:04:50 PDT
ah last thing, for bogus naming convensions moz_anno_attributes is usually aliased as "n" so FROM moz_anno_attributes n (in this case I suppose you don't need to alias it though since it's a subquery, so unless needed I'd not alias it and just go for name = something)
Comment 10 Philipp von Weitershausen [:philikon] 2011-08-29 09:20:47 PDT
Sure, we can get rid of all weave/* annotations. Note that we can also get rid of sync/children (see comment 2).
Comment 11 Richard Newman [:rnewman] 2011-08-29 09:34:13 PDT
(In reply to Marco Bonardo [:mak] from comment #9)
> so unless needed I'd
> not alias it and just go for name = something)

Yeah, in my initial test this was a SELECT with an explicit JOIN ON, and I left the alias in. Fixx0red.

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Sure, we can get rid of all weave/* annotations.

Updated patch to follow.
Comment 12 Richard Newman [:rnewman] 2011-08-29 09:51:17 PDT
Created attachment 556586 [details] [diff] [review]
Part 0: trailing whitespace in test file.

Couldn't resist.
Comment 13 Richard Newman [:rnewman] 2011-08-29 09:52:34 PDT
Created attachment 556587 [details] [diff] [review]
Part 1: add cleanup query.

Notes:

* >=, not >. Otherwise we don't catch guid itself :D
* Hand-tested on my own places.sqlite.
Comment 14 Richard Newman [:rnewman] 2011-08-29 09:59:41 PDT
Comment on attachment 556587 [details] [diff] [review]
Part 1: add cleanup query.

Might be a new patch for this; sync/children lives in moz_items_annos, so this query doesn't actually catch them.
Comment 15 Richard Newman [:rnewman] 2011-08-29 10:06:36 PDT
Created attachment 556592 [details] [diff] [review]
Part 1: add cleanup queries. v2

OK, two queries: one for moz_annos (weave/guid etc.) and one for moz_items_annos (sync/children).

Tested by hand on my profile.
Comment 16 Philipp von Weitershausen [:philikon] 2011-08-29 10:48:59 PDT
(In reply to Richard Newman [:rnewman] from comment #15)
> OK, two queries: one for moz_annos (weave/guid etc.) and one for
> moz_items_annos (sync/children).

Yeah, so moz_annos has the annotations for history places, moz_items_annos has the annotations for bookmarks. We want to remove all weave/* from moz_annos *AND* moz_items_annos and in addition to that sync/children from moz_items_annos.
Comment 17 Richard Newman [:rnewman] 2011-08-29 10:57:32 PDT
Created attachment 556611 [details] [diff] [review]
Part 1: add cleanup queries. v3

Address Comment 16.
Comment 18 Marco Bonardo [::mak] 2011-08-29 11:48:28 PDT
Comment on attachment 556586 [details] [diff] [review]
Part 0: trailing whitespace in test file.

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

you don't really want my review to remove some trailing spaces :)
Comment 19 Marco Bonardo [::mak] 2011-08-29 11:54:48 PDT
Comment on attachment 556611 [details] [diff] [review]
Part 1: add cleanup queries. v3

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

almost there

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +303,5 @@
>    {
>      let cleanupStatements = [];
>  
>      // MOZ_ANNO_ATTRIBUTES
> +    // A.1 remove old weave/* annotations from moz_annos.

I'd love to transform these into the "remove no more useful annos" task, so that in future we can just add more conditions (did someone say google toolbar annos?), so just avoid making comments and names Weave or Sync specific.

@@ +304,5 @@
>      let cleanupStatements = [];
>  
>      // MOZ_ANNO_ATTRIBUTES
> +    // A.1 remove old weave/* annotations from moz_annos.
> +    let deleteOldWeaveAnnos = DBConn.createAsyncStatement(

like for example deleteUselessAnnos or something similar (your english beats mine 100:0)

@@ +311,5 @@
> +      "  SELECT id FROM moz_anno_attributes WHERE " +
> +      "    name >= 'weave/guid' AND "               +
> +      "    name <  'weave0' "                       +
> +      ")");
> +    cleanupStatements.push(deleteOldWeaveAnnos);

let's use WHERE name BETWEEN 'weave/' AND 'weave0'

@@ +314,5 @@
> +      ")");
> +    cleanupStatements.push(deleteOldWeaveAnnos);
> +
> +    // A.2 remove old Weave and Sync annotations from moz_items_annos.
> +    let deleteOldSyncAnnos = DBConn.createAsyncStatement(

and deleteUselessItemsAnnos or similar

@@ +320,5 @@
> +      "WHERE anno_attribute_id IN ( "               +
> +      "  SELECT id FROM moz_anno_attributes WHERE " +
> +      "    name = 'sync/children' OR "              +
> +      "    name >= 'weave/guid' AND "               +
> +      "    name <  'weave0' "                       +

as well here WHERE name BETWEEN 'weave/' AND 'weave0' OR name = 'sync/children'
Comment 20 Richard Newman [:rnewman] 2011-08-29 12:15:23 PDT
Created attachment 556643 [details] [diff] [review]
Part 1: add cleanup queries. v4

Switches to using BETWEEN. Rename. Add comment about cryptic "weave0".
Comment 21 Marco Bonardo [::mak] 2011-08-29 12:36:49 PDT
Comment on attachment 556643 [details] [diff] [review]
Part 1: add cleanup queries. v4

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

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +303,5 @@
>    {
>      let cleanupStatements = [];
>  
>      // MOZ_ANNO_ATTRIBUTES
> +    // A.1 remove old annotations from moz_annos.

s/old/obsolete/ (being old is not being useless, otherwise I'd not be here!)

@@ +310,5 @@
> +    let deleteObsoleteAnnos = DBConn.createAsyncStatement(
> +      "DELETE FROM moz_annos "                      +
> +      "WHERE anno_attribute_id IN ( "               +
> +      "  SELECT id FROM moz_anno_attributes WHERE " +
> +      "    name BETWEEN 'weave/' AND 'weave0' "     +

nit: I prefer WHERE on new line (aligned with SELECT), but it's not that important

@@ +314,5 @@
> +      "    name BETWEEN 'weave/' AND 'weave0' "     +
> +      ")");
> +    cleanupStatements.push(deleteObsoleteAnnos);
> +
> +    // A.2 remove old annotations from moz_items_annos.

ditto

@@ +319,5 @@
> +    let deleteObsoleteItemsAnnos = DBConn.createAsyncStatement(
> +      "DELETE FROM moz_items_annos "                +
> +      "WHERE anno_attribute_id IN ( "               +
> +      "  SELECT id FROM moz_anno_attributes WHERE " +
> +      "    name = 'sync/children' OR "              +

ditto nit: aligned like
WHERE name =
   OR name BETWEEN

Note You need to log in before you can comment on or make changes to this bug.