Closed
Bug 615413
Opened 14 years ago
Closed 13 years ago
Clear out old weave/* annotations
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 9
People
(Reporter: philikon, Assigned: rnewman)
References
Details
Attachments
(3 files, 3 obsolete files)
4.73 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
mak
:
feedback+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•14 years ago
|
||
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
Reporter | ||
Comment 2•13 years ago
|
||
Since bug 623418 landed we can also get rid of the sync/children annotation.
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Target Milestone: mozilla8 → ---
Assignee | ||
Comment 4•13 years ago
|
||
I seem to have accidentally started hacking on this.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Comment 5•13 years ago
|
||
fwiw, we may do this in PlacesDBUtils, it is already setup to do cleanup work once a week.
Reporter | ||
Comment 6•13 years ago
|
||
(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.
Assignee | ||
Comment 7•13 years ago
|
||
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 8•13 years ago
|
||
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)
Comment 9•13 years ago
|
||
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)
Reporter | ||
Comment 10•13 years ago
|
||
Sure, we can get rid of all weave/* annotations. Note that we can also get rid of sync/children (see comment 2).
Assignee | ||
Comment 11•13 years ago
|
||
(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
Assignee | ||
Comment 12•13 years ago
|
||
Couldn't resist.
Assignee | ||
Comment 13•13 years ago
|
||
Notes: * >=, not >. Otherwise we don't catch guid itself :D * Hand-tested on my own places.sqlite.
Attachment #556587 -
Flags: review?(mak77)
Assignee | ||
Updated•13 years ago
|
Attachment #556586 -
Flags: review?(mak77)
Assignee | ||
Updated•13 years ago
|
Attachment #556260 -
Attachment is obsolete: true
Assignee | ||
Comment 14•13 years ago
|
||
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)
Assignee | ||
Comment 15•13 years ago
|
||
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)
Reporter | ||
Comment 16•13 years ago
|
||
(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.
Assignee | ||
Comment 17•13 years ago
|
||
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 18•13 years ago
|
||
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 19•13 years ago
|
||
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+
Assignee | ||
Comment 20•13 years ago
|
||
Switches to using BETWEEN. Rename. Add comment about cryptic "weave0".
Attachment #556643 -
Flags: review?(mak77)
Comment 21•13 years ago
|
||
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+
Assignee | ||
Comment 22•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/9d393210a75e http://hg.mozilla.org/integration/mozilla-inbound/rev/4ce9a64d1ddf
Whiteboard: [inbound]
Comment 23•13 years ago
|
||
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
Reporter | ||
Updated•13 years ago
|
Attachment #556611 -
Flags: feedback?(philipp)
You need to log in
before you can comment on or make changes to this bug.
Description
•