[sql] Retry post-poison open as soon as possible.

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 <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
@@ skipping 1675 matching lines @@
   sqlite3_db_config(db_, SQLITE_DBCONFIG_LOOKASIDE, NULL, 0, 0);
 
   // Enable extended result codes to provide more color on I/O errors.
   // Not having extended result codes is not a fatal problem, as
   // Chromium code does not attempt to handle I/O errors anyhow.  The
   // current implementation always returns SQLITE_OK, the DCHECK is to
   // quickly notify someone if SQLite changes.
   err = sqlite3_extended_result_codes(db_, 1);
   DCHECK_EQ(err, SQLITE_OK) << "Could not enable extended result codes";
 
-  // sqlite3_open() does not actually read the database file (unless a
-  // hot journal is found).  Successfully executing this pragma on an
-  // existing database requires a valid header on page 1.
+  // sqlite3_open() does not actually read the database file (unless a hot
+  // journal is found).  Successfully executing this pragma on an existing
+  // database requires a valid header on page 1.  ExecuteAndReturnErrorCode() to
+  // get the error code before error callback (potentially) overwrites.
   // TODO(shess): For now, just probing to see what the lay of the
   // land is.  If it's mostly SQLITE_NOTADB, then the database should
   // be razed.
   err = ExecuteAndReturnErrorCode("PRAGMA auto_vacuum");

[afakhry, 2016/08/18 21:32:29]

Shouldn't we use Execute() instead so that the error callback is invoked, in
order to give a chance to clients to Raze() the DB on a catastrophic error?

[Scott Hess - ex-Googler, 2016/08/18 22:06:53]

Sqlite.OpenProbeFailure tracks the specific SQLite error.  Execute() followed by
GetErrorCode() could work, but if the error callback does RazeAndClose() or
calls recovery code, GetErrorCode() will return the last error from _that_ (kind
of like how errno needs to be pinned in error-handling code if you want to log
it).  OnSqliteError() will call into the regular error-handling, including
callback.

[afakhry, 2016/08/19 15:25:09]

Oh yes, sorry, I didn't notice the OnSqliteError() even though I wrote something
similar in one of the patches in my other CL. :D

-  if (err != SQLITE_OK)
+  if (err != SQLITE_OK) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("Sqlite.OpenProbeFailure", err);
+    OnSqliteError(err, NULL, "PRAGMA auto_vacuum");

[afakhry, 2016/08/19 15:25:09]

Nit: NULL --> nullptr.

[Scott Hess - ex-Googler, 2016/08/19 21:51:03]

Done.

+
+    // Retry or bail out if the error handler poisoned the handle.
+    // TODO(shess): Move this handling to one place (see also sqlite3_open and
+    // secure_delete).  Possibly a wrapper function?
+    if (poisoned_) {
+      Close();
+      if (retry_flag == RETRY_ON_POISON)
+        return OpenInternal(file_name, NO_RETRY);
+      return false;
+    }
+  }
 
 #if defined(OS_IOS) && defined(USE_SYSTEM_SQLITE)
   // The version of SQLite shipped with iOS doesn't enable ICU, which includes
   // REGEXP support. Add it in dynamically.
   err = sqlite3IcuInit(db_);
   DCHECK_EQ(err, SQLITE_OK) << "Could not enable ICU support";
 #endif  // OS_IOS && USE_SYSTEM_SQLITE
 
   // If indicated, lock up the database before doing anything else, so
   // that the following code doesn't have to deal with locking.
   // TODO(shess): This code is brittle.  Find the cases where code
   // doesn't request |exclusive_locking_| and audit that it does the
   // right thing with SQLITE_BUSY, and that it doesn't make
   // assumptions about who might change things in the database.
   // http://crbug.com/56559
   if (exclusive_locking_) {
     // TODO(shess): This should probably be a failure.  Code which
     // requests exclusive locking but doesn't get it is almost certain
     // to be ill-tested.
     ignore_result(Execute("PRAGMA locking_mode=EXCLUSIVE"));
   }
 
   // http://www.sqlite.org/pragma.html#pragma_journal_mode
   // DELETE (default) - delete -journal file to commit.
   // TRUNCATE - truncate -journal file to commit.
   // PERSIST - zero out header of -journal file to commit.
   // TRUNCATE should be faster than DELETE because it won't need directory
   // changes for each transaction.  PERSIST may break the spirit of using
   // secure_delete.
   ignore_result(Execute("PRAGMA journal_mode = TRUNCATE"));

[afakhry, 2016/08/18 21:32:29]

Why retrying up there and not here?

[Scott Hess - ex-Googler, 2016/08/18 22:06:52]

Any Execute() calls in between will cause poisoned_ to be set, and then the
retry under secure_delete should handle it.  I haven't been able to formulate a
case where any of these would fail catastrophically but secure_delete would
succeed.  That case would also apply to the auto_vacuum case I'm changing, here,
but pushing it as early as possible removes all the busy-work of firing errors
over and over again.

I _believe_ that the majority of the poison-type errors in Open() will happen
either in sqlite3_open() or probing the first page (which the auto_vacuum check
does).  I think the next-most-likely case will be in loading the schema, which I
don't think anything in Open() does, though I'm semi-seriously considering
adding a call to force that.

Put another way, I don't want to litter this code with retries, I'd rather
refactor to make a helper and then put a retry around it.  But I don't think
your CL will profit from pending on that.

[afakhry, 2016/08/19 15:25:09]

Makes sense. Thanks. Acknowledged.

 
   const base::TimeDelta kBusyTimeout =
     base::TimeDelta::FromSeconds(kBusyTimeoutSeconds);
 
   if (page_size_ != 0) {
     // Enforce SQLite restrictions on |page_size_|.
     DCHECK(!(page_size_ & (page_size_ - 1)))
         << " page_size_ " << page_size_ << " is not a power of two.";
     const int kSqliteMaxPageSize = 32768;  // from sqliteLimit.h
     DCHECK_LE(page_size_, kSqliteMaxPageSize);
@@ skipping 229 matching lines @@
   ignore_result(Execute(kNoWritableSchema));
 
   return ret;
 }
 
 base::TimeTicks TimeSource::Now() {
   return base::TimeTicks::Now();
 }
 
 }  // namespace sql