predictors: Version the resource_prefetch_predictor database.

chrome/browser/predictors/resource_prefetch_predictor_tables.cc

Index: chrome/browser/predictors/resource_prefetch_predictor_tables.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_tables.cc b/chrome/browser/predictors/resource_prefetch_predictor_tables.cc
index a3dd8c1e429adf005511c75f364ad9f0f19045ca..86c687a5c0b487e9103112db37d724868759111f 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_tables.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_tables.cc
@@ -14,7 +14,9 @@
 #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;
@@ -23,18 +25,39 @@ 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) {
@@ -435,17 +458,46 @@ bool ResourcePrefetchPredictorTables::StringsAreSmallerThanDBLimit(
   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;
 }
 
@@ -454,36 +506,36 @@ void ResourcePrefetchPredictorTables::CreateTableIfNonExistent() {
   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();
-  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()));
+  sql::Transaction transaction{db};
+  bool success = transaction.Begin();
+
+  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();