[sql] Log tag with sqlite errors.

Log lines like:
ERROR:connection.cc(1007)] sqlite error 266, errno 5: disk I/O error
would be ever so much more useful if they indicated which database
they were associated with.  This logs the histogram tag, which indicates
the logical database (the code which owns this connection).  [As
opposed to the filesystem name.]

BUG=none

Review URL: https://codereview.chromium.org/24638002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@225518 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 23 insertions, 10 deletions

diff --git a/sql/connection.cc b/sql/connection.cc
index 0b49786..0f40779 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -641,7 +641,7 @@ bool Connection::Execute(const char* sql) {
 
   int error = ExecuteAndReturnErrorCode(sql);
   if (error != SQLITE_OK)
-    error = OnSqliteError(error, NULL);
+    error = OnSqliteError(error, NULL, sql);
 
   // This needs to be a FATAL log because the error case of arriving here is
   // that there's a malformed SQL statement. This can arise in development if
@@ -702,7 +702,7 @@ scoped_refptr<Connection::StatementRef> Connection::GetUniqueStatement(
     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
 
     // It could also be database corruption.
-    OnSqliteError(rc, NULL);
+    OnSqliteError(rc, NULL, sql);
     return new StatementRef(NULL, NULL, false);
   }
   return new StatementRef(this, stmt, true);
@@ -864,7 +864,7 @@ bool Connection::OpenInternal(const std::string& file_name,
     // purposes.
     UMA_HISTOGRAM_SPARSE_SLOWLY("Sqlite.OpenFailure", err);
 
-    OnSqliteError(err, NULL);
+    OnSqliteError(err, NULL, "-- sqlite3_open()");
     bool was_poisoned = poisoned_;
     Close();
 
@@ -1022,14 +1022,19 @@ void Connection::AddTaggedHistogram(const std::string& name,
     histogram->Add(sample);
 }
 
-int Connection::OnSqliteError(int err, sql::Statement *stmt) {
+int Connection::OnSqliteError(int err, sql::Statement *stmt, const char* sql) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("Sqlite.Error", err);
   AddTaggedHistogram("Sqlite.Error", err);
 
   // Always log the error.
-  LOG(ERROR) << "sqlite error " << err
+  if (!sql && stmt)
+    sql = stmt->GetSQLStatement();
+  if (!sql)
+    sql = "-- unknown";
+  LOG(ERROR) << histogram_tag_ << " sqlite error " << err
              << ", errno " << GetLastErrno()
-             << ": " << GetErrorMessage();
+             << ": " << GetErrorMessage()
+             << ", sql: " << sql;
 
   if (!error_callback_.is_null()) {
     // Fire from a copy of the callback in case of reentry into
diff --git a/sql/connection.h b/sql/connection.h
index 5475a84..ec76e3b 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -516,9 +516,17 @@ class SQL_EXPORT Connection {
   void StatementRefCreated(StatementRef* ref);
   void StatementRefDeleted(StatementRef* ref);
 
-  // Called by Statement objects when an sqlite function returns an error.
-  // The return value is the error code reflected back to client code.
-  int OnSqliteError(int err, Statement* stmt);
+  // Called when a sqlite function returns an error, which is passed
+  // as |err|.  The return value is the error code to be reflected
+  // back to client code.  |stmt| is non-NULL if the error relates to
+  // an sql::Statement instance.  |sql| is non-NULL if the error
+  // relates to non-statement sql code (Execute, for instance).  Both
+  // can be NULL, but both should never be set.
+  // NOTE(shess): Originally, the return value was intended to allow
+  // error handlers to transparently convert errors into success.
+  // Unfortunately, transactions are not generally restartable, so
+  // this did not work out.
+  int OnSqliteError(int err, Statement* stmt, const char* sql);
 
   // Like |Execute()|, but retries if the database is locked.
   bool ExecuteWithTimeout(const char* sql, base::TimeDelta ms_timeout)
diff --git a/sql/statement.cc b/sql/statement.cc
index da2c58f..67b7334 100644
--- a/sql/statement.cc
+++ b/sql/statement.cc
@@ -309,7 +309,7 @@ int Statement::CheckError(int err) {
   // Please don't add DCHECKs here, OnSqliteError() already has them.
   succeeded_ = (err == SQLITE_OK || err == SQLITE_ROW || err == SQLITE_DONE);
   if (!succeeded_ && ref_.get() && ref_->connection())
-    return ref_->connection()->OnSqliteError(err, this);
+    return ref_->connection()->OnSqliteError(err, this, NULL);
   return err;
 }