+++ This bug was initially created as a clone of Bug #614104 +++ Bug 607112 will introduce GUID columns in moz_places and moz_bookmarks proper. Places will do an initial migration that will port over our old GUIDs to new ones. Sync will have to handle this in a sane way that will work on Firefox 3.6 as well as on Firefox 4.0 as well as on profiles that have gone through the migration but users went back to 3.6. We also do not want to do a full reupload when the migration happens (since it could happen multiple times as users go back and forth between Firefox versions on the same profile.)

Assign new-style GUIDs to bookmarks in annotations. Reusing a lot of code from history engine. Lots of code duplication, should clean that up eventually. Not submitting this for review yet because I'd like to write more tests first, then rebase this patch on top of the added tests. The bookmark tests we have so far are a joke and don't make me feel confident at all, especially in the light of further refactorings to come (bug 615285).

This is far from a comprehensive test suite, but at least it adds tests for some of the more involved code paths we have in bookmark sync.

Rebase v1.0 on top of additional tests in prereq (v1). No code change.

Comment on attachment 494300 [details] [diff] [review] v1.1 This all looks sane to me, but it'd be good to get sdwilsh to look over the sql and places API stuff, especially since this is on a tight deadline.

Comment on attachment 494300 [details] [diff] [review] v1.1 >+ get _checkGUIDItemAnnotationStm() { >+ let stmt = this._getStmt( >+ "SELECT b.id AS item_id, " + >+ "(SELECT id FROM moz_anno_attributes WHERE name = :anno_name) AS name_id, " + >+ "a.id AS anno_id, a.dateAdded AS anno_date " + >+ "FROM moz_bookmarks b " + >+ "LEFT JOIN moz_items_annos a ON a.item_id = b.id " + >+ "AND a.anno_attribute_id = name_id " + >+ "WHERE b.id = :item_id"); It is probably cheaper to join moz_items_annos with moz_annot_attributes. Check EXPLAIN output, and go with what's cheaper. It'd really be best if you could just note the id of your annotation, and store it. Having to look it up each time is sucky. But then, I'm not sure if I care that much since this is only for 3.6. My biggest concern about this patch is that it doesn't check for the guid column on moz_bookmarks, so the second we merge Places into mozilla-central, things get weird fast.

(In reply to comment #5) > It is probably cheaper to join moz_items_annos with moz_annot_attributes. This is straight out of nsAnnotationService.cpp (and similar queries are doing their work in the history engine already). > Check EXPLAIN output, and go with what's cheaper. It'd really be best if you > could just note the id of your annotation, and store it. Having to look it up > each time is sucky. Hmm, I guess. Would there be a potential issue with cache invalidation? If not, why doesn't the nsAnnotationService do it this way? I'll be happy to address performance concerns in a follow-up. This is definitely not slower than what we currently have because it uses the same queries as nsAnnotationService itself. For b8 it's just important that we have the right GUIDs out there as long as everybody is reupload their stuff. > But then, I'm not sure if I care that much since this is only for 3.6. In the long run, yes. > My biggest concern about this patch is that it doesn't check for the guid > column on moz_bookmarks, so the second we merge Places into mozilla-central, > things get weird fast. Right, well, we'll definitely have to address that in a post-b8 follow up, both for bookmarks and history. Just like the temp table stuff, it'll have to land in Sync before you guys can merge places. Can you file a bug on us for that?

(In reply to comment #6) > This is straight out of nsAnnotationService.cpp (and similar queries are doing > their work in the history engine already). I never said the places code was using the best optimization ;) > Hmm, I guess. Would there be a potential issue with cache invalidation? If not, > why doesn't the nsAnnotationService do it this way? Nope. With temp tables it was cheaper to do subqueries, but without them, it seems to be simpler to do joins. That, and the SQLite optimizer changes over time. We should probably update our code if it turns out the join is faster. > Right, well, we'll definitely have to address that in a post-b8 follow up, both > for bookmarks and history. Just like the temp table stuff, it'll have to land > in Sync before you guys can merge places. It's looking like that is going to happen right after beta 8. Would it be feasible to have sync merge onto the Places branch first so we can make sure the world is green before we merge Places to mozilla-central?

Comment on attachment 494300 [details] [diff] [review] v1.1 r=sdwilsh

(In reply to comment #7) > > Hmm, I guess. Would there be a potential issue with cache invalidation? If not, > > why doesn't the nsAnnotationService do it this way? > Nope. With temp tables it was cheaper to do subqueries, but without them, it > seems to be simpler to do joins. That, and the SQLite optimizer changes over > time. We should probably update our code if it turns out the join is faster. I see. That makes sense. Let's investigate in a follow-up. Filed bug 616002. > > Right, well, we'll definitely have to address that in a post-b8 follow up, both > > for bookmarks and history. Just like the temp table stuff, it'll have to land > > in Sync before you guys can merge places. > It's looking like that is going to happen right after beta 8. Would it be > feasible to have sync merge onto the Places branch first so we can make sure > the world is green before we merge Places to mozilla-central? That seems like a good plan to me.

(In reply to comment #6) > > My biggest concern about this patch is that it doesn't check for the guid > > column on moz_bookmarks, so the second we merge Places into mozilla-central, > > things get weird fast. > > Right, well, we'll definitely have to address that in a post-b8 follow up, both > for bookmarks and history. Just like the temp table stuff, it'll have to land > in Sync before you guys can merge places. > > Can you file a bug on us for that? Filed bug 616001, and set it to blocking beta 9 (although we need it early in that cycle)

Crossweave revealed some problems with the v1.1 patch, will reupload a new patch shortly.

Fix some smaller bugs exposed by Crossweave: * failure to set GUID in some cases * wrong default return value from store.idForGUID Also fix a failing unit tests that I must've missed the first time round.

Use different annotations for parent and predecessor. The old annotations will no longer be valid because they point to a different kind of GUID. Since we're using our own GUID system now, we also don't have to munge predecessor and parent GUIDs before setting them.

https://hg.mozilla.org/services/fx-sync/rev/6d75dc3c3c16 https://hg.mozilla.org/services/fx-sync/rev/4307f8edcf93 https://hg.mozilla.org/services/fx-sync/rev/fb727f693cfe