[sql] Use recover virtual table in sql::Recovery.

sql/recovery_unittest.cc

Index: sql/recovery_unittest.cc
diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc
index e91ee10680e9c6d5f52ef234f9b9b01295d5351b..fc7c2f2dc5b725888fcbb0f4aaf6f613ffc9a286 100644
--- a/sql/recovery_unittest.cc
+++ b/sql/recovery_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
@@ -47,6 +48,49 @@ std::string GetSchema(sql::Connection* db) {
   return ExecuteWithResults(db, kSql, "|", "\n");
 }
 
+int GetPageSize(sql::Connection* db) {
+  sql::Statement s(db->GetUniqueStatement("PRAGMA page_size"));
+  EXPECT_TRUE(s.Step());
+  return s.ColumnInt(0);
+}
+
+// Get |name|'s root page number in the database.
+int GetRootPage(sql::Connection* db, const char* name) {
+  const char kPageSql[] = "SELECT rootpage FROM sqlite_master WHERE name = ?";
+  sql::Statement s(db->GetUniqueStatement(kPageSql));
+  s.BindString(0, name);
+  EXPECT_TRUE(s.Step());
+  return s.ColumnInt(0);
+}
+
+// Helper to read a SQLite page into a buffer.  |page_no| is 1-based
+// per SQLite usage.
+bool ReadPage(const base::FilePath& path, size_t page_no,
+              char* buf, size_t page_size) {
+  file_util::ScopedFILE file(file_util::OpenFile(path, "rb"));
+  if (!file.get())
+    return false;
+  if (0 != fseek(file.get(), (page_no - 1) * page_size, SEEK_SET))
+    return false;
+  if (1u != fread(buf, page_size, 1, file.get()))
+    return false;
+  return true;
+}
+
+// Helper to write a SQLite page into a buffer.  |page_no| is 1-based
+// per SQLite usage.
+bool WritePage(const base::FilePath& path, size_t page_no,
+               const char* buf, size_t page_size) {
+  file_util::ScopedFILE file(file_util::OpenFile(path, "rb+"));
+  if (!file.get())
+    return false;
+  if (0 != fseek(file.get(), (page_no - 1) * page_size, SEEK_SET))
+    return false;
+  if (1u != fwrite(buf, page_size, 1, file.get()))
+    return false;
+  return true;
+}
+
 class SQLRecoveryTest : public testing::Test {
  public:
   SQLRecoveryTest() {}
@@ -138,8 +182,244 @@ TEST_F(SQLRecoveryTest, RecoverBasic) {
   ASSERT_EQ("CREATE TABLE x (t TEXT)", GetSchema(&db()));
 
   const char* kXSql = "SELECT * FROM x ORDER BY 1";
-  ASSERT_EQ(ExecuteWithResults(&db(), kXSql, "|", "\n"),
-            "That was a test");
+  ASSERT_EQ("That was a test",
+            ExecuteWithResults(&db(), kXSql, "|", "\n"));
+}
+
+// The recovery virtual table is only supported for Chromium's SQLite.
+#if !defined(USE_SYSTEM_SQLITE)
+
+// Run recovery through its paces on a valid database.
+TEST_F(SQLRecoveryTest, VirtualTable) {
+  const char kCreateSql[] = "CREATE TABLE x (t TEXT)";
+  ASSERT_TRUE(db().Execute(kCreateSql));
+  ASSERT_TRUE(db().Execute("INSERT INTO x VALUES ('This is a test')"));
+  ASSERT_TRUE(db().Execute("INSERT INTO x VALUES ('That was a test')"));
+
+  // Successfully recover the database.
+  {
+    scoped_ptr<sql::Recovery> recovery = sql::Recovery::Begin(&db(), db_path());
+
+    // Tables to recover original DB, now at [corrupt].
+    const char kRecoveryCreateSql[] =
+        "CREATE VIRTUAL TABLE temp.recover_x using recover("
+        "  corrupt.x,"
+        "  t TEXT STRICT"
+        ")";
+    ASSERT_TRUE(recovery->db()->Execute(kRecoveryCreateSql));
+
+    // Re-create the original schema.
+    ASSERT_TRUE(recovery->db()->Execute(kCreateSql));
+
+    // Copy the data from the recovery tables to the new database.
+    const char kRecoveryCopySql[] =
+        "INSERT INTO x SELECT t FROM recover_x";
+    ASSERT_TRUE(recovery->db()->Execute(kRecoveryCopySql));
+
+    // Successfully recovered.
+    ASSERT_TRUE(sql::Recovery::Recovered(recovery.Pass()));
+  }
+
+  // Since the database was not corrupt, the entire schema and all
+  // data should be recovered.
+  ASSERT_TRUE(Reopen());
+  ASSERT_EQ("CREATE TABLE x (t TEXT)", GetSchema(&db()));
+
+  const char* kXSql = "SELECT * FROM x ORDER BY 1";
+  ASSERT_EQ("That was a test\nThis is a test",
+            ExecuteWithResults(&db(), kXSql, "|", "\n"));
+}
+
+void RecoveryCallback(sql::Connection* db, const base::FilePath& db_path,
+                      int* record_error, int error, sql::Statement* stmt) {
+  *record_error = error;
+
+  // Clear the error callback to prevent reentrancy.
+  db->reset_error_callback();

[erikwright (departed), 2013/07/26 16:05:33]

Is this something most clients would need to do? With the penalty for negligence
being infinite recursion when recovery fails?

Presumably a real error handler would also have to restore itself at the end?

If so could we find some way to deal with that? Maybe it makes sense to always
disable the error handler during recovery.

[Scott Hess - ex-Googler, 2013/07/26 17:15:58]

Possibilities are probably endless.  It could recurse, or it could fail due to
conflict with the outer handler's setup, or it could fail and mess up the outer
handler while doing so.
Still working through this.  In the context of sql::Recovery, once we're done
the handle will be closed.  Which I thought was the end of it, until ... now
that you asked, it occurs to me that the error handler could be considered an
attribute of the handle, not of the connection, so you could re-open things and
now be in a weird state.  Re-open of a previously-closed handle probably isn't
intended, but it works.

Right now my goal is to aim for something unlikely to provide unexpected results
when faced with real-world crazy.  sql::Recovery asserts that the connection
being recovered has no error callback, because needing to recover a database
probably implies that that database will throw errors.  But reviewing the calls
agains the original connection, I'm not sure any of them would ever throw errors
in practice.  Having the sql::Recovery reset/restore the error callback might
make sense, but could cause odd behavior if the calling code also resets the
callback.  I'm not enthusiastic about adding additional wiring across
sql::Recovery and sql::Connection until I'm sure it's actually helpful.

I expect things will become more clear as I grind through writing code for the
database users to recover things.  If you feel strongly that we should define
this earlier, let me know.  I've been intending to write the thumbnails_database
recovery handler today for about a week, now, so it should happen soon :-).

[erikwright (departed), 2013/07/26 17:56:57]

That's fine for now.

+
+  scoped_ptr<sql::Recovery> recovery = sql::Recovery::Begin(db, db_path);
+  ASSERT_TRUE(recovery.get());
+
+  const char kRecoveryCreateSql[] =
+      "CREATE VIRTUAL TABLE temp.recover_x using recover("
+      "  corrupt.x,"
+      "  id INTEGER STRICT,"
+      "  v INTEGER STRICT"
+      ")";
+  const char kCreateTable[] = "CREATE TABLE x (id INTEGER, v INTEGER)";
+  const char kCreateIndex[] = "CREATE UNIQUE INDEX x_id ON x (id)";
+
+  // Replicate data over.
+  const char kRecoveryCopySql[] =
+      "INSERT OR REPLACE INTO x SELECT id, v FROM recover_x";
+
+  ASSERT_TRUE(recovery->db()->Execute(kRecoveryCreateSql));
+  ASSERT_TRUE(recovery->db()->Execute(kCreateTable));
+  ASSERT_TRUE(recovery->db()->Execute(kCreateIndex));
+  ASSERT_TRUE(recovery->db()->Execute(kRecoveryCopySql));
+
+  ASSERT_TRUE(sql::Recovery::Recovered(recovery.Pass()));
+}
+
+// Build a database, corrupt it by making an index reference to
+// deleted row, then recover when a query selects that row.
+TEST_F(SQLRecoveryTest, RecoverCorruptIndex) {
+  const char kCreateTable[] = "CREATE TABLE x (id INTEGER, v INTEGER)";
+  const char kCreateIndex[] = "CREATE UNIQUE INDEX x_id ON x (id)";
+  ASSERT_TRUE(db().Execute(kCreateTable));
+  ASSERT_TRUE(db().Execute(kCreateIndex));
+
+  // Insert a bit of data.
+  {
+    ASSERT_TRUE(db().BeginTransaction());
+
+    const char kInsertSql[] = "INSERT INTO x (id, v) VALUES (?, ?)";
+    sql::Statement s(db().GetUniqueStatement(kInsertSql));
+    for (int i = 0; i < 10; ++i) {
+      s.Reset(true);
+      s.BindInt(0, i);
+      s.BindInt(1, i);
+      EXPECT_FALSE(s.Step());
+      EXPECT_TRUE(s.Succeeded());
+    }
+
+    ASSERT_TRUE(db().CommitTransaction());
+  }
+
+
+  // Capture the index's root page into |buf|.
+  int index_page = GetRootPage(&db(), "x_id");
+  int page_size = GetPageSize(&db());
+  scoped_ptr<char[]> buf(new char[page_size]);
+  ASSERT_TRUE(ReadPage(db_path(), index_page, buf.get(), page_size));
+
+  // Delete the row from the table and index.
+  ASSERT_TRUE(db().Execute("DELETE FROM x WHERE id = 0"));
+
+  // Close to clear any cached data.
+  db().Close();
+
+  // Put the stale index page back.
+  ASSERT_TRUE(WritePage(db_path(), index_page, buf.get(), page_size));
+
+  // At this point, the index references a value not in the table.
+
+  ASSERT_TRUE(Reopen());
+
+  int error = SQLITE_OK;
+  db().set_error_callback(base::Bind(&RecoveryCallback,
+                                     &db(), db_path(), &error));
+
+  // This works before the callback is called.
+  const char kTrivialSql[] = "SELECT COUNT(*) FROM sqlite_master";
+  EXPECT_TRUE(db().IsSQLValid(kTrivialSql));
+
+  // TODO(shess): Could this be delete?  Anything which fails should work.
+  const char kSelectSql[] = "SELECT v FROM x WHERE id = 0";
+  ASSERT_FALSE(db().Execute(kSelectSql));
+  EXPECT_EQ(SQLITE_CORRUPT, error);
+
+  // Database handle has been poisoned.
+  EXPECT_FALSE(db().IsSQLValid(kTrivialSql));
+
+  ASSERT_TRUE(Reopen());
+
+  // The recovered table should reflect the deletion.
+  const char kSelectAllSql[] = "SELECT v FROM x ORDER BY id";
+  EXPECT_EQ("1,2,3,4,5,6,7,8,9",
+            ExecuteWithResults(&db(), kSelectAllSql, "|", ","));
+
+  // The failing statement should now succeed, with no results.
+  EXPECT_EQ("", ExecuteWithResults(&db(), kSelectSql, "|", ","));
+}
+
+// Build a database, corrupt it by making a table contain a row not
+// referenced by the index, then recover the database.
+TEST_F(SQLRecoveryTest, RecoverCorruptTable) {
+  const char kCreateTable[] = "CREATE TABLE x (id INTEGER, v INTEGER)";
+  const char kCreateIndex[] = "CREATE UNIQUE INDEX x_id ON x (id)";
+  ASSERT_TRUE(db().Execute(kCreateTable));
+  ASSERT_TRUE(db().Execute(kCreateIndex));
+
+  // Insert a bit of data.
+  {
+    ASSERT_TRUE(db().BeginTransaction());
+
+    const char kInsertSql[] = "INSERT INTO x (id, v) VALUES (?, ?)";
+    sql::Statement s(db().GetUniqueStatement(kInsertSql));
+    for (int i = 0; i < 10; ++i) {
+      s.Reset(true);
+      s.BindInt(0, i);
+      s.BindInt(1, i);
+      EXPECT_FALSE(s.Step());
+      EXPECT_TRUE(s.Succeeded());
+    }
+
+    ASSERT_TRUE(db().CommitTransaction());
+  }
+
+  // Capture the table's root page into |buf|.
+  // Find the page the table is stored on.
+  const int table_page = GetRootPage(&db(), "x");
+  const int page_size = GetPageSize(&db());
+  scoped_ptr<char[]> buf(new char[page_size]);
+  ASSERT_TRUE(ReadPage(db_path(), table_page, buf.get(), page_size));
+
+  // Delete the row from the table and index.
+  ASSERT_TRUE(db().Execute("DELETE FROM x WHERE id = 0"));
+
+  // Close to clear any cached data.
+  db().Close();
+
+  // Put the stale table page back.
+  ASSERT_TRUE(WritePage(db_path(), table_page, buf.get(), page_size));
+
+  // At this point, the table contains a value not referenced by the
+  // index.
+  // TODO(shess): Figure out a query which causes SQLite to notice

[erikwright (departed), 2013/07/26 16:05:33]

IIUC, this type of error (an extra value in the table) is something that SQLite
will continue to gloss over while returning inconsistent results, and thus the
client would be responsible for detecting this state and initiating recovery?

[Scott Hess - ex-Googler, 2013/07/26 17:15:58]

It's not glossing it over, it never even sees the problem, because it only looks
at the data necessary to satisfy the query.  Doing a query through the table
sees the table data without the constraints, doing a query through the index
works fine because what the index says is there is there.  The only way to
detect such things is to do a full scan of the database ("pragma
integrity_check" will do).  It might make sense to do that check automatically
every once and awhile.

This TODO isn't about making SQLite notice this case, it's about having a
corruption which SQLite will notice when querying the table.  The index test is
kind of a softball, since the recovery callback ignores indexes entirely.  I
want to have at least one test which demonstrates that invalid constraints can
be handled in a way which recovers some data.  I'm also hoping that it can be
simple, like restoring an old version of a page, rather than a complicated
sequence of mumbo-jumbo which is brittle in the face of minor SQLite changes.

+  // this organically.  Meanwhile, just handle it manually.
+
+  ASSERT_TRUE(Reopen());
+
+  // Index shows one less than originally inserted.
+  const char kCountSql[] = "SELECT COUNT (*) FROM x";
+  EXPECT_EQ("9", ExecuteWithResults(&db(), kCountSql, "|", ","));
+
+  // A full table scan shows all of the original data.
+  const char kDistinctSql[] = "SELECT DISTINCT COUNT (id) FROM x";
+  EXPECT_EQ("10", ExecuteWithResults(&db(), kDistinctSql, "|", ","));
+
+  // Insert id 0 again.  Since it is not in the index, the insert
+  // succeeds, but results in a duplicate value in the table.
+  const char kInsertSql[] = "INSERT INTO x (id, v) VALUES (0, 100)";
+  ASSERT_TRUE(db().Execute(kInsertSql));
+
+  // Duplication is visible.
+  EXPECT_EQ("10", ExecuteWithResults(&db(), kCountSql, "|", ","));
+  EXPECT_EQ("11", ExecuteWithResults(&db(), kDistinctSql, "|", ","));
+
+  // This works before the callback is called.
+  const char kTrivialSql[] = "SELECT COUNT(*) FROM sqlite_master";
+  EXPECT_TRUE(db().IsSQLValid(kTrivialSql));
+
+  // Call the recovery callback manually.
+  int error = SQLITE_OK;
+  RecoveryCallback(&db(), db_path(), &error, SQLITE_CORRUPT, NULL);
+  EXPECT_EQ(SQLITE_CORRUPT, error);
+
+  // Database handle has been poisoned.
+  EXPECT_FALSE(db().IsSQLValid(kTrivialSql));
+
+  ASSERT_TRUE(Reopen());
+
+  // The recovered table has consistency between the index and the table.
+  EXPECT_EQ("10", ExecuteWithResults(&db(), kCountSql, "|", ","));
+  EXPECT_EQ("10", ExecuteWithResults(&db(), kDistinctSql, "|", ","));
+
+  // The expected value was retained.
+  const char kSelectSql[] = "SELECT v FROM x WHERE id = 0";
+  EXPECT_EQ("100", ExecuteWithResults(&db(), kSelectSql, "|", ","));
 }
+#endif  // !defined(USE_SYSTEM_SQLITE)
 
 }  // namespace