Closed Bug 615413 Opened 14 years ago Closed 13 years ago

Clear out old weave/* annotations

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 9

People

(Reporter: philikon, Assigned: rnewman)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
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)
Summary: Clear out old weave/guid history annotations → Clear out old weave/* annotations
Since bug 623418 landed we can also get rid of the sync/children annotation.
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.
Target Milestone: --- → mozilla8
Target Milestone: mozilla8 → ---
I seem to have accidentally started hacking on this.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
fwiw, we may do this in PlacesDBUtils, it is already setup to do cleanup work once a week.
(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.
Attached patch Possible patch. v1 (obsolete) — Splinter Review
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?
Attachment #556260 - Flags: feedback?(mak77)
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
Attachment #556260 - Flags: feedback?(mak77)
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)
Sure, we can get rid of all weave/* annotations. Note that we can also get rid of sync/children (see comment 2).
(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.
Component: Firefox Sync: Backend → Bookmarks & History
Product: Mozilla Services → Firefox
QA Contact: sync-backend → bookmarks
Couldn't resist.
Attached patch Part 1: add cleanup query. (obsolete) — Splinter Review
Notes:

* >=, not >. Otherwise we don't catch guid itself :D
* Hand-tested on my own places.sqlite.
Attachment #556587 - Flags: review?(mak77)
Attachment #556586 - Flags: review?(mak77)
Attachment #556260 - Attachment is obsolete: true
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.
Attachment #556587 - Flags: review?(mak77)
Attached patch Part 1: add cleanup queries. v2 (obsolete) — Splinter Review
OK, two queries: one for moz_annos (weave/guid etc.) and one for moz_items_annos (sync/children).

Tested by hand on my profile.
Attachment #556587 - Attachment is obsolete: true
Attachment #556592 - Flags: review?(mak77)
(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.
Address Comment 16.
Attachment #556592 - Attachment is obsolete: true
Attachment #556611 - Flags: review?(mak77)
Attachment #556611 - Flags: feedback?(philipp)
Attachment #556592 - Flags: review?(mak77)
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 :)
Attachment #556586 - Flags: review?(mak77) → review+
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'
Attachment #556611 - Flags: review?(mak77) → feedback+
Switches to using BETWEEN. Rename. Add comment about cryptic "weave0".
Attachment #556643 - Flags: review?(mak77)
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
Attachment #556643 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/9d393210a75e
http://hg.mozilla.org/mozilla-central/rev/4ce9a64d1ddf
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Version: unspecified → Trunk
Attachment #556611 - Flags: feedback?(philipp)
You need to log in before you can comment on or make changes to this bug.