Changed DB to store node positions as Ordinals.

sync/syncable/directory_backing_store.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/syncable/directory_backing_store.h"
 
 #include "build/build_config.h"
 
 #include <limits>
 
 #include "base/base64.h"
 #include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/hash_tables.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "sql/connection.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/syncable-inl.h"
 #include "sync/syncable/syncable_columns.h"
 #include "sync/util/time.h"
+#include "sync/internal_api/public/base/ordinal.h"
+#include "sync/internal_api/public/base/node_ordinal.h"
 
 using std::string;
 
 namespace syncer {
 namespace syncable {
 
 // This just has to be big enough to hold an UPDATE or INSERT statement that
 // modifies all the columns in the entry table.
 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.
 void BindFields(const EntryKernel& entry,
                 sql::Statement* statement) {
   int index = 0;
   int i = 0;
   for (i = BEGIN_FIELDS; i < INT64_FIELDS_END; ++i) {
+    // Store SERVER_POSITION_IN_PARENT as a blob for eventual shift
+    // to using ordinals instead of int64's

[rlarocque, 2012/09/29 01:13:42]

nit: put period after comment.

Also, you might want to reference the bug number here.

[vishwath, 2012/10/01 20:39:12]

Done.

+    if(i == SERVER_POSITION_IN_PARENT) {

[rlarocque, 2012/09/29 01:13:42]

nit: space between if and parenthesis.

[vishwath, 2012/10/01 20:39:12]

Done.

+      std::string ordinal = Int64ToNodeOrdinal(
+          entry.ref(SERVER_POSITION_IN_PARENT)).ToInternalValue();
+      statement->BindBlob(index++, ordinal.data(), ordinal.size());
+      continue;

[rlarocque, 2012/09/29 01:13:42]

I'd prefer an 'else' branch to 'continue'.

[vishwath, 2012/10/01 20:39:12]

Done.

+    }
     statement->BindInt64(index++, entry.ref(static_cast<Int64Field>(i)));
   }
   for ( ; i < TIME_FIELDS_END; ++i) {
     statement->BindInt64(index++,
                          TimeToProtoTime(
                              entry.ref(static_cast<TimeField>(i))));
   }
   for ( ; i < ID_FIELDS_END; ++i) {
     statement->BindString(index++, entry.ref(static_cast<IdField>(i)).s_);
   }
@@ skipping 10 matching lines @@
   }
 }
 
 // The caller owns the returned EntryKernel*.  Assumes the statement currently
 // points to a valid row in the metas table.
 EntryKernel* UnpackEntry(sql::Statement* statement) {
   EntryKernel* kernel = new EntryKernel();
   DCHECK_EQ(statement->ColumnCount(), static_cast<int>(FIELD_COUNT));
   int i = 0;
   for (i = BEGIN_FIELDS; i < INT64_FIELDS_END; ++i) {
+    // Store SERVER_POSITION_IN_PARENT as a string for eventual shift
+    // to using ordinals instead of int64's
+    if(i == SERVER_POSITION_IN_PARENT &&
+       statement->ColumnType(i) == sql::COLUMN_TYPE_BLOB) {

[rlarocque, 2012/09/29 01:13:42]

We shouldn't get here unless the database is migrated, right?  

Maybe the check for ColumnType(i) should be a DCHECK_EQ()?  

Would the sql functions DCHECK on you if it wasn't a blob?  Maybe you should let
them catch the error failure.

+      std::string ordinal_blob;
+      DCHECK(statement->ColumnBlobAsString(i, &ordinal_blob));
+      int64 server_position = NodeOrdinalToInt64(
+          NodeOrdinal(ordinal_blob));
+      kernel->put(static_cast<Int64Field>(i), server_position);

[rlarocque, 2012/09/29 01:13:42]

nit: I'd prefer that the first argument to put be SERVER_POSITION_IN_PARENT
because I think that's more readable, though I admit it doesn't make much
difference either way.

[vishwath, 2012/10/01 20:39:12]

Done.

+      continue;

[rlarocque, 2012/09/29 01:13:42]

I'd prefer an else branch here.

[vishwath, 2012/10/01 20:39:12]

Done.

+    }
     kernel->put(static_cast<Int64Field>(i), statement->ColumnInt64(i));
   }
   for ( ; i < TIME_FIELDS_END; ++i) {
     kernel->put(static_cast<TimeField>(i),
                 ProtoTimeToTime(statement->ColumnInt64(i)));
   }
   for ( ; i < ID_FIELDS_END; ++i) {
     kernel->mutable_ref(static_cast<IdField>(i)).s_ =
         statement->ColumnString(i);
   }
@@ skipping 225 matching lines @@
     if (MigrateVersion78To79())
       version_on_disk = 79;
   }
 
   // Version 80 migration is adding the bag_of_chips column.
   if (version_on_disk == 79) {
     if (MigrateVersion79To80())
       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.
   if (version_on_disk == kCurrentDBVersion && needs_column_refresh_) {
     if (!RefreshColumns())
       version_on_disk = 0;
   }
 
   // A final, alternative catch-all migration to simply re-sync everything.
   //
@@ skipping 625 matching lines @@
   // Some users are stuck with a DB that causes them to reuse existing IDs.  We
   // perform this one-time fixup on all users to help the few that are stuck.
   // See crbug.com/142987 for details.
   if (!db_->Execute(
           "UPDATE share_info SET next_id = next_id - 65536")) {
     return false;
   }
   SetVersion(79);
   return true;
 }
+
 bool DirectoryBackingStore::MigrateVersion79To80() {
   if (!db_->Execute(
           "ALTER TABLE share_info ADD COLUMN bag_of_chips BLOB"))
     return false;
   sql::Statement update(db_->GetUniqueStatement(
           "UPDATE share_info SET bag_of_chips = ?"));
   // An empty message is serialized to an empty string.
   update.BindBlob(0, NULL, 0);
   if (!update.Run())
     return false;
   SetVersion(80);
   return true;
 }
 
+bool DirectoryBackingStore::MigrateVersion80To81() {
+  // Version 81 changes server_position_in_parent from a potentially lossy
+  // bigint to an unbounded fidelity ordinal (stored as a blob)

[rlarocque, 2012/09/29 01:13:42]

I wouldn't call it a bigint, that term could be overloaded.  Let's just say it's
an int64.

[vishwath, 2012/10/01 20:39:12]

Done.

+  // 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");

[rlarocque, 2012/09/29 01:13:42]

Is there a transaction open at this point?  If not, we should open one.  This
could do a lot of damage if we crash when migration is half done.

[vishwath, 2012/10/01 20:39:12]

I think all the Migrate*() functions rely on the calling functions to set the
transaction up (either InitializeTables() or the appropriate unittest). 
On 2012/09/29 01:13:42, rlarocque wrote:

+  if(!db_->Execute(
+         "ALTER TABLE metas RENAME TO temp_metas")) {
+    return false;
+  }
+  if(!CreateMetasTable(false))
+    return false;
+
+  std::string old_info_string = "SELECT ";

[rlarocque, 2012/09/29 01:13:42]

I was hoping we could perform the migration without forcing every value in the
database to round-trip through C++.

This might be naive, but here's what I had in mind:
1. Rename SERVER_POSITION_IN_PARENT to OLD_SERVER_POSITION_IN_PARENT.
2. Add a new SERVER_POSITION_IN_PARENT column of type BLOB
3. Extract pairs of (metahandle (ie. primary key),
OLD_SERVER_POSITION_IN_PARENT)
4. Perform the conversion
5. Insert the data into SERVER_POSITION_IN_PARENT
6. Have RefreshColumns() clean up the old, temporary column.

[vishwath, 2012/10/01 20:39:12]

That'a actually the way I'd prefer to do it as well. Unfortunately, sqlite does
not have a column renaming command (alter table is far more restrictive) so it
seems we're forced to do this round about approach.

On 2012/09/29 01:13:42, rlarocque wrote:

+  AppendColumnList(&old_info_string);
+  old_info_string.append(" FROM temp_metas");
+
+  std::string new_info_string = "INSERT INTO metas (";
+  AppendColumnList(&new_info_string);
+  new_info_string.append(") VALUES (");
+  for(int i = BEGIN_FIELDS; i < BEGIN_FIELDS + FIELD_COUNT - 1; ++i) {

[rlarocque, 2012/09/29 01:13:42]

I'd really like to avoid this sort of thing, though I have an unusually strong
dislike of code that pieces together SQL queries through string manipulation. 
Others may not share my disapproval.

[vishwath, 2012/10/01 20:39:12]

I see the inherent vulnerabilities in misusing this approach, so if you feel
strongly enough about it I can do this the hard way.

On 2012/09/29 01:13:42, rlarocque wrote:

+    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 < FIELD_COUNT; ++i) {
+      if (i == SERVER_POSITION_IN_PARENT) {
+        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
   if (!db_->Execute(
           "CREATE TABLE share_version ("
           "id VARCHAR(128) primary key, data INT)")) {
     return false;
   }
 
   {
@@ skipping 158 matching lines @@
     bool prev_exists = (ids_set.find(entry->ref(PREV_ID).value()) != end);
     bool parent_exists = (ids_set.find(entry->ref(PARENT_ID).value()) != end);
     bool next_exists = (ids_set.find(entry->ref(NEXT_ID).value()) != end);
     is_ok = is_ok && prev_exists && parent_exists && next_exists;
   }
   return is_ok;
 }
 
 }  // namespace syncable
 }  // namespace syncer