[sql] Database recovery system for Shortcuts.

sql/recovery.h

Index: sql/recovery.h
diff --git a/sql/recovery.h b/sql/recovery.h
index af99108192908593db1ef4844086324609f35a2b..9ff2bed3abccafb9aff5f302f45cae6432416398 100644
--- a/sql/recovery.h
+++ b/sql/recovery.h
@@ -149,6 +149,44 @@ class SQL_EXPORT Recovery {
   // Only valid to call after successful SetupMeta().
   bool GetMetaVersionNumber(int* version_number);
 
+  // Attempt to recover the database by creating a new database with schema from
+  // |db|, then copying over as much data as possible.  If the database is
+  // entirely unreadable, the new database will be empty.  After this call, the
+  // |db| handle will be poisoned so that future calls will return errors until
+  // the handle is re-opened.
+  //
+  // The "OrRaze" portion means that if recovery fails due to errors, the
+  // database can be razed or deleted.  The goal is for this function to make

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

Uh, did you mean "will be", not "can be"?  And will the result be razed (i.e.,
deleted) or an empty database?
If you didn't mean that, then how will the caller know whether the recovery
failed and the database is okay to be razed?  This function has no return value.
If you're intentionally vague because you plan to change this from
leave-corrupt-database-around to something else, just give it the right name for
now.  Later when you make the intended change, you can rename the function.

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

I'm not sure how to answer!

Right now, it only applies the safe cases.  It either thinks it recovered the
database, or it rolls things back.  But I've seen results from the fleet which
make me believe that running recovery to success didn't actually work (possibly
because of shenanigans in the system, like malware or profile's being
overwritten or something).  The intention is to monitor the histograms to see if
any of the failure outcomes is due to a broken assumption rather than broken
user data.  If the failures cases look weirdly high, I'll instrument to debug
the fleet, but if they look sensible, I'll switch them over from Rollback() to
Unrecoverable() (leading to Raze()).

sql::Connection::Raze() means that it overwrites the database with a single-page
empty database.  The delete language is because if the Raze() doesn't work, then
what?  Probably attempt to Delete().  I'm nervous about landing either of those
up front, though, on the off chance that I write a bug which works right in my
tests but in the field blows away a bunch of data.

Anyhow, my intention with this function is that it consumes the database handle
in all cases, while making a best effort to recover the data, and failing that a
best effort to clear the database of data, and failing that a best effort to
delete the database.  On next run the database should either be a valid
data-containing SQLite database (doesn't throw SQLITE_CORRUPT), or it should be
a valid empty SQLite database, or it should be missing (SQLite would create the
database empty).

Raze() versus Delete() is because Raze() works using SQLite locking and I/O.  It
can clear the database in cases where Delete() won't work.  Likewise, Delete()
may work in cases where Raze() does not.

+  // every possible effort to leave behind a correctly operating database file.
+  static void RecoverDatabaseOrRaze(Connection* db,
+                                    const base::FilePath& db_path);
+
+  // Returns true if the SQLite |extended_error| is one which can plausibly be
+  // recovered, or which cannot possibly be recovered.
+  //

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

Please provide an transition sentence here.  It appears everything else in the
comment explains how this function works, i.e., which error code you consider
recoverable and why.  Another sentence would make this comment more readable.

Also, consider whether these comments better belong in the implementation, not
the .h.

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

Moved cases into the implementation file.

I'm not sure how to make this fully hermetic WRT SQLite.  Some SQLite errors
stem from obviously broken database files, some stem from obviously broken
client code, and some are ambiguous.  My goal is to get rid of the ambiguity
over time, but I'd rather approach it piecemeal, fixing the known-possible cases
first.

+  // SQLITE_CANTOPEN is associated with an entirely broken file (for instance a
+  // symlink to a non-existent path, or the file is a directory).  The best fix
+  // is probably to delete the file and start over.

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

probably?
When this is not the best fix?
(consider omitting the word probably)

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

I guess this is expressing my certainty level.  SQLite is very layered, so
sometimes the error you get is unexpected - when you get that error and decode
why, it makes sense how it happened, but reading the code it's challenging to
verify which specific errors are possible at which specific API calls. 
SQLITE_CANTOPEN is returned for the example cases given, but I don't know how
far the unknown unknowns run.

+  //
+  // SQLITE_NOTADB can happen if the SQLite header is broken.  In earlier
+  // versions of SQLite, this was returned if the header size information did
+  // not match the OS file size information (now that causes SQLITE_CORRUPT).
+  // In that case much of the data can be recovered.  In other cases, the best
+  // fix is probably to delete the file and start over.

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

This last sentence is confusing.  Did you mean that there are other times this
error code can be returned and those are probably not recoverable?  If so, then
this comment fails to explain what this function will return for this error
code.
Perhaps you want to drop the last sentence if appropriate?

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

When the recovery code was first written, there was a case where SQLITE_NOTADB
was recoverable.  Now that case returns SQLITE_CORRUPT (which is a more sensible
return value).  So it made sense to run the recovery code, then delete it if
that failed.

+  //
+  // SQLITE_CORRUPT means that the database is readable, but the contents have
+  // an inconsistency.  In the worst case, this could be isolated garbage in a
+  // page, but generally it means that pages which are separately valid do not
+  // make sense when taken together.  For instance if an index refers to a row
+  // which is no longer in the table.

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

Again, this explanation doesn't state what this function will return for this
error code.  In fact, it implies that the function will say to raze the
database, when that's the opposite of what the function does.

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

Revised in implementation.

+  //
+  // 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.
+  static bool ShouldRecoverOrRaze(int extended_error);

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

nit: given your explanation of what this does, I think it should be named
something like IsRecoverable or IsLikelyRecoverable or IsRecoverableGivenError
or ShouldTryRecovering or something.

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

Hmm.  None of the first three work with me - there's no way to guarantee that it
is recoverable short of trying, and there isn't really any way to productively
quantify the level of success recovery had.  ShouldTry makes it sound like you
should try recovery, and if that doesn't work maybe something else, but I'm not
sure there is anything else to try.

The reason for the separation is to let the calling code be structured like:

if (ShouldTry(error)) {
  // Cleanup local state before the destructive recover call.
  Recover(db, db_path);
}

I could instead pass the error into the operative function, like:

if (AutomatedErrorHandler(db, error, db_path)) {
  // Cleanup local state because the database is now poisoned.
}

I think phrasing that name would also be challenging, and I don't like having
the return value signal whether serious side effects have happened or not.

PROBABLY in all cases getting into the error handler should lead to a poisoned
handle.  In the best cases, it was a benign syntax error which inadvertently got
shipped to production and doesn't harm anyone, but in the worst cases you're
messing up your data and then messing up the mess.  So it should be like an OOM
CHECK(), where once you notice something awry, you minimize the damage. 
Unfortunately, there's code in Chromium which implicitly depends on working
degraded because things were too relaxed when that code landed (which is why I'm
even here, my long-term goal is to have no errors in the static sense, only in
the dynamic sense, and then have things to automatically handle the dynamic
errors).

["static sense" meaning incorrectly-coded clients, "dynamic sense" meaning
problems due to the operating environment.]

+
  private:
   explicit Recovery(Connection* connection);