Add diagnostics to debug database open problems.

Add diagnostics to debug database open problems.

Currently the histograms are binned together for all errors, but
knowing open-specific errors is helpful for dealing with problems on
open.

BUG=159490
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171862

[Scott Hess - ex-Googler, 2012-12-07 18:12:40]

I'm working on a modification to Open() which will automatically reset the
database if it's beyond recovery, including attempting to delete it if it cannot
be opened.  I'd like to pull these histograms forward because after reading
through the sqlite code, I suspect that the set of errors from sqlite3_open() is
probably pretty restricted.  I'd rather not chance incorrectly solving
hypothetical cases.

[erikwright (departed), 2012-12-07 18:23:15]

What is the impact of the PRAGMA auto_vacuum?

On Fri, Dec 7, 2012 at 10:12 AM, <shess@chromium.org> wrote:

> Reviewers: erikwright,
>
> Message:
> I'm working on a modification to Open() which will automatically reset the
> database if it's beyond recovery, including attempting to delete it if it
> cannot
> be opened.  I'd like to pull these histograms forward because after reading
> through the sqlite code, I suspect that the set of errors from
> sqlite3_open() is
> probably pretty restricted.  I'd rather not chance incorrectly solving
> hypothetical cases.
>
> Description:
> Add diagnostics to debug database open problems.
>
> Currently the histograms are binned together for all errors, but
> knowing open-specific errors is helpful for dealing with problems on
> open.
>
> BUG=159490
>
>
> Please review this at
https://codereview.chromium.**org/11468024/<https://codereview.chromium.org/1...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>   M sql/connection.cc
>
>
> Index: sql/connection.cc
> diff --git a/sql/connection.cc b/sql/connection.cc
> index 1de9fc5cfa536c65fd2328640f5d6e**4ec443a08f..**
> 99bc38173ee2daa7cb486abc2deddc**d674a251a3 100644
> --- a/sql/connection.cc
> +++ b/sql/connection.cc
> @@ -8,6 +8,7 @@
>
>  #include "base/file_path.h"
>  #include "base/logging.h"
> +#include "base/metrics/histogram.h"
>  #include "base/string_util.h"
>  #include "base/stringprintf.h"
>  #include "base/utf_string_conversions.**h"
> @@ -538,12 +539,26 @@ bool Connection::OpenInternal(const std::string&
> file_name) {
>
>    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;
>      return false;
>    }
>
> +  // 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.
> +  // 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");
> +  if (err != SQLITE_OK)
> +    UMA_HISTOGRAM_ENUMERATION("**Sqlite.OpenProbeFailure", err & 0xff,
> 50);
> +
>    // 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
>
>
>

[Scott Hess - ex-Googler, 2012-12-07 18:49:50]

On 2012/12/07 18:23:15, erikwright wrote:
> What is the impact of the PRAGMA auto_vacuum?

Various settings (auto_vacuum, encoding, schema_version, page_count, others) are
stored in the header area of the first page in the file.  Reading any of those
settings will cause SQLite to read that page and do a number of local
validations (which don't require reading other parts of the file).  So the file
needs to have "SQLite format 3", be at least one page in size, record a page
count consistent with the file size, etc.

For a newly-created database, any of these will require the value which will be
used when the database is first written, which at this point in the code will be
the default value.  It won't select anything, though.

[Scott Hess - ex-Googler, 2012-12-07 18:50:22]

On 2012/12/07 18:49:50, shess wrote:
> On 2012/12/07 18:23:15, erikwright wrote:
> > What is the impact of the PRAGMA auto_vacuum?
> 
> Various settings (auto_vacuum, encoding, schema_version, page_count, others)
are
> stored in the header area of the first page in the file.  Reading any of those
> settings will cause SQLite to read that page and do a number of local
> validations (which don't require reading other parts of the file).  So the
file
> needs to have "SQLite format 3", be at least one page in size, record a page
> count consistent with the file size, etc.
> 
> For a newly-created database, any of these will require the value which will
be
> used when the database is first written, which at this point in the code will
be
> the default value.  It won't select anything, though.

Sorry, "select" is overloaded in sql/ - I meant it won't change the setting's
value, just ask what it currently is.

[erikwright (departed), 2012-12-07 19:36:59]

LGTM.

On Fri, Dec 7, 2012 at 10:50 AM, <shess@chromium.org> wrote:

> On 2012/12/07 18:49:50, shess wrote:
>
>> On 2012/12/07 18:23:15, erikwright wrote:
>> > What is the impact of the PRAGMA auto_vacuum?
>>
>
>  Various settings (auto_vacuum, encoding, schema_version, page_count,
>> others)
>>
> are
>
>> stored in the header area of the first page in the file.  Reading any of
>> those
>> settings will cause SQLite to read that page and do a number of local
>> validations (which don't require reading other parts of the file).  So the
>>
> file
>
>> needs to have "SQLite format 3", be at least one page in size, record a
>> page
>> count consistent with the file size, etc.
>>
>
>  For a newly-created database, any of these will require the value which
>> will
>>
> be
>
>> used when the database is first written, which at this point in the code
>> will
>>
> be
>
>> the default value.  It won't select anything, though.
>>
>
> Sorry, "select" is overloaded in sql/ - I meant it won't change the
> setting's
> value, just ask what it currently is.
>
>
https://codereview.chromium.**org/11468024/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2012-12-07 20:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/11468024/1

[commit-bot: I haz the power, 2012-12-07 22:14:51]

Change committed as 171862