[sql] Database recovery system for Shortcuts.

sql/recovery.h

Index: sql/recovery.h
diff --git a/sql/recovery.h b/sql/recovery.h
index 8efd71877318b04f1df5486f135257038717918d..20b85d7f05a8e233b9d91420cddd25777e916990 100644
--- a/sql/recovery.h
+++ b/sql/recovery.h
@@ -18,13 +18,17 @@ class FilePath;
 
 namespace sql {
 
-// Recovery module for sql/.  The basic idea is to create a fresh
-// database and populate it with the recovered contents of the
-// original database.  If recovery is successful, the recovered
-// database is backed up over the original database.  If recovery is
-// not successful, the original database is razed.  In either case,
-// the original handle is poisoned so that operations on the stack do
-// not accidentally disrupt the restored data.
+// Recovery module for sql/.  The basic idea is to create a fresh database and
+// populate it with the recovered contents of the original database.  If
+// recovery is successful, the recovered database is backed up over the original
+// database.  If recovery is not successful, the original database is razed.  In
+// either case, the original handle is poisoned so that operations on the stack
+// do not accidentally disrupt the restored data.
+//
+// RecoverDatabaseOrRaze() automates this, including recoverying the schema of
+// from the suspect database.  If a database requires special handling, such as
+// recovering between different schema, or tables requiring post-processing,
+// then the module can be used manually like:
 //
 // {
 //   std::unique_ptr<sql::Recovery> r =
@@ -151,6 +155,31 @@ 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
+  // every possible effort to leave behind a correctly operating database file.
+  //
+  // If a corrupt database contains tables without unique indices, the resulting
+  // table may contain duplication.  If this is not acceptable, the client
+  // should use the manual process as described in the example at the top of the
+  // file, cleaning up data at the appropriate points.
+  static void RecoverDatabaseOrRaze(Connection* db,

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

Thanks for the long response.  Fundamentally, my complaint is that the comment
above and indeed the function name doesn't correspond with what the function
does.  There is no razing going on here.

Why not just call it RecoverDatabase()?  And remove comments that are entirely
wrong such as "If the database is entirely unreadable, the new database will be
empty.", and, for that matter, the whole second paragraph (each sentence in it
is currently wrong).

You can add your raze or delete language back into the function name and
comments once you actually switch the code from doing Rollback to Unrecoverable.
 And at that time add comments about the goal of the function (leaving a working
database, even if empty) at that time.  That's not the goal at this time.

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

Done.

+                                    const base::FilePath& db_path);
+
+  // Returns true for SQLite errors which RecoverDatabaseOrRaze() can recover,
+  // or which cannot plausibly be recovered (and should be razed).  This does

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

nit: or which -> versus
You don't mean "or", at least in the boolean sense.
Consider making a similar change to the function name.

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

Just removed the comparison clause entirely.

+  // not guarantee that RecoverDatabaseOrRaze() will successfully recover any
+  // data.  Returning false means the error is not related to the contents of
+  // the database (such as SQLITE_BUSY or SQLITE_MISUSE), or is not verified to
+  // be handled correctly (such as SQLITE_IOERR or SQLITE_FULL).
+  static bool ShouldRecoverOrRaze(int extended_error);
+
  private:
   explicit Recovery(Connection* connection);