predictors: Version the resource_prefetch_predictor database.

chrome/browser/predictors/resource_prefetch_predictor_tables.cc

 // Copyright 2014 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 "chrome/browser/predictors/resource_prefetch_predictor_tables.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/stringprintf.h"
 #include "content/public/browser/browser_thread.h"
+#include "sql/meta_table.h"
 #include "sql/statement.h"
+#include "sql/transaction.h"
 
 using content::BrowserThread;
 using sql::Statement;
 
 namespace {
 
 using ResourceRow = predictors::ResourcePrefetchPredictorTables::ResourceRow;
 
+const char kMetadataTableName[] = "resource_prefetch_predictor_metadata";
 const char kUrlResourceTableName[] = "resource_prefetch_predictor_url";
 const char kUrlMetadataTableName[] = "resource_prefetch_predictor_url_metadata";
 const char kHostResourceTableName[] = "resource_prefetch_predictor_host";
 const char kHostMetadataTableName[] =
     "resource_prefetch_predictor_host_metadata";
 
+const char kCreateGlobalMetadataStatementTemplate[] =
+    "CREATE TABLE %s ( "
+    "key TEXT, value INTEGER, "
+    "PRIMARY KEY (key))";
+const char kCreateResourceTableStatementTemplate[] =
+    "CREATE TABLE %s ( "
+    "main_page_url TEXT, "
+    "resource_url TEXT, "
+    "proto BLOB, "
+    "PRIMARY KEY(main_page_url, resource_url))";
+const char kCreateMetadataTableStatementTemplate[] =
+    "CREATE TABLE %s ( "
+    "main_page_url TEXT, "
+    "last_visit_time INTEGER, "
+    "PRIMARY KEY(main_page_url))";
+
 const char kInsertResourceTableStatementTemplate[] =
     "INSERT INTO %s (main_page_url, resource_url, proto) VALUES (?,?,?)";
 const char kInsertMetadataTableStatementTemplate[] =
     "INSERT INTO %s (main_page_url, last_visit_time) VALUES (?,?)";
 const char kDeleteStatementTemplate[] = "DELETE FROM %s WHERE main_page_url=?";
 
+// Database version. Always increment it when any change is made to the data
+// schema (including the .proto).

[pasko, 2016/08/26 14:37:18]

is this mandatory? We might be OK with default backfill for optional fields ..
sometimes, right?

For the comment to be useful, it would be better to keep this description closer
to the protobuf definition. How about enum Version { CURRENT_VERSION = 1; } in
the .proto itself?

[Benoit L, 2016/08/26 14:52:39]

I prefer to err on the side of caution.
What about adding a comment in the proto?

+const int kDatabaseVersion = 1;
+
 void BindResourceRowToStatement(const ResourceRow& row,
                                 const std::string& primary_key,
                                 Statement* statement) {
   chrome_browser_predictors::ResourceData proto;
   row.ToProto(&proto);
   int size = proto.ByteSize();
   std::vector<char> proto_buffer(size);
   proto.SerializeToArray(&proto_buffer[0], size);
 
   statement->BindString(0, primary_key);
@@ skipping 380 matching lines @@
     return false;
 
   for (ResourceRows::const_iterator it = data.resources.begin();
        it != data.resources.end(); ++it) {
     if (it->resource_url.spec().length() > kMaxStringLength)
       return false;
   }
   return true;
 }
 
+// static
 bool ResourcePrefetchPredictorTables::DropTablesIfOutdated(
     sql::Connection* db) {
+  int tables_version = 0;
+  if (db->DoesTableExist(kMetadataTableName)) {
+    sql::Statement statement(db->GetUniqueStatement(
+        base::StringPrintf("SELECT value FROM %s WHERE key='version'",
+                           kMetadataTableName)
+            .c_str()));
+    if (statement.Step())
+      tables_version = statement.ColumnInt(0);
+  }
+
   bool success = true;
-  for (const char* table_name :
-       {kUrlResourceTableName, kHostResourceTableName}) {
-    if (db->DoesTableExist(table_name) &&
-        !db->DoesColumnExist(table_name, "proto")) {
-      success &=
-          db->Execute(base::StringPrintf("DROP TABLE %s", table_name).c_str());
+  // Too new is also a problem.
+  bool incompatible_version = tables_version != kDatabaseVersion;
+
+  if (incompatible_version) {
+    for (const char* table_name :
+         {kMetadataTableName, kUrlResourceTableName, kHostResourceTableName,
+          kUrlMetadataTableName, kHostMetadataTableName}) {
+      success &= db->Execute(
+          base::StringPrintf("DROP TABLE IF EXISTS %s", table_name).c_str());
     }
   }
+
+  if (incompatible_version) {
+    success &=
+        db->Execute(base::StringPrintf(kCreateGlobalMetadataStatementTemplate,
+                                       kMetadataTableName)
+                        .c_str());
+
+    sql::Statement statement(db->GetUniqueStatement(
+        base::StringPrintf(
+            "INSERT OR REPLACE INTO %s (key,value) VALUES ('version',%d)",
+            kMetadataTableName, kDatabaseVersion)
+            .c_str()));
+    success &= statement.Run();
+  }
+
   return success;
 }
 
 void ResourcePrefetchPredictorTables::CreateTableIfNonExistent() {
   DCHECK_CURRENTLY_ON(BrowserThread::DB);
   if (CantAccessDatabase())
     return;
 
-  const char resource_table_creator[] =
-      "CREATE TABLE %s ( "
-      "main_page_url TEXT, "
-      "resource_url TEXT, "
-      "proto BLOB, "
-      "PRIMARY KEY(main_page_url, resource_url))";
-  const char* metadata_table_creator =
-      "CREATE TABLE %s ( "
-      "main_page_url TEXT, "
-      "last_visit_time INTEGER, "
-      "PRIMARY KEY(main_page_url))";
+  // Database initialization is all-or-nothing.
+  sql::Connection* db = DB();
+  sql::Transaction transaction{db};
+  bool success = transaction.Begin();
 
-  sql::Connection* db = DB();
-  bool success = DropTablesIfOutdated(db) &&
-                 (db->DoesTableExist(kUrlResourceTableName) ||
-                  db->Execute(base::StringPrintf(resource_table_creator,
-                                                 kUrlResourceTableName)
-                                  .c_str())) &&
-                 (db->DoesTableExist(kUrlMetadataTableName) ||
-                  db->Execute(base::StringPrintf(metadata_table_creator,
-                                                 kUrlMetadataTableName)
-                                  .c_str())) &&
-                 (db->DoesTableExist(kHostResourceTableName) ||
-                  db->Execute(base::StringPrintf(resource_table_creator,
-                                                 kHostResourceTableName)
-                                  .c_str())) &&
-                 (db->DoesTableExist(kHostMetadataTableName) ||
-                  db->Execute(base::StringPrintf(metadata_table_creator,
-                                                 kHostMetadataTableName)
-                                  .c_str()));
+  success &= DropTablesIfOutdated(db);
+
+  success &=
+      (db->DoesTableExist(kUrlResourceTableName) ||

[pasko, 2016/08/26 14:37:18]

what if we decided to drop the table because its version is old, but the table
still exists because we did not commit the transaction? I assume this check
would return true and the table is not created.

[Benoit L, 2016/08/26 14:52:39]

We are within the transaction, so it's fine.

+       db->Execute(base::StringPrintf(kCreateResourceTableStatementTemplate,
+                                      kUrlResourceTableName)
+                       .c_str())) &&
+      (db->DoesTableExist(kUrlMetadataTableName) ||
+       db->Execute(base::StringPrintf(kCreateMetadataTableStatementTemplate,
+                                      kUrlMetadataTableName)
+                       .c_str())) &&
+      (db->DoesTableExist(kHostResourceTableName) ||
+       db->Execute(base::StringPrintf(kCreateResourceTableStatementTemplate,
+                                      kHostResourceTableName)
+                       .c_str())) &&
+      (db->DoesTableExist(kHostMetadataTableName) ||
+       db->Execute(base::StringPrintf(kCreateMetadataTableStatementTemplate,
+                                      kHostMetadataTableName)
+                       .c_str()));
+
+  if (success) {
+    success = transaction.Commit();
+  } else {
+    transaction.Rollback();
+  }

[pasko, 2016/08/26 14:37:18]

nit: I think chromium style suggests no braces in this case. I am not a fan of
this style, but prefer consistency

[Benoit L, 2016/08/26 14:52:39]

Done.

 
   if (!success)
     ResetDB();
 }
 
 void ResourcePrefetchPredictorTables::LogDatabaseStats()  {
   DCHECK_CURRENTLY_ON(BrowserThread::DB);
   if (CantAccessDatabase())
     return;
 
@@ skipping 69 matching lines @@
 }
 
 Statement* ResourcePrefetchPredictorTables::GetHostMetadataUpdateStatement() {
   return new Statement(DB()->GetCachedStatement(
       SQL_FROM_HERE, base::StringPrintf(kInsertMetadataTableStatementTemplate,
                                         kHostMetadataTableName)
                          .c_str()));
 }
 
 }  // namespace predictors