Changed DB to store node positions as Ordinals.

sync/syncable/directory_backing_store.cc

Index: sync/syncable/directory_backing_store.cc
diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc
index 9b8e0dba855edc22128e0cb4aab84d96ec485da5..208f992218bb3ac5e168fb50730e9823b1738f79 100644
--- a/sync/syncable/directory_backing_store.cc
+++ b/sync/syncable/directory_backing_store.cc
@@ -22,6 +22,8 @@
 #include "sql/connection.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
+#include "sync/internal_api/public/base/ordinal.h"

[akalin, 2012/10/02 22:43:01]

i think you can include just node_ordinal.h, and not ordinal.h

+#include "sync/internal_api/public/base/node_ordinal.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/syncable-inl.h"
@@ -39,7 +41,7 @@ static const string::size_type kUpdateStatementBufferSize = 2048;
 
 // Increment this version whenever updating DB tables.
 extern const int32 kCurrentDBVersion;  // Global visibility for our unittest.
-const int32 kCurrentDBVersion = 80;
+const int32 kCurrentDBVersion = 81;
 
 // Iterate over the fields of |entry| and bind each to |statement| for
 // updating.  Returns the number of args bound.
@@ -48,7 +50,16 @@ void BindFields(const EntryKernel& entry,
   int index = 0;
   int i = 0;
   for (i = BEGIN_FIELDS; i < INT64_FIELDS_END; ++i) {
-    statement->BindInt64(index++, entry.ref(static_cast<Int64Field>(i)));
+    // Store SERVER_POSITION_IN_PARENT as a blob for eventual shift
+    // to using ordinals instead of int64s.
+    // See crbug.com/145412

[rlarocque, 2012/10/02 21:48:21]

I think there should be a period here, too.

[akalin, 2012/10/02 22:43:01]

preferably with a space before (so the link doesn't get messed up).

+    if (i == SERVER_POSITION_IN_PARENT) {

[rlarocque, 2012/10/02 21:48:21]

What will we do when SERVER_POSITION_IN_PARENT is no longer an int64?  I don't
think we want this column to live among the ints forever.  

Let's create a new SERVER_POSITION_ORDINAL enum (or whatever it is we decide to
call it) of "type" ORDINAL_FIELD and put the new ordinal column there.  We'll
still need the special-case to populate SERVER_POSITION_IN_PARENT with an int64
value for now, but we'll be able to remove it eventually.

[akalin, 2012/10/02 22:43:01]

I think we should get rid of SERVER_POSITION_IN_PARENT, actually, and do the
int64<->NodeOrdinal conversion whenever we use SERVER_POSITION_IN_PARENT now. 
What do you think?

Since the column list in syncable_columns.h must match the field order, I think
we either need to keep both columns or get rid of SERVER_POSITION_IN_PARENT.

+      std::string ordinal = Int64ToNodeOrdinal(
+          entry.ref(SERVER_POSITION_IN_PARENT)).ToInternalValue();
+      statement->BindBlob(index++, ordinal.data(), ordinal.size());
+    } else {
+      statement->BindInt64(index++, entry.ref(static_cast<Int64Field>(i)));
+    }
   }
   for ( ; i < TIME_FIELDS_END; ++i) {
     statement->BindInt64(index++,
@@ -78,7 +89,24 @@ EntryKernel* UnpackEntry(sql::Statement* statement) {
   DCHECK_EQ(statement->ColumnCount(), static_cast<int>(FIELD_COUNT));
   int i = 0;
   for (i = BEGIN_FIELDS; i < INT64_FIELDS_END; ++i) {
-    kernel->put(static_cast<Int64Field>(i), statement->ColumnInt64(i));
+    // Store SERVER_POSITION_IN_PARENT as a blob for eventual shift
+    // to using ordinals instead of int64s.
+    if(i == SERVER_POSITION_IN_PARENT) {
+      int64 server_position;
+      // Null-valued columns get a default ordinal.
+      if (statement->ColumnType(i) == sql::COLUMN_TYPE_NULL)
+        server_position = NodeOrdinalToInt64((
+            NodeOrdinal::CreateInitialOrdinal()));
+      else {
+        std::string ordinal_blob;
+        DCHECK(statement->ColumnBlobAsString(i, &ordinal_blob));
+        server_position = NodeOrdinalToInt64(NodeOrdinal(ordinal_blob));
+        kernel->put(static_cast<Int64Field>(SERVER_POSITION_IN_PARENT),
+                    server_position);
+      }
+    } else {
+      kernel->put(static_cast<Int64Field>(i), statement->ColumnInt64(i));
+    }
   }
   for ( ; i < TIME_FIELDS_END; ++i) {
     kernel->put(static_cast<TimeField>(i),
@@ -323,6 +351,13 @@ bool DirectoryBackingStore::InitializeTables() {
       version_on_disk = 80;
   }
 
+  // Version 81 changes the server_position_in_parent_field from an int64
+  // to a string.
+  if (version_on_disk == 80) {
+    if (MigrateVersion80To81())
+      version_on_disk = 81;
+  }
+
   // If one of the migrations requested it, drop columns that aren't current.
   // It's only safe to do this after migrating all the way to the current
   // version.
@@ -968,6 +1003,7 @@ bool DirectoryBackingStore::MigrateVersion78To79() {
   SetVersion(79);
   return true;
 }
+
 bool DirectoryBackingStore::MigrateVersion79To80() {
   if (!db_->Execute(
           "ALTER TABLE share_info ADD COLUMN bag_of_chips BLOB"))
@@ -982,6 +1018,144 @@ bool DirectoryBackingStore::MigrateVersion79To80() {
   return true;
 }
 
+namespace {
+// Some definitions to help with the 80->81 migration.
+void Version80ColumnList(std::string& str) {
+  str.append(
+      "metahandle,"
+      "base_version,"
+      "server_version,"
+      "server_position_in_parent,"
+      "local_external_id,"
+      "mtime,"
+      "server_mtime,"
+      "ctime,"
+      "server_ctime,"
+      "id,"
+      "parent_id,"
+      "server_parent_id,"
+      "prev_id,"
+      "next_id,"
+      "is_unsynced,"
+      "is_unapplied_update,"
+      "is_del,"
+      "is_dir,"
+      "server_is_dir,"
+      "server_is_del,"
+      "non_unique_name,"
+      "server_non_unique_name,"
+      "unique_server_tag,"
+      "unique_client_tag,"
+      "specifics,"
+      "server_specifics,"
+      "base_server_specifics");
+}
+
+const int V80_COLUMN_COUNT = 27;
+const int V80_ORDINAL_FIELD = 3;
+
+void Version81ColumnList(std::string& str) {
+  str.append(
+      "metahandle,"
+      "base_version,"
+      "server_version,"
+      "server_ordinal_in_parent,"
+      "local_external_id,"
+      "mtime,"
+      "server_mtime,"
+      "ctime,"
+      "server_ctime,"
+      "id,"
+      "parent_id,"
+      "server_parent_id,"
+      "prev_id,"
+      "next_id,"
+      "is_unsynced,"
+      "is_unapplied_update,"
+      "is_del,"
+      "is_dir,"
+      "server_is_dir,"
+      "server_is_del,"
+      "non_unique_name,"
+      "server_non_unique_name,"
+      "unique_server_tag,"
+      "unique_client_tag,"
+      "specifics,"
+      "server_specifics,"
+      "base_server_specifics");
+}
+
+} //namespace.
+
+bool DirectoryBackingStore::MigrateVersion80To81() {

[akalin, 2012/10/02 22:43:01]

whoa, this is scary.  might be a stupid question, but why can't we do something
like:

ALTER TABLE metas ADD COLUMN server_ordinal_in_parent;
// Loop through table, build ordinals from server_position_in_parent
ALTER TABLE metas DROP COLUMN server_position_in_parent;

? (I guess this would depend on my other comments.)

+  // Version 81 changes server_position_in_parent from a potentially lossy
+  // int64 to an unbounded fidelity ordinal (stored as a blob).
+  // We make the current metas table temporary, create a new table where
+  // server_position_in_parent is of the right type, and transfer values
+  // after converting from int64's to Ordinals.
+  SafeDropTable("temp_metas");
+  if(!db_->Execute("ALTER TABLE metas RENAME TO temp_metas")) {
+    return false;
+  }
+  if(!CreateMetasTable(false))
+    return false;
+
+  std::string old_info_string = "SELECT ";
+  Version80ColumnList(old_info_string);
+  old_info_string.append(" FROM temp_metas");
+
+  std::string new_info_string = "INSERT INTO metas (";
+  Version81ColumnList(new_info_string);
+  new_info_string.append(") VALUES (");
+  for(int i = 0; i < V80_COLUMN_COUNT - 1; ++i) {
+    new_info_string.append("?, ");
+  }
+  new_info_string.append("?)");
+
+  sql::Statement old_info(db_->GetUniqueStatement(
+      old_info_string.c_str()));
+  sql::Statement new_info(db_->GetUniqueStatement(
+      new_info_string.c_str()));
+
+  while(old_info.Step()) {
+    int64 int_ordinal = old_info.ColumnInt(3);
+    std::string new_ordinal =
+        Int64ToNodeOrdinal(int_ordinal).ToInternalValue();
+    //Loop and bind each new column based on the old column type.
+    for(int i = 0; i < V80_COLUMN_COUNT; ++i) {
+      if (i == V80_ORDINAL_FIELD) {
+        new_info.BindBlob(i, new_ordinal.data(), new_ordinal.length());
+        continue;
+      }
+      switch(old_info.ColumnType(i)) {
+        case sql::COLUMN_TYPE_INTEGER:
+          new_info.BindInt64(i, old_info.ColumnInt64(i));
+          break;
+        case sql::COLUMN_TYPE_FLOAT:
+          new_info.BindDouble(i, old_info.ColumnDouble(i));
+          break;
+        case sql::COLUMN_TYPE_TEXT:
+          new_info.BindString(i, old_info.ColumnString(i));
+          break;
+        case sql::COLUMN_TYPE_BLOB:
+          new_info.BindBlob(i,
+                            old_info.ColumnBlob(i),
+                            old_info.ColumnByteLength(i));
+          break;
+        case sql::COLUMN_TYPE_NULL:
+          new_info.BindNull(i);
+      }
+    }
+
+    if(!new_info.Run())
+      return false;
+    new_info.Reset(true);
+  }
+
+  SetVersion(81);
+  return true;
+}
+
 bool DirectoryBackingStore::CreateTables() {
   DVLOG(1) << "First run, creating tables";
   // Create two little tables share_version and share_info