[sql] Database recovery system for Shortcuts.

sql/recovery.cc

Index: sql/recovery.cc
diff --git a/sql/recovery.cc b/sql/recovery.cc
index 7b9ca9f6fee65724a310e1f34620b97faf60bc7b..9e6932777f21e395822f9c0c27c6936fe59811c6 100644
--- a/sql/recovery.cc
+++ b/sql/recovery.cc
@@ -84,6 +84,31 @@ enum RecoveryEventType {
   // No version key in recovery meta table.
   RECOVERY_FAILED_META_NO_VERSION,
 
+  // Automatically recovered entire database successfully.

[Mark P, 2016/04/07 23:09:43]

This comment sounds like it could equally apply to the earlier enum
RECOVERY_SUCCESS_AUTORECOVER.  Please figure out a way to make their differences
clearer.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

The earlier applies to a specific table, this applies to the entire database. 
So would you prefer to rename the earlier ones to reflect their specific scope?

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

No, I thought you were being sloppy about database versus table here (as the
autorecovery code is currently only used for a single table).  I was wrong. 
It's fine as is.

+  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.

[Mark P, 2016/04/08 20:12:06]

Please clearly state these are subcategories of
RECOVERY_FAILED_AUTORECOVERDB_SCHEMA.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Done.

+  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,
 };
@@ -509,4 +534,161 @@ bool Recovery::GetMetaVersionNumber(int* version) {
   return true;
 }
 
+namespace {
+
+// Collect each SQL statement from [corrupt.sqlite_master] which starts with
+// |prefix|, and apply it to [main].  Skip 'sqlite_sequence', which will be

[Mark P, 2016/04/07 23:09:43]

This comment is too broad.  Below you explicitly assume the prefix is the right
amount of text so if you add a "main." right after it, it makes sense as a sql
statement.
SELECT foo from BAR;
->
SELmain.ECT foo from BAR;
Ugh, you don't mean this.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

I agree that the example case isn't what I want, but I'm not sure what to do
about it.  The prefix has to be some sort of valid SQL prefix thingie, but I'm
not sure what to call it, and I'm not going to be writing a parser to validate
it.  I guess I can assert that it ends in a space, but even that doesn't mean
much, since I'm using string manipulation to dynamically construct statements.

+// created on demand by SQLite if any tables use AUTOINCREMENT.

[Mark P, 2016/04/07 23:09:43]

This comment should mention the meaning of the return value.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Done.

+bool SchemaCopyHelper(Connection* db, const char* prefix) {
+  const size_t prefix_len = strlen(prefix);
+  sql::Statement s(db->GetUniqueStatement(
+      "SELECT sql FROM corrupt.sqlite_master "
+      "WHERE substr(sql, 1, ?)=? AND name<>'sqlite_sequence'"));

[Mark P, 2016/04/07 23:09:43]

What happens if prefix_length > length(sql)?  What does substr return in this
case?
The sqlite3 documentation I found doesn't define this.
https://www.sqlite.org/lang_corefunc.html
Perhaps you want to add another another condition ensuring that sql is long
enough?
(Though I can't imagine this causing any problems unless substr() actually fails
on this input, as the equality test still applies)

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

I think I'll just rewrite this as a full table scan and do the filtering in C++.
 It won't change the efficiency at all, and I expect it will be approximately
the same weight in code.

+  s.BindInt(0, prefix_len);
+  s.BindCString(1, prefix);
+
+  // TODO(shess): In case of row duplication when balancing a [sqlite_master]
+  // btree, this could inject IF NOT EXISTS.  I suspect the occurance is too low
+  // to be worth the complexity, though.

[Mark P, 2016/04/07 23:09:43]

Can you expand this slightly
to be worth complexity to skip statements with IF NOT EXISTS?
to be worth complexity to identify IF NOT EXISTS statement and then ...?

Also, if you don't plan to do it, please don't label it as a TODO. :-P

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

I think I'll remove the comment entirely.  A common corruption is having a
transaction broken when a page is being rebalanced, which can lead to seeing the
same data in multiple places.  This could only happen to the sqlite_master btree
if a transaction was broken while making a schema change, which is already
unlikely, but it could only be successfully coded against if the result was a
valid schema, which is probably even more unlikely.

OK, and it just occurred to me that I could add a DISTINCT to the selection
statement and ignore the problem entirely.

+

[Mark P, 2016/04/07 23:09:43]

This blank line here feels unnecessary (but then I could be misunderstanding the
scope of the TODO comment above.)

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Done.

+  while (s.Step()) {

[Mark P, 2016/04/07 23:09:43]

Should all these executions be wrapped in a transaction?

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Recovery happens to a temporary database, which is only committed to the main
database in Recovered().  So transactions would only be necessary insofar as the
(local) caller needs to do controlled rollback of one approach in order to try
another approach.  I don't think that can really come up, here, mostly because I
can't think of a second simple approach (either the schema will work for this
kind of recovery, or it won't, and the client shouldn't be calling this code if
the schema is too complex or subtle).

+    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) {
+  scoped_ptr<sql::Recovery> recovery = sql::Recovery::Begin(db, db_path);
+  if (!recovery) {
+    // TODO(shess): If recovery can't even get started, consider Raze() or
+    // Delete().
+    //
+    // Reviewing histograms, the majority of Begin() failures are in the ATTACH
+    // (788k/month).  This may happen for SQLITE_CANTOPEN and SQLITE_NOTADB
+    // cases which cannot be processed using writable_schema.  The second
+    // biggest case is lack of vtable support on iOS, which is fixed but not yet
+    // in the channel (175k/month).  Lastly, there were 3 failures in a month

[Mark P, 2016/04/08 20:12:06]

Please don't put non-normalized numbers in comments that will become out of date
over time.
e.g., say something like X failures per million database opens in M-49.

Also, if you're referencing an issues that fixed, please mention when, e.g.,
fixed for M-50.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Maybe I'll keep the comment in my local client, since it's mostly just to inform
my thinking about whether this is safe to flip to Raze() or Delete() or
something.  The specific numbers aren't interesting, I was just thinking "BIG",
"Less big", and "trivial or maybe noise".

+    // opening the temporary table - in theory, that may work later, but in
+    // practice 3 in 1M might be a reasonable false-positive rate.

[Mark P, 2016/04/08 20:12:06]

I think this last sentence isn't worth mentioning anyway.  Things that occur
three times in a month aren't worth thinking about, let alone documenting in a
place that will force thinking about it to cross other people's mind.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Acknowledged.

+    RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_BEGIN);
+    db->Poison();

[Mark P, 2016/04/08 20:12:06]

In the header comment, you write the function will poison the database so it
"will return errors until the handle is re-opened."  This implies the database
is closed.  Yet, you don't close it here.  The comment by the definition for
Poison says
  // This is intended as an alternative to Close() in error callbacks.
  // Close() should still be called at some point.
so shouldn't you close it...?

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

The originating caller's Open()/Close() calls should balance out.  This call can
happen deep in a stack of calls in a case handling an exceptional error, making
it impractical for the client to track the Close() status.  So Poison() poisons
the sql::Connection handle, but logically the handle is still open.

For instance, warnings are thrown if you try to run Execute() on a handle which
was never opened, or which has been closed.  But if the handle was poisoned,
then the Execute() will fail without warnings.

+    return;
+  }
+
+#if DCHECK_IS_ON()

[Mark P, 2016/04/08 20:12:06]

I don't understand this block of code.  That said, I am willing to approve the
changelist without any changes to this block; I understand that it's okay that I
don't have the background for it.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

fts3 is a virtual table which we used to use in history, and remains compiled
into Chrome for WebSQL.  I don't think anyone in the browser process is using
it.  AutorecoverTable() fails on fts3 tables by successfully dropping all of the
data, so I want to prevent anyone from calling this code on an fts3 virtual
table.

+  // 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() trust the INTEGER tag
+  // when setting up the recover vtable, so those rows get dropped.  Setting
+  // that column to ANY may work, but maybe it's better to fail hard until
+  // someone needs to recover a virtual table.
+  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));

[Mark P, 2016/04/08 20:12:06]

Header talks about poisoning; why are you using a rollback here?  Should you
poison after this rollback?
ditto multiple places below

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

See comment I made about the RecoverDatabaseOrRaze() header-file comment.  I
intend all of these to because Unrecoverable() (leading to clearing the database
data).

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

[Mark P, 2016/04/08 20:12:06]

AND != corrupt.sqlite_sequence ?

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

sqlite_master is implicitly main.sqlite_master .  SQLite doesn't
interpret/interpolate the columns, so selecting from corrupt.sqlite_master would
still have 'sqlite_sequence' as the name and the sql would not have db.table
naming.

[I think?  OMG, my precious brain!  Yes, AFAICT from testing it.]

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

I'm sorry for making your brain explode. ;-)

+    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/08 20:12:06]

Do you have a sense of whether this will be different in some cases?  Or are you
nervous, which is why you left it like vacuum.c?

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

I do not fully understand why vacuum.c uses the rootpage like this.  SQLite
creates tables with a root page even if empty, and I believe it could also
validly use a null root page.  Most likely the assumption is that you created a
table, so you'll populate it, though I've certainly seen that assumption broken
w/in Chromium.  Testing 'CREATE VIRTUAL TABLE' seems safer to me, since it can't
accidentally match 'CREATE TABLE' or any other sort of faux table.

+  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) {
+  switch (extended_error & 0xFF) {

[Mark P, 2016/04/07 23:09:43]

You do the same & 0xFF in connection.cc as well.  Consider making this bitmask a
constant somewhere.  Or at least comment it better (in both places), maybe
something as simple as
// Mask down to the primary result code.

[Scott Hess - ex-Googler, 2016/04/15 00:38:15]

Copied language from connection.cc.  Unfortunately SQLite doesn't provide a
really distinct name for the basic error codes, so I'm phrasing in terms of
removing extended error codes.

+    case SQLITE_CANTOPEN:
+    case SQLITE_NOTADB:
+    case SQLITE_CORRUPT:
+      return true;
+    default:
+      return false;
+  }
+}
+
 }  // namespace sql