[sql] Database recovery system for Shortcuts.

sql/recovery.cc

Index: sql/recovery.cc
diff --git a/sql/recovery.cc b/sql/recovery.cc
index 92c7f875e152f9d234f1d00ab6c4b23f6376b151..1a8fe7bada4da8362d9d87789caf03f71906f393 100644
--- a/sql/recovery.cc
+++ b/sql/recovery.cc
@@ -84,6 +84,32 @@ enum RecoveryEventType {
   // No version key in recovery meta table.
   RECOVERY_FAILED_META_NO_VERSION,
 
+  // Automatically recovered entire database successfully.
+  RECOVERY_SUCCESS_AUTORECOVERDB,
+
+  // Database was so broken recovery couldn't be entered.
+  RECOVERY_FAILED_AUTORECOVERDB_BEGIN,
+
+  // Failed to copy schema to autorecover db.
+  RECOVERY_FAILED_AUTORECOVERDB_SCHEMA,
+
+  // Distinguish failure in querying schema from failure in creating schema.
+  // These sum to RECOVERY_FAILED_AUTORECOVERDB_SCHEMA.
+  RECOVERY_FAILED_AUTORECOVERDB_SCHEMACREATE,
+  RECOVERY_FAILED_AUTORECOVERDB_SCHEMASELECT,
+
+  // Failed querying tables to recover.  Should be impossible.
+  RECOVERY_FAILED_AUTORECOVERDB_NAMESELECT,
+
+  // Failed to recover an individual table.
+  RECOVERY_FAILED_AUTORECOVERDB_TABLE,
+
+  // Failed to recover [sqlite_sequence] table.
+  RECOVERY_FAILED_AUTORECOVERDB_SEQUENCE,
+
+  // Failed to recover triggers or views or virtual tables.
+  RECOVERY_FAILED_AUTORECOVERDB_AUX,
+
   // Always keep this at the end.
   RECOVERY_EVENT_MAX,
 };
@@ -508,4 +534,180 @@ bool Recovery::GetMetaVersionNumber(int* version) {
   return true;
 }
 
+namespace {
+
+// Collect statements from [corrupt.sqlite_master.sql] which start with
+// |prefix|, and apply them to [main].  Skip any table named 'sqlite_sequence',
+// which is created on demand by SQLite if any tables use AUTOINCREMENT.

[Mark P, 2016/04/19 23:34:28]

You still should allude to this limitation here, if only to say something like
|prefix| must be the beginning of valid SQL string so that we can insert "main."
after it in SQL commands that start with prefix and have the result be another
valid SQL command.

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Done.

I'm reluctant to get too elaborate with the comment, because at some point it
will just be an English version of the precise series of steps the code
implements.

+//
+// Returns true if all of the matching items were created in the main database.
+// Returns false if an item fails on creation, or if the corrupt database schema
+// cannot be queried.
+bool SchemaCopyHelper(Connection* db, const char* prefix) {
+  const size_t prefix_len = strlen(prefix);
+  DCHECK_EQ(' ', prefix[prefix_len-1]);
+  sql::Statement s(db->GetUniqueStatement(
+      "SELECT DISTINCT sql FROM corrupt.sqlite_master "
+      "WHERE substr(sql, 1, ?)=? AND name<>'sqlite_sequence'"));

[Mark P, 2016/04/19 23:34:28]

You didn't do this rewrite you promised.

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Was thinking out loud, earlier.  Done.

+  s.BindInt(0, prefix_len);
+  s.BindCString(1, prefix);
+
+  while (s.Step()) {

[Mark P, 2016/04/19 23:34:28]

You're not concerned with having a failed attempt leave additional commands in
corrupt.sqlite_master?  Or does that not happen?

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

In case of failure the recovery database [main] does not replace the original
database [corrupt].

+    std::string sql = s.ColumnString(0);
+    sql.insert(prefix_len, "main.");
+    if (!db->Execute(sql.c_str())) {
+      RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_SCHEMACREATE);
+      return false;
+    }
+  }
+  if (!s.Succeeded()) {
+    RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_SCHEMASELECT);
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+// This method is derived from SQLite's vacuum.c.  VACUUM operates very
+// similarily, creating a new database, populating the schema, then copying the
+// data.
+//
+// TODO(shess): This conservatively uses Rollback() rather than Unrecoverable().
+// With Rollback(), it is expected that the database will continue to generate
+// errors.  Change the failure cases to Unrecoverable() if/when histogram
+// results indicate that everything is working reasonably.
+//
+// static
+void Recovery::RecoverDatabaseOrRaze(Connection* db,
+                                     const base::FilePath& db_path) {
+  std::unique_ptr<sql::Recovery> recovery = sql::Recovery::Begin(db, db_path);
+  if (!recovery) {
+    // TODO(shess): If recovery can't even get started, Raze() or Delete().
+    RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_BEGIN);
+    db->Poison();

[Mark P, 2016/04/19 23:34:28]

In this case, I think it'd be better in the header to say, after you say that it
poisons the database "(though the handle technically remains open)" before
saying "will return errors until the handle is re-opened."

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Done.

[Scott Hess - ex-Googler, 2016/06/27 21:35:38]

Header-comment-change really done, this time.

+    return;
+  }
+
+#if DCHECK_IS_ON()
+  // This code silently fails to recover fts3 virtual tables.  At this time no
+  // browser database contain fts3 tables.  Just to be safe, complain loudly if
+  // the database contains virtual tables.
+  //
+  // fts3 has an [x_segdir] table containing a column [end_block INTEGER].  But
+  // it actually stores either an integer or a text containing a pair of
+  // integers separated by a space.  AutoRecoverTable() trusts the INTEGER tag
+  // when setting up the recover vtable, so those rows get dropped.  Setting
+  // that column to ANY may work.
+  if (db->is_open()) {
+    sql::Statement s(db->GetUniqueStatement(
+        "SELECT 1 FROM sqlite_master WHERE sql LIKE 'CREATE VIRTUAL TABLE %'"));
+    DCHECK(!s.Step()) << "Recovery of virtual tables not supported";
+  }
+#endif
+
+  // TODO(shess): vacuum.c turns off checks and foreign keys.
+
+  // TODO(shess): vacuum.c turns synchronous=OFF for the target.  I do not fully
+  // understand this, as the temporary db should not have a journal file at all.
+  // Perhaps it does in case of cache spill?
+
+  // Copy table schema from [corrupt] to [main].
+  if (!SchemaCopyHelper(recovery->db(), "CREATE TABLE ") ||
+      !SchemaCopyHelper(recovery->db(), "CREATE INDEX ") ||
+      !SchemaCopyHelper(recovery->db(), "CREATE UNIQUE INDEX ")) {
+    RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_SCHEMA);
+    Recovery::Rollback(std::move(recovery));
+    return;
+  }
+
+  // Run auto-recover against each table, skipping the sequence table.  This is
+  // necessary because earlier table recovery may populate the sequence table,
+  // then copying the sequence table would create duplicates.
+  {
+    sql::Statement s(recovery->db()->GetUniqueStatement(
+        "SELECT name FROM sqlite_master WHERE sql LIKE 'CREATE TABLE %' "
+        "AND name!='sqlite_sequence'"));
+    while (s.Step()) {
+      const std::string name = s.ColumnString(0);
+      size_t rows_recovered;
+      if (!recovery->AutoRecoverTable(name.c_str(), &rows_recovered)) {
+        RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_TABLE);
+        Recovery::Rollback(std::move(recovery));
+        return;
+      }
+    }
+    if (!s.Succeeded()) {
+      RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NAMESELECT);
+      Recovery::Rollback(std::move(recovery));
+      return;
+    }
+  }
+
+  // Overwrite any sequences created.
+  if (recovery->db()->DoesTableExist("corrupt.sqlite_sequence")) {
+    ignore_result(recovery->db()->Execute("DELETE FROM main.sqlite_sequence"));
+    size_t rows_recovered;
+    if (!recovery->AutoRecoverTable("sqlite_sequence", &rows_recovered)) {
+      RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_SEQUENCE);
+      Recovery::Rollback(std::move(recovery));
+      return;
+    }
+  }
+
+  // Copy triggers, views, and virtual tables directly to sqlite_master.  Any
+  // tables they refer to should already exist.
+  // TODO(shess): The rootpage=0 test is from vacuum.c.  Consider instead:
+  //   sql LIKE "CREATE VIRTUAL TABLE %"

[Mark P, 2016/04/19 23:34:28]

If it seems safer to you, why don't you do it (and comment on why you deviate
from vacuum.c in this case)?

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

Assuming my reasoning is correct, then the DCHECK_IS_ON() section above implies
that virtual tables shouldn't work at all, so I'll just remove the part I
believe to be associated with virtual tables.

I'm leaving the DCHECK_IS_ON() section in place, though.  My experience is that
error-handling code sometimes does not have good testing coverage, so I'd rather
it actively failed in development rather than falsely succeeding.

+  char kCreateMetaItems[] =
+      "INSERT INTO main.sqlite_master "
+      "SELECT type, name, tbl_name, rootpage, sql "
+      "FROM corrupt.sqlite_master "
+      "WHERE type='view' OR type='trigger' OR (type='table' AND rootpage=0)";
+  if (!recovery->db()->Execute(kCreateMetaItems)) {
+    RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_AUX);
+    Recovery::Rollback(std::move(recovery));
+    return;
+  }
+
+  RecordRecoveryEvent(RECOVERY_SUCCESS_AUTORECOVERDB);
+  ignore_result(Recovery::Recovered(std::move(recovery)));
+}
+
+// static
+bool Recovery::ShouldRecoverOrRaze(int extended_error) {
+  // Trim extended error codes.
+  int error = extended_error & 0xFF;
+  switch (error) {
+    case SQLITE_CANTOPEN:
+      // SQLITE_CANTOPEN is associated with an entirely broken file (for
+      // instance a symlink to a non-existent path, or the file is a directory).

[Mark P, 2016/04/19 23:34:28]

How is this recoverable?  This comment implies you should be returning false.

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

I intended this for the delete option, which you asked me to remove, so I'll
remove it.

+      return true;
+
+    case SQLITE_NOTADB:
+      // SQLITE_NOTADB happens if the SQLite header is broken.  Some versions of
+      // SQLite return this where other versions return SQLITE_CORRUPT.

[Mark P, 2016/04/19 23:34:28]

How is this likely to be recoverable?

[Scott Hess - ex-Googler, 2016/05/13 21:24:36]

In the case where it was incorrectly returning SQLITE_NOTADB instead of
SQLITE_CORRUPT, it would be recoverable.  For the version of SQLite currently
checked into Chromium's third_party repo, I think SQLITE_NOTADB is never
recoverable, but some iOS versions of SQLite are old enough to hit this.

[Scott Hess - ex-Googler, 2016/06/27 21:35:38]

Adjusted the comment a bit more.  Long-term, this case will lead to recovery for
some versions of SQLite, and to deletion for others.  I don't think it would be
productive to isolate which versions of SQLite have the different return code,
because the only change to the worst-case outcome is that the handle is poisoned
(which is actually preferable, since SQLITE_NOTADB will never resolve itself).

+      return true;
+
+    case SQLITE_CORRUPT:
+      // SQLITE_CORRUPT generally means that the database is readable as a
+      // SQLite database, but some inconsistency has been detected by SQLite.
+      // In many cases the inconsistency is relatively trivial, such as if an
+      // index refers to a row which was deleted, in which case most or even all
+      // of the data can be recovered.  This can also be reported if parts of
+      // the file have been overwritten with garbage data, in which recovery
+      // should be able to recover partial data.
+      return true;
+
+      // TODO(shess): Possible future options for automated fixing:
+      // - SQLITE_PERM - permissions could be fixed.
+      // - SQLITE_READONLY - permissions could be fixed.
+      // - SQLITE_IOERR - rewrite using new blocks.
+      // - SQLITE_FULL - recover in memory and rewrite subset of data.
+
+    default:
+      return false;
+  }
+}
+
 }  // namespace sql