[sql] Additional Raze() unit tests.

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index e99b6bc53dacb8f0ab2569d5d6eb01245446be93..3e85245d6f237bf4e85437140f8361c2e8365de5 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -64,6 +64,31 @@ class ScopedWritableSchema {
   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 {
@@ -313,33 +338,51 @@ bool Connection::Raze() {
   // 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;
 }
 
@@ -663,6 +706,7 @@ bool Connection::OpenInternal(const std::string& file_name) {
   // 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) {