[sql] Additional Raze() unit tests.

sql/connection.cc

 // Copyright (c) 2012 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.
 
 #include "sql/connection.h"
 
 #include <string.h>
 
 #include "base/files/file_path.h"
 #include "base/file_util.h"
@@ skipping 46 matching lines @@
     sqlite3_exec(db_, "PRAGMA writable_schema=1", NULL, NULL, NULL);
   }
   ~ScopedWritableSchema() {
     sqlite3_exec(db_, "PRAGMA writable_schema=0", NULL, NULL, NULL);
   }
 
  private:
   sqlite3* db_;
 };
 
+// Helper to wrap the sqlite3_backup_*() step of Raze().  Return
+// SQLite error code from running the backup step.
+int BackupDatabase(sqlite3* src, sqlite3* dst, const char* db_name) {

[Greg Billock, 2013/06/27 21:01:47]

Docs say src and dst must be different. Want to DCHECK that?

[Scott Hess - ex-Googler, 2013/06/27 22:41:47]

Done.

+  sqlite3_backup* backup = sqlite3_backup_init(dst, db_name, src, db_name);
+  if (!backup) {
+    // Since this call only sets things up, this indicates a gross
+    // error in SQLite.
+    DLOG(FATAL) << "Unable to start sqlite3_backup().";

[Greg Billock, 2013/06/27 21:01:47]

Print out dst->errmsg/errcode as well here?

[Scott Hess - ex-Googler, 2013/06/27 22:41:47]

OK.  Not entirely convinced it will be useful due to D, but also not convinced
anyone will ever see things if it was LOG(ERROR).  Also converted return code to
appropriate errorcode.

+    return SQLITE_ABORT;
+  }
+
+  // -1 backs up the entire database.
+  int rc = sqlite3_backup_step(backup, -1);
+  int pages = sqlite3_backup_pagecount(backup);

[Greg Billock, 2013/06/27 21:01:47]

Useful if rc != SQLITE_DONE? Also, need to deal with SQLITE_BUSY/SQLITE_LOCKED?

[Scott Hess - ex-Googler, 2013/06/27 22:41:47]

Honestly, I am not sure what to do in those cases.  Sleep and retry?  I'll add
some histograms to see which areas merit attention.

+  sqlite3_backup_finish(backup);
+
+  // If successful, exactly one page should have been backed up.  If
+  // this breaks, check this function to make sure assumptions aren't
+  // being broken.
+  if (rc == SQLITE_DONE)
+    DCHECK_EQ(pages, 1);
+
+  return rc;
+}
+
 }  // namespace
 
 namespace sql {
 
 // static
 Connection::ErrorIgnorerCallback* Connection::current_ignorer_cb_ = NULL;
 
 // static
 bool Connection::ShouldIgnore(int error) {
   if (!current_ignorer_cb_)
@@ skipping 229 matching lines @@
   // page, and if it does not match the total retrieved from a
   // filesystem call, treats the database as corrupt.  This situation
   // breaks almost all SQLite calls.  "PRAGMA writable_schema" can be
   // used to hint to SQLite to soldier on in that case, specifically
   // for purposes of recovery.  [See SQLITE_CORRUPT_BKPT case in
   // sqlite3.c lockBtree().]
   // TODO(shess): With this, "PRAGMA auto_vacuum" and "PRAGMA
   // page_size" can be used to query such a database.
   ScopedWritableSchema writable_schema(db_);
 
-  sqlite3_backup* backup = sqlite3_backup_init(db_, "main",
-                                               null_db.db_, "main");
-  if (!backup) {
-    DLOG(FATAL) << "Unable to start sqlite3_backup().";
-    return false;
-  }
-
-  // -1 backs up the entire database.
-  int rc = sqlite3_backup_step(backup, -1);
-  int pages = sqlite3_backup_pagecount(backup);
-  sqlite3_backup_finish(backup);
+  const char* kMain = "main";
+  int rc = BackupDatabase(null_db.db_, db_, kMain);
 
   // The destination database was locked.
   if (rc == SQLITE_BUSY) {
     return false;
   }
 
+  // SQLITE_NOTADB can happen if page 1 exists but is not formatted

[Greg Billock, 2013/06/27 21:01:47]

So this is page 1 of the destination?

[Scott Hess - ex-Googler, 2013/06/27 22:41:47]

Comment amended.

+  // correctly.  SQLITE_IOERR_SHORT_READ can happen if the database
+  // isn't even big enough for one page.  Either way, reach in and
+  // truncate it before trying again.

[Greg Billock, 2013/06/27 21:01:47]

Above it sounds like PRAGMA writeable_schema will achieve this. Is that not so?
That seems like a not-quite-so-low-level approach if it worked...

[Scott Hess - ex-Googler, 2013/06/27 22:41:47]

That case covers when the database in the filesystem is N*page_size, but the
header says that it should be N+1 or N-1 or something other than N pages. 
SQLITE_IOERR_SHORT_READ happens if the database is less than page_size.

[Greg Billock, 2013/06/28 14:29:09]

So we see both these on corruption?

On 2013/06/27 22:41:47, shess wrote:

+  // TODO(shess): Maybe it would be worthwhile to just truncate from
+  // the get-go?
+  if (rc == SQLITE_NOTADB || rc == SQLITE_IOERR_SHORT_READ) {

[Greg Billock, 2013/06/27 21:01:47]

Should this stanza go in the backup method?

[Scott Hess - ex-Googler, 2013/06/27 22:41:47]

That's what the TODO is about.  First it tries to do the backup straight up.  If
that works, grand, if it fails for a few specific reasons, it adjusts the file
and tries again.  Putting the error-handler in BackupDatabase() would duplicate
that code (once for first try, once for retry).

OTOH, if it _started_ with the below code to truncate the database in all cases,
the there wouldn't need to be duplication.  In fact, it could maybe just
truncate and _not_ do the backup trick, because AFAICT SQLite treats the
database as empty if it's zero-length.  AFAICT.

Right now, this code only does the truncate if SQLITE_NOTADB and
SQLITE_IOERR_SHORT_READ, specifically resulting from the cases tested, so I'm
pretty sure about them.  AFAICT, in both of those cases it would be impossible
for anyone else to have legitimately opened the database, so there is no scope
for confusion.

For other errors, I'm less certain whether truncating is the right thing to do. 
Certainly if the first backup attempt got SQLITE_BUSY, truncation would likely
cause interesting results.  I'm not sure what to do about it, though.

For background, the reason to do things in this subtle way rather than deleting
the file is because if anyone has the file open on Windows, delete won't work
but this can.  This can include things like anti-virus programs.  I considered
having this be the backup plan in case delete fails, but I thought it prudent to
make the subtle case the common case, so that it gets more thoroughly tested. 
I'm thinking of adding delete as a backup to this, but I'm not certain in which
cases that makes sense.

[Greg Billock, 2013/06/28 14:29:09]

I see. The histogram ought to provide good guidance here.

On 2013/06/27 22:41:47, shess wrote:

+    sqlite3_file* file = NULL;
+    rc = sqlite3_file_control(db_, "main", SQLITE_FCNTL_FILE_POINTER, &file);
+    if (rc != SQLITE_OK) {
+      DLOG(FATAL) << "Failure getting file handle.";
+      return false;
+    } else if (!file) {
+      DLOG(FATAL) << "File handle is empty.";
+      return false;
+    }
+
+    rc = file->pMethods->xTruncate(file, 0);
+    if (rc != SQLITE_OK) {
+      DLOG(FATAL) << "Failed to truncate file.";
+      return false;
+    }
+
+    rc = BackupDatabase(null_db.db_, db_, kMain);
+
+    if (rc != SQLITE_DONE) {
+      DLOG(FATAL) << "Failed retrying Raze().";
+    }
+  }
+
   // The entire database should have been backed up.
   if (rc != SQLITE_DONE) {
+    // TODO(shess): Figure out which other cases can happen.
     DLOG(FATAL) << "Unable to copy entire null database.";
     return false;
   }
 
-  // Exactly one page should have been backed up.  If this breaks,
-  // check this function to make sure assumptions aren't being broken.
-  DCHECK_EQ(pages, 1);
-
   return true;
 }
 
 bool Connection::RazeWithTimout(base::TimeDelta timeout) {
   if (!db_) {
     DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
   ScopedBusyTimeout busy_timeout(db_);
@@ skipping 303 matching lines @@
     return false;
   }
 
   // If |poisoned_| is set, it means an error handler called
   // RazeAndClose().  Until regular Close() is called, the caller
   // should be treating the database as open, but is_open() currently
   // only considers the sqlite3 handle's state.
   // TODO(shess): Revise is_open() to consider poisoned_, and review
   // to see if any non-testing code even depends on it.
   DLOG_IF(FATAL, poisoned_) << "sql::Connection is already open.";
+  poisoned_ = false;
 
   int err = sqlite3_open(file_name.c_str(), &db_);
   if (err != SQLITE_OK) {
     // Histogram failures specific to initial open for debugging
     // purposes.
     UMA_HISTOGRAM_ENUMERATION("Sqlite.OpenFailure", err & 0xff, 50);
 
     OnSqliteError(err, NULL);
     Close();
     db_ = NULL;
@@ skipping 164 matching lines @@
   }
 
   // Best effort to put things back as they were before.
   const char kNoWritableSchema[] = "PRAGMA writable_schema = OFF";
   ignore_result(Execute(kNoWritableSchema));
 
   return ret;
 }
 
 }  // namespace sql