[sql] Database recovery system for Shortcuts.

sql/recovery.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SQL_RECOVERY_H_
 #define SQL_RECOVERY_H_
 
 #include <stddef.h>
 
 #include <memory>
 
 #include "base/macros.h"
 #include "sql/connection.h"
 
 namespace base {
 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 =
 //       sql::Recovery::Begin(orig_db, orig_db_path);
 //   if (r) {
 //     // Create the schema to recover to.  On failure, clear the
 //     // database.
 //     if (!r.db()->Execute(kCreateSchemaSql)) {
 //       sql::Recovery::Unrecoverable(std::move(r));
 //       return;
@@ skipping 37 matching lines @@
   // and attach the existing database to it at "corrupt".  To prevent
   // deadlock, all transactions on |connection| are rolled back.
   //
   // Returns NULL in case of failure, with no cleanup done on the
   // original connection (except for breaking the transactions).  The
   // caller should Raze() or otherwise cleanup as appropriate.
   //
   // TODO(shess): Later versions of SQLite allow extracting the path
   // from the connection.
   // TODO(shess): Allow specifying the connection point?
   static std::unique_ptr<Recovery> Begin(Connection* connection,

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

I'm surprised that you're changing the type of this function and the ones below
it and don't need to add additional files to this changelist.

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

The change from base::scoped_ptr<> to std::unique_ptr<> happened since the CL
was originally posted.  So any changes you see there are probably just the
rebase, and wouldn't show in a diff from trunk (except any new code following
the conversion, of course).

                                          const base::FilePath& db_path)
       WARN_UNUSED_RESULT;
 
   // Mark recovery completed by replicating the recovery database over
   // the original database, then closing the recovery database.  The
   // original database handle is poisoned, causing future calls
   // against it to fail.
   //
   // If Recovered() is not called, the destructor will call
   // Unrecoverable().
@@ skipping 48 matching lines @@
   // table as needed.
   bool SetupMeta();
 
   // Fetch the version number from temp.recover_meta.  Returns false
   // if the query fails, or if there is no version row.  Otherwise
   // returns true, with the version in |*version_number|.
   //
   // 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);
 
   // Setup the recovery database handle for Begin().  Returns false in
   // case anything failed.
   bool Init(const base::FilePath& db_path) WARN_UNUSED_RESULT;
 
   // Copy the recovered database over the original database.
   bool Backup() WARN_UNUSED_RESULT;
 
   // Close the recovery database, and poison the original handle.
   // |raze| controls whether the original database is razed or just
   // poisoned.
   enum Disposition {
     RAZE_AND_POISON,
     POISON,
   };
   void Shutdown(Disposition raze);
 
   Connection* db_;         // Original database connection.
   Connection recover_db_;  // Recovery connection.
 
   DISALLOW_COPY_AND_ASSIGN(Recovery);
 };
 
 }  // namespace sql
 
 #endif  // SQL_RECOVERY_H_