607112 - make GUID a column in moz_places and moz_bookmarks

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mike Connor [:mconnor]]

Sync is built-in, and ends up creating GUIDs for everything, which means we have to hit the annotations a lot.  Making this a column will make things faster and saner.

For bonus points, bookmark GUIDs are in moz_items_annos, and history GUIDs are in moz_annos.  That really doesn't feel like victory.

Comment 1

[Shawn Wilsher :sdwilsh]

We'll need an entry on moz_places for history, and then moz_bookmarks for each bookmark (since you can have more than one bookmark per place).  This isn't a problem, right?

Comment 2

[Shawn Wilsher :sdwilsh]

When migrating old guids from annotation table, we'll need to modify them to the new guid style implemented in bug 607115.  Consumers will need to know about this one-time guid change, so we can dispatch an annotation observer for removal (before we do the removal, so consumers can, sadly, synchronously obtain the current guid).  We can then dispatch one more notification via the annotation service indicating that the guid was added.  This might not break existing consumers, but chances are it would.

Alternatively, we can add a new interface that looks something like this:
interface nsIGuidChangedObserver : public nsISupports
{
  void onItemGuidChange(AUTF8String aOldGuid, AUTF8String aNewGuid);
  void onPlaceGuidChange(AUTF8String aOldGuid, aUTF8String aNewGuid);
}
We could also pass the bookmark id and place id, respectively, if desired.

I think the new interface approach is smarter, and works well for when something like Sync needs to change the guid to match what is in the cloud.  Thoughts?

Comment 3

[Shawn Wilsher :sdwilsh]

OK, after talking this over with philiKON, this is what we've come up with:
For bookmarks, we'll continue to generate the old-stye guid for all bookmarks (after bug 607117 lands).  We'll then take the generated guid, and morph it into the new style guid by taking the first 18 characters from it (9 bytes), unhexing it, and then base64url encoding that result.  This will provide a mapping from 3.6 style guids to the new style guid.  It sucks that we'll have to keep adding these, but it means we'll stay compatible with 3.6.

For history, we'll import guids from the annotation table if they are in the right form, and then remove them from the annotation table.  Sync can handle either way because it can roll its own queries.

On first run, as long as we support 3.6, we'll have to check to see if moz_places.guid or moz_bookmarks.guid have null entries and update them accordingly.  Sucks, I know.

Comment 4

[Shawn Wilsher :sdwilsh]

I think I am going to break this bug up like this:
Part 1 - implement a helper method old guid -> new guid
Part 2 - implement a SQL function to wrap part 1 to be used in queries + tests
Part 3 - add colun + migration code for bookmarks
Part 4 - implement a valid-guid function that checks if something is a valid new-style guid
Part 5 - Add column + migration code for history

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - create a helper method for converting guids

This creates a helper method that will convert the old-style guid to the new-style guid.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - SQL function to wrap part 1

SQL function that wraps part 1.  This also includes a bunch of tests to make sure this is all sane.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Part 3 - add colun + migration code for bookmarks
> Part 4 - implement a valid-guid function that checks if something is a valid
> new-style guid
> Part 5 - Add column + migration code for history
These all get shifted by one.  The new part 3 is adding a SQL function for our old-style guid.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - old-style guid SQL function

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: Part 4 - create column and add guids (wip)

This is a work in progress.  Putting this up in case mak wants to grab this while I am away.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 492764 [details] [diff] [review]
Part 1 - create a helper method for converting guids

>diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp

>+nsresult
>+ConvertGUID(const nsACString& aOriginalGUID,
>+            nsCString& _guid)
>+{

>+  // and 12.  Depending on the size of our extra int, use as many of bytes 1, 2,
>+  // 3, and 4 (in that order).

nit: This can probably be clarified a bit, you always set bytes to [1, 2, 3, 4], but then later you overwrite as many of them as needed with the int.
From the comment instead looks like you append only some of the bytes here and then append non-zero bytes of the int.

>diff --git a/toolkit/components/places/src/Helpers.h b/toolkit/components/places/src/Helpers.h

> /**
>+ * Generates a 12 character guid in the same form as GenerateGUID from a guid
>+ * generated by nsIUUIDGenerator.
>+ *
>+ * @param aOriginalGUID
>+ *        The original guid that we'll convert.
>+ */
>+nsresult ConvertGUID(const nsACString& aOriginalGUID, nsCString& _guid);

javadoc is missing the second param or a @return,
Can we avoid mixup abstract and concrete string classes?

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 492766 [details] [diff] [review]
Part 2 - SQL function to wrap part 1

>diff --git a/toolkit/components/places/tests/unit/test_sql_guid_functions.js b/toolkit/components/places/tests/unit/test_sql_guid_functions.js

>+function test_convert_guid_invariants()
>+{

>+  let newGuids = [
>+    "XXFD1yXmOZXR", // 0x0
>+    "XXFD1yXmOZUB", // 0x01
>+    "XXFD1yXmOQEA", // 0x0100
>+    "XXFD1yXmAQAA", // 0x010000
>+    "XXFD1yUBAAAA", // 0x01000000
>+  ];
>+  do_check_eq(oldGuids.length, newGuids.length);

this test is useful only when making changes to the test, but pollutes output uselessly otherwise.
please convert it to:
if (oldGuids.length != newGuids.length) {
  do_throw("something");
}

and probably the same for the allowed chars length check you have at the beginning

>+    handleResult: function(aResult) {
>+      try {
>+        let row = aResult.getNextRow();
>+        let guid = row.getResultByIndex(0);
>+        check_invariants(guid);
>+        do_check_eq(guid, NEW_GUID);
>+        do_check_eq(aResult.getNextRow(), null);

I guess do_check_false could work here.

> let tests = [
>   test_guid_invariants,
>   test_guid_on_background,
>+  test_convert_guid_invariants,
>+  test_convert_guid_on_background,

it's probably worth to also test that trying to convert bad guids does correctly throw.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 492764 [details] [diff] [review]
Part 1 - create a helper method for converting guids

>+  // Our new guids use 72 bits (9 bytes).  To convert this into our new style
>+  // guid, imagine the uuid part of the old guid in the form of numbered bytes:
>+  // [1, 2, 3, 4]-[5, 6]-[7, 8]-[9, 10]-[11, 12, 13, 14, 15, 16]
>+  // For the new guid, we'll always take (in this order) bytes 16, 15, 14, 13,
>+  // and 12.  Depending on the size of our extra int, use as many of bytes 1, 2,
>+  // 3, and 4 (in that order).  Finally append the non-zero bytes of the int.

Sadly we can't use this scheme because we'll run into lots of collisions, e.g.:

  {c95768e2-43b2-ca41-8670-b1ca7eb89249}0
  {c95768e2-43b2-ca41-8670-b1ca7eb89249}226

both convert to {73,146,184,126,202,201,87,104,226} == SZK4fsrJV2ji in base64url because the 4th in the UUID happens to be 0xe2 == 226. So essentially we'll always run into collisions if the int matches what's in the 4th byte of the UUID, or the 3rd and 4th byte, etc.

Proposal: Always reserve the last 2 bytes for the integer. This reduces the entropy to roughly 56 + log2(N) bits where N is the average number of bookmarks created per session. Compared to the 61 bits of the current Sync GUIDs, I think that's not so bad. It also means we don't support cases where more than 65536 bookmarks were created in one session. I think that's ok, though.

>+  // Now add the bytes from additionalInt.  If you can imagine the 32 bit
>+  // integer in the form of numbered bytes [1, 2, 3, 4], take bytes 4, 3, 2, and
>+  // then 1 (most significant to least significant).  Once we start getting
>+  // bytes representing zero, we stop.
>+  for (PRUint32 idx = kBytes - 1; additionalInt;
>+       additionalInt = additionalInt >> 8, idx--) {
>+    buffer[idx] = additionalInt & 0xFF;
>+  }

I think the comment is wrong. It looks to me we take the least to most significant bytes, not the other way around.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> I think the comment is wrong. It looks to me we take the least to most
> significant bytes, not the other way around.
This code assumes little-endian, where the most significant byte is the right-most [1].  Since I am filtering the rightmost byte of the number, and bit shifting it away in each loop iteration, I'm pretty sure I'm taking the most significant to least significant in the code, as the comment says.

Although, maybe what you were saying is that we assign it to the new guid bytes in reverse order.  The comment doesn't really talk about it, so it could certainly be improved.

[1] http://en.wikipedia.org/wiki/Endianness

Comment 14

[Philipp von Weitershausen [:philikon]]

Thinking about this some more, I think we should do for bookmarks what we do for history: Discard the old GUIDs and just use the new ones.

We devised the old GUID -> new GUID derivation scheme so that Sync could start using new-style GUIDs before Places introduced them, ensuring that the ones Places would assign to bookmarks were the same as the ones Sync would derive now. Unfortunately this system breaks down when the bookmarks are synced to another computer. There the bookmarks are going to get some random old-style GUIDs that won't translate to their new-style GUID. Unless we keep syncing the old-style GUID somehow there's no way to know what the original old-style GUID was.

Proposal: Just like with history, Sync will generate random new style GUIDs for bookmarks and store them in a custom annotation (item annotations for bookmarks, as opposed to page annotations for places). The Places migration will transfer any GUIDs present in those annotations to the GUID column. That way GUID integrity is ensured beyond the migration.

To summarize, Sync will
* stop using old-style GUIDs for bookmarks entirely,
* use an existing new-style GUID if it's present in either the GUID column or the custom GUID annotation,
* otherwise generate a new-style GUID and store it in either the GUID column or the custom GUID annotation (depending on whether the GUID column is available or not),

and Places will
* stop supporting old-style GUIDs for bookmarks entirely,
* migrate any existing new-style GUIDs from custom Sync annotations to the GUID columns,
* generate new-style GUIDs for all items that don't have one yet during the migration

That way we can generate and assign new-style GUIDs in Sync now and they'll the same after the migration, even when the user time-travels with their profile.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> Proposal: Just like with history, Sync will generate random new style GUIDs for
> bookmarks and store them in a custom annotation (item annotations for
> bookmarks, as opposed to page annotations for places). The Places migration
> will transfer any GUIDs present in those annotations to the GUID column. That
> way GUID integrity is ensured beyond the migration.
Places code can transfer the guids no problem.  What I'm concerned about is the case where we go from 4.0 -> 3.6 and then possibly back again.  We could do a one time breakage of all guid consumers, but that would cause problems when a user downgraded to 3.6.  We can keep both the new and old style guid, and we just have to generate the new-style whenever the user launches 4.0 again.  There isn't anything terrible with this approach, I don't think.  We could also not have a mapping of old->new and just generate the new-style guid.  I'm not sure if there is a draw-back there either.

mak, dietrich: thoughts?

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to comment #15)
> We can keep both the new and old style guid, and we
> just have to generate the new-style whenever the user launches 4.0 again.

Yes. Or, in case they had Sync running on 3.6, transfer the new-style GUIDs from Sync's custom annotation.

> There isn't anything terrible with this approach, I don't think.  We could also
> not have a mapping of old->new and just generate the new-style guid.  I'm not
> sure if there is a draw-back there either.

Well, for backward compat reasons we probably have to keep the old GUIDs around, at least as long as the nsINavBookmarkService::getItemGUID and ::getItemIdForGUID APIs are still there. So indeed what I'm proposing is to have a separate set of new-style GUIDs. They can be randomly generated, there's really no point in limiting the entropy by deriving them from the old-style GUID.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Yes. Or, in case they had Sync running on 3.6, transfer the new-style GUIDs
> from Sync's custom annotation.
We need to make sure Sync doesn't use the same annotation name; it should be completely different from what Firefox currently uses.

> Well, for backward compat reasons we probably have to keep the old GUIDs
> around, at least as long as the nsINavBookmarkService::getItemGUID and
> ::getItemIdForGUID APIs are still there. So indeed what I'm proposing is to
> have a separate set of new-style GUIDs. They can be randomly generated, there's
> really no point in limiting the entropy by deriving them from the old-style
> GUID.
I think this is OK.  Going to move forward with this.

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to comment #17)
> (In reply to comment #16)
> > Yes. Or, in case they had Sync running on 3.6, transfer the new-style GUIDs
> > from Sync's custom annotation.
> We need to make sure Sync doesn't use the same annotation name; it should be
> completely different from what Firefox currently uses.

Yes. I propose to use "sync/guid" for both history page annotations and bookmark item annotations.

> > Well, for backward compat reasons we probably have to keep the old GUIDs
> > around, at least as long as the nsINavBookmarkService::getItemGUID and
> > ::getItemIdForGUID APIs are still there. So indeed what I'm proposing is to
> > have a separate set of new-style GUIDs. They can be randomly generated, there's
> > really no point in limiting the entropy by deriving them from the old-style
> > GUID.
> I think this is OK.  Going to move forward with this.

Great! Me too (in Sync)

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - create column and add guids v2.0

OK, Part 4 is now Part 1, and this is the new part breakdown:
Part 1: create and update moz_bookmarks.guid
Part 2: import guids from annotation item table for bookmarks, and nuke them
Part 3: create and update moz_places.guid
Part 4: import guids from annotation table for places, and nuke them

Marco - I'm interested in more testing ideas for migration (bonus points if you want to add them).

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 493822 [details] [diff] [review]
Part 1 - create column and add guids v2.0

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>+nsresult
>+nsNavHistory::CheckAndUpdateGUIDs()
>+{
>+  // NSS must be initialized on the main thread.  The GENERATE_GUID SQL function
>+  // might end up doing that, so grab nsIRandomGenerator now.
>+  nsCOMPtr<nsIRandomGenerator> dummy =
>+    do_GetService("@mozilla.org/security/random-generator;1");

I think a failure in doing this would end up being pretty much negative on us (we won't be able to make guids and the service could then be initialized off the main thread), maybe you should ENSURE_STATE it

>+  // TODO See if Sync has created any of these annotations first.
>+

these don't look like annotations, and the isnull is skipping populated fields.

>+  // Update moz_bookmarks.guid

looks useless, the query is readable enough

>@@ -1817,6 +1843,26 @@ nsNavHistory::MigrateV11Up(mozIStorageCo
>   );
>   NS_ENSURE_SUCCESS(rv, rv);

>+  if (NS_FAILED(rv)) {
>+    // moz_bookmarks grew a guid column.  Add the column, but do not populate it
>+    // with anything just yet.  We will do that soon (but not during migration).

well, checkAndUpdateGUIDs is called during migration...

>+    rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "ALTER TABLE moz_bookmarks "
>+      "ADD COLUMN guid TEXT"

I know that could slightly slowdown inserts, but we should consider forcing a UNIQUE constraint, especially if we will do any SELECT WHERE guid=something, that wants a index (looks like Sync could do that often). I'd add such a index now indeed.

>diff --git a/toolkit/components/places/tests/migration/head_migration.js b/toolkit/components/places/tests/migration/head_migration.js

>+function setPlacesDatabase(aFileName)
>+{

>+  file.copyTo(gProfD, kDBName);

I have some fear regarding file locking on windows, I hope it won't go intermittent orange for this. I guess if a rename(move) would be better.

>diff --git a/toolkit/components/places/tests/migration/places_v10.sqlite b/toolkit/components/places/tests/migration/places_v10.sqlite

for maintenance of the tests it would probably be better to have a create_database function that creates a db with a given version and given data.
But I see that could be much longer to do, so it's ok to use provided .sqlite files for now.

>diff --git a/toolkit/components/places/tests/migration/test_v11_from_v10.js b/toolkit/components/places/tests/migration/test_v11_from_v10.js

sanity check initial and final schema versions are correct please

which part is going to handle guids creation for each new added bookmark, I don't see that part listed.

If there would not be downgrades we could have setup the column to NOT NULL DEFAULT GENERATE_GUID(). On alter table SQLite will automatically populate all null entries, and for each added entry it will automatically make a guid, without us having to do anything. The problem with this is that old versions of the browser don't know about GENERATE_GUID() funcion and they will throw :( that's a pity.

there is nothing blocking, but I think we should evaluate the index now, and you need part 1.5 to add guids to new bookmark entries.

Comment 21

[Marco Bonardo [:mak]]

you also probably want to add a simple test for v11->v10->v11... you could either try using setUserSchema and createInstance on history, or just take a v11 db, set schema to v10 and ship it with the test.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #20)
> I think a failure in doing this would end up being pretty much negative on us
> (we won't be able to make guids and the service could then be initialized off
> the main thread), maybe you should ENSURE_STATE it
ExecuteStep would fail in the line before, so it's not a big deal to ignore it.

> these don't look like annotations, and the isnull is skipping populated fields.
This TODO will go away in part 2.  It's just there so I didn't forget.

> well, checkAndUpdateGUIDs is called during migration...
I didn't used to do that.  I guess I forgot to update the comment :)

> I know that could slightly slowdown inserts, but we should consider forcing a
> UNIQUE constraint, especially if we will do any SELECT WHERE guid=something,
> that wants a index (looks like Sync could do that often). I'd add such a index
> now indeed.
We cannot do that because then anyone who downgrades back to 3.6 would never add a guid, so every bookmark insert would fail.  I know, it sucks :(

> I have some fear regarding file locking on windows, I hope it won't go
> intermittent orange for this. I guess if a rename(move) would be better.
We don't want to remove the file from the source dir though, and I think that is what would happen.  Regardless, the download manager test code has been doing this for years to test it's schema migration code, so I don't think we'll have any problems.

> for maintenance of the tests it would probably be better to have a
> create_database function that creates a db with a given version and given data.
> But I see that could be much longer to do, so it's ok to use provided .sqlite
> files for now.
I think it's better to actually generate a database with 3.6 to make sure we are testing real-life things.  It's too easy to get the schema wrong given how complicated ours is now anyway.

> which part is going to handle guids creation for each new added bookmark, I
> don't see that part listed.
That is bug 607117.  Feel free to grab that if you want, although it might be hard to write tests for without this bug done yet (this bug is my first priority today).

> If there would not be downgrades we could have setup the column to NOT NULL
> DEFAULT GENERATE_GUID(). On alter table SQLite will automatically populate all
> null entries, and for each added entry it will automatically make a guid,
> without us having to do anything. The problem with this is that old versions of
> the browser don't know about GENERATE_GUID() funcion and they will throw :(
> that's a pity.
Indeed.

> there is nothing blocking, but I think we should evaluate the index now, and
> you need part 1.5 to add guids to new bookmark entries.
Oh right, we should add an index here too.  That's actually really really important :)

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> you also probably want to add a simple test for v11->v10->v11... you could
> either try using setUserSchema and createInstance on history, or just take a
> v11 db, set schema to v10 and ship it with the test.
Let's do that in part 5.  At that point, we'll have the completed v11 schema (I think).

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #22)
> (In reply to comment #20)
> > I think a failure in doing this would end up being pretty much negative on us
> > (we won't be able to make guids and the service could then be initialized off
> > the main thread), maybe you should ENSURE_STATE it
> ExecuteStep would fail in the line before, so it's not a big deal to ignore it.

which executeStep and how is Storage related to getting the randomGenerator service?

(In reply to comment #22)
> > I know that could slightly slowdown inserts, but we should consider forcing a
> > UNIQUE constraint, especially if we will do any SELECT WHERE guid=something,
> > that wants a index (looks like Sync could do that often). I'd add such a index
> > now indeed.
> We cannot do that because then anyone who downgrades back to 3.6 would never
> add a guid, so every bookmark insert would fail.  I know, it sucks :(

I don't think unique forces not null, does it?

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> which executeStep and how is Storage related to getting the randomGenerator
> service?
The ExecuteStep in that same method.  GENERATE_GUID uses nsIRandomGenerator to generate its byte stream to create its guid.  The problem is that NSS needs to be initialized on the main thread.  Actually...this can go away now since I'm not doing this async anymore.

> I don't think unique forces not null, does it?
No, but I think you can only have one null value, otherwise it's not unique.

Comment 26

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - create column and add guids v2.1

Enough changes here that I want you to look at this again.

Comment 27

[Shawn Wilsher :sdwilsh]

Note: in the test, I've locally changed openDatabase to openUnsharedDatabase because we don't want to mess with the shared cache.

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 494019 [details] [diff] [review]
Part 1 - create column and add guids v2.1

>diff --git a/toolkit/components/places/src/nsPlacesIndexes.h b/toolkit/components/places/src/nsPlacesIndexes.h

>+#define CREATE_IDX_MOZ_BOOKMARKS_GUID \
>+  CREATE_PLACES_IDX( \
>+    "guidindex", "moz_bookmarks", "guid", "UNIQUE" \

nit: per convention with other indices, this should be "guid_uniqueindex" (see "url_uniqueindex") but I'm not sure I care so much :p

>diff --git a/toolkit/components/places/tests/migration/test_v11_from_v10.js b/toolkit/components/places/tests/migration/test_v11_from_v10.js

>+function test_final_state()
>+{

>+  let db = Services.storage.openDatabase(dbFile);
>+
>+  let (stmt = db.createStatement("PRAGMA journal_mode")) {
>+    do_check_true(stmt.executeStep());
>+    // WAL journal mode should be set on this database.
>+    do_check_eq(stmt.getString(0).toLowerCase(), "wal");
>+    stmt.finalize();
>+  }
>+
>+  do_check_true(DBConn().indexExists("moz_bookmarks_guidindex"));
>+
>+  do_check_eq(db.schemaVersion, 11);

you are mixing usage of db and DBConn, is this intended?

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> you are mixing usage of db and DBConn, is this intended?
No :(

Fixing that, and making more changes to the test since I found that it wasn't testing something that I thought it was (and something is broken :( )

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> nit: per convention with other indices, this should be "guid_uniqueindex" (see
> "url_uniqueindex") but I'm not sure I care so much :p
We actually are not consistent, but I'm OK with being more verbose here.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - create column and add guids v2.2

per comments, plus a bug fix that mak and I talked about on irc

Comment 32

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - import bookmark guids from annotation table v2.0

I added five annotations to bookmarks in the database, all of which are guids.  They were:
__TESTGUID__
__TESTGUID__ (duplicate)
__TESTGUID2_
INVALIDGUID_LENGTH
INVALUDGUID%
+ tests to make sure these don't get imported.

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 494153 [details] [diff] [review]
Part 2 - import bookmark guids from annotation table v2.0

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+  // Next, generate guids for any bookmark that does not already have one.
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>     "UPDATE moz_bookmarks "
>     "SET guid = GENERATE_GUID() "
>     "WHERE guid IS NULL "
Note: I'm unsure if we should instead loop this query (and place a LIMIT 1 on it) and retry if we get a constraint error or not.  It's exceedingly unlikely that we'll have a collision, but...

Comment 34

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - create column and add guids for moz_places v2.0

Just like part 1, but for moz_places

Comment 35

[Shawn Wilsher :sdwilsh]

Attachment: Part 4 - import place guids from annotation table v2.0

Just like part 2, but for moz_places.  Added the same test guids to some entries.

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 494153 [details] [diff] [review]
Part 2 - import bookmark guids from annotation table v2.0

>diff --git a/toolkit/components/places/src/Helpers.h b/toolkit/components/places/src/Helpers.h

> /**
>+ * Determines if the string is a valid guid or not.
>+ *
>+ * @return true if it is a valid guid, false otherwise.
>+ */
>+bool IsValidGUID(const nsCString& aGUID);

nit: this is missing a @param

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

> nsresult
> nsNavHistory::CheckAndUpdateGUIDs()
> {
>   nsCOMPtr<mozIStorageStatement> stmt;
>-  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    "SELECT item_id, content "
>+    "FROM moz_items_annos "
>+    "WHERE anno_attribute_id = ( "
>+      "SELECT id "
>+      "FROM moz_anno_attributes "
>+      "WHERE name = :anno_name "
>+    ") "

please, check with explain if this makes less steps than a join. I seem to recall the join is smaller without temp tables, but my mind is faulty.

hm, actually, I don't recall you opening a transaction for this work, are we in a migration transaction already?

Comment 37

[Marco Bonardo [:mak]]

Comment on attachment 494169 [details] [diff] [review]
Part 3 - create column and add guids for moz_places v2.0

I love boilerplate reviews.

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 494188 [details] [diff] [review]
Part 4 - import place guids from annotation table v2.0

check my comments on part 2, since this is a mirrror I doubt I will find much more than that.

>diff --git a/toolkit/components/places/tests/migration/test_v11_from_v10.js b/toolkit/components/places/tests/migration/test_v11_from_v10.js

>@@ -53,7 +55,7 @@ function test_initial_state()
>   do_check_false(db.indexExists("moz_bookmarks_guid_uniqueindex"));
>   do_check_false(db.indexExists("moz_places_guid_uniqueindex"));
> 
>-  // There should be one item annotation for a bookmark guid.
>+  // There should be five item annotations for a bookmark guid.

merge this change to the correct part please.

Comment 39

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> Let's do that in part 5.  At that point, we'll have the completed v11 schema (I
> think).
Actually, it will be a lot easier to get a good test database if we do that after I get bug 607117 done.  This bug is all done with the current set of patches up.

Comment 40

[Marco Bonardo [:mak]]

(In reply to comment #33)
> >+++ b/toolkit/components/places/src/nsNavHistory.cpp
> >+  // Next, generate guids for any bookmark that does not already have one.
> >+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
> >     "UPDATE moz_bookmarks "
> >     "SET guid = GENERATE_GUID() "
> >     "WHERE guid IS NULL "
> Note: I'm unsure if we should instead loop this query (and place a LIMIT 1 on
> it) and retry if we get a constraint error or not.  It's exceedingly unlikely
> that we'll have a collision, but...

Well, yeah, it's a bit scary case, but if it happens we are in a bad shape. How would Sync react to a entry without a guid?
We could handle these edge cases in PlacesDBUtils.jsm on idle if we don't want to complicate this code, provided Sync can survive the issue for some time..

Comment 41

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - import bookmark guids from annotation table v2.1

Addresses review comments

Comment 42

[Shawn Wilsher :sdwilsh]

Attachment: Part 3 - create column and add guids for moz_places v2.1

For checkin.

Comment 43

[Shawn Wilsher :sdwilsh]

Attachment: Part 4 - import place guids from annotation table v2.1

Addresses comments.

Comment 44

[Shawn Wilsher :sdwilsh]

(In reply to comment #40)
> Well, yeah, it's a bit scary case, but if it happens we are in a bad shape. How
> would Sync react to a entry without a guid?
I would just try again.  The likelihood of us having two collisions in a row is very low, but we could also just loop until it succeeds.  If you want this to happen, please file a follow-up bug.

Comment 45

[Shawn Wilsher :sdwilsh]

Comment on attachment 494204 [details] [diff] [review]
Part 2 - import bookmark guids from annotation table v2.1

We don't actually need Sync's review, just feedback.  Any issues that fallout can be addressed in a new bug.

Comment 46

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/projects/places/rev/325173c0f7e9
http://hg.mozilla.org/projects/places/rev/60e4b26536ff
http://hg.mozilla.org/projects/places/rev/fa66a8f0f02a
http://hg.mozilla.org/projects/places/rev/7bfe544f55de

Comment 47

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/7bfe544f55de
http://hg.mozilla.org/mozilla-central/rev/fa66a8f0f02a
http://hg.mozilla.org/mozilla-central/rev/60e4b26536ff
http://hg.mozilla.org/mozilla-central/rev/325173c0f7e9