Put debugging assertions into sql::Statement.

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index f7fef120d03deeab9fe6ebd4bb0190ceb428972d..6b89851f63783402870948e503e00b3ccddb743f 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -132,7 +132,7 @@ void Connection::Close() {
 
 void Connection::Preload() {
   if (!db_) {
-    NOTREACHED();
+    DLOG(FATAL) << "Cannot preload null db";
     return;
   }
 
@@ -142,7 +142,7 @@ void Connection::Preload() {
   if (!DoesTableExist("meta"))
     return;
   Statement dummy(GetUniqueStatement("SELECT * FROM meta"));
-  if (!dummy || !dummy.Step())
+  if (!dummy.Step())
     return;
 
 #if !defined(USE_SYSTEM_SQLITE)
@@ -166,7 +166,7 @@ bool Connection::BeginTransaction() {
     needs_rollback_ = false;
 
     Statement begin(GetCachedStatement(SQL_FROM_HERE, "BEGIN TRANSACTION"));
-    if (!begin || !begin.Run())
+    if (!begin.Run())
       return false;
   }
   transaction_nesting_++;
@@ -175,7 +175,7 @@ bool Connection::BeginTransaction() {
 
 void Connection::RollbackTransaction() {
   if (!transaction_nesting_) {
-    NOTREACHED() << "Rolling back a nonexistent transaction";
+    DLOG(FATAL) << "Rolling back a nonexistent transaction";
     return;
   }
 
@@ -192,7 +192,7 @@ void Connection::RollbackTransaction() {
 
 bool Connection::CommitTransaction() {
   if (!transaction_nesting_) {
-    NOTREACHED() << "Rolling back a nonexistent transaction";
+    DLOG(FATAL) << "Rolling back a nonexistent transaction";
     return false;
   }
   transaction_nesting_--;
@@ -208,15 +208,20 @@ bool Connection::CommitTransaction() {
   }
 
   Statement commit(GetCachedStatement(SQL_FROM_HERE, "COMMIT"));
-  if (!commit)
-    return false;
   return commit.Run();
 }
 
-bool Connection::Execute(const char* sql) {
+int Connection::ExecuteAndReturnErrorCode(const char* sql) {
   if (!db_)
     return false;
-  return sqlite3_exec(db_, sql, NULL, NULL, NULL) == SQLITE_OK;
+  return sqlite3_exec(db_, sql, NULL, NULL, NULL);
+}
+
+bool Connection::Execute(const char* sql) {
+  int error = ExecuteAndReturnErrorCode(sql);
+  if (error == SQLITE_ERROR)
+    DLOG(FATAL) << "SQL Error in " << sql;

[Scott Hess - ex-Googler, 2011/12/09 22:54:40]

In my final review, I went over the OnSqliteError() cases.  I agree with your
change to only call that for Step() and Run() in statement.  But I found myself
wondering if it shouldn't also apply to Execute() and GetUniqueStatement().  It
does not at this time, and it seems to me that this could imply that our
histograms underestimate the number of affected users (the error_delegate_ in
OnSqliteError is often something to record stats on SQLITE_CORRUPT and other
error codes).

WDYT?  Or stick to knitting and land that in a follow-up?

+  return error == SQLITE_OK;
 }
 
 bool Connection::ExecuteWithTimeout(const char* sql, base::TimeDelta timeout) {
@@ -225,7 +230,7 @@ bool Connection::ExecuteWithTimeout(const char* sql, base::TimeDelta timeout) {
 
   ScopedBusyTimeout busy_timeout(db_);
   busy_timeout.SetTimeout(timeout);
-  return sqlite3_exec(db_, sql, NULL, NULL, NULL) == SQLITE_OK;
+  return Execute(sql);
 }
 
 bool Connection::HasCachedStatement(const StatementID& id) const {
@@ -259,22 +264,28 @@ scoped_refptr<Connection::StatementRef> Connection::GetUniqueStatement(
 
   sqlite3_stmt* stmt = NULL;
   if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK) {
-    // Treat this as non-fatal, it can occur in a number of valid cases, and
-    // callers should be doing their own error handling.
-    DLOG(WARNING) << "SQL compile error " << GetErrorMessage();
+    // This is evidence of a syntax error in the incoming SQL.
+    DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
     return new StatementRef(this, NULL);
   }
   return new StatementRef(this, stmt);
 }
 
+bool Connection::IsSQLValid(const char* sql) {
+  sqlite3_stmt* stmt = NULL;
+  if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK)
+    return false;
+
+  sqlite3_finalize(stmt);
+  return true;
+}
+
 bool Connection::DoesTableExist(const char* table_name) const {
   // GetUniqueStatement can't be const since statements may modify the
   // database, but we know ours doesn't modify it, so the cast is safe.
   Statement statement(const_cast<Connection*>(this)->GetUniqueStatement(
       "SELECT name FROM sqlite_master "
       "WHERE type='table' AND name=?"));
-  if (!statement)
-    return false;
   statement.BindString(0, table_name);
   return statement.Step();  // Table exists if any row was returned.
 }
@@ -288,8 +299,6 @@ bool Connection::DoesColumnExist(const char* table_name,
   // Our SQL is non-mutating, so this cast is OK.
   Statement statement(const_cast<Connection*>(this)->GetUniqueStatement(
       sql.c_str()));
-  if (!statement)
-    return false;
 
   while (statement.Step()) {
     if (!statement.ColumnString(1).compare(column_name))
@@ -300,7 +309,7 @@ bool Connection::DoesColumnExist(const char* table_name,
 
 int64 Connection::GetLastInsertRowId() const {
   if (!db_) {
-    NOTREACHED();
+    DLOG(FATAL) << "Illegal use of connection without a db";
     return 0;
   }
   return sqlite3_last_insert_rowid(db_);
@@ -308,7 +317,7 @@ int64 Connection::GetLastInsertRowId() const {
 
 int Connection::GetLastChangeCount() const {
   if (!db_) {
-    NOTREACHED();
+    DLOG(FATAL) << "Illegal use of connection without a db";
     return 0;
   }
   return sqlite3_changes(db_);
@@ -339,7 +348,7 @@ const char* Connection::GetErrorMessage() const {
 
 bool Connection::OpenInternal(const std::string& file_name) {
   if (db_) {
-    NOTREACHED() << "sql::Connection is already open.";
+    DLOG(FATAL) << "sql::Connection is already open.";
     return false;
   }
 
@@ -370,7 +379,7 @@ bool Connection::OpenInternal(const std::string& file_name) {
     // which requests exclusive locking but doesn't get it is almost
     // certain to be ill-tested.
     if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
-      NOTREACHED() << "Could not set locking mode: " << GetErrorMessage();
+      DLOG(FATAL) << "Could not set locking mode: " << GetErrorMessage();
   }
 
   const base::TimeDelta kBusyTimeout =
@@ -384,17 +393,17 @@ bool Connection::OpenInternal(const std::string& file_name) {
     DCHECK_LE(page_size_, kSqliteMaxPageSize);
     const std::string sql = StringPrintf("PRAGMA page_size=%d", page_size_);
     if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
-      NOTREACHED() << "Could not set page size: " << GetErrorMessage();
+      DLOG(FATAL) << "Could not set page size: " << GetErrorMessage();
   }
 
   if (cache_size_ != 0) {
     const std::string sql = StringPrintf("PRAGMA cache_size=%d", cache_size_);
     if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
-      NOTREACHED() << "Could not set cache size: " << GetErrorMessage();
+      DLOG(FATAL) << "Could not set cache size: " << GetErrorMessage();
   }
 
   if (!ExecuteWithTimeout("PRAGMA secure_delete=ON", kBusyTimeout)) {
-    NOTREACHED() << "Could not enable secure_delete: " << GetErrorMessage();
+    DLOG(FATAL) << "Could not enable secure_delete: " << GetErrorMessage();
     Close();
     return false;
   }
@@ -404,8 +413,7 @@ bool Connection::OpenInternal(const std::string& file_name) {
 
 void Connection::DoRollback() {
   Statement rollback(GetCachedStatement(SQL_FROM_HERE, "ROLLBACK"));
-  if (rollback)
-    rollback.Run();
+  rollback.Run();
 }
 
 void Connection::StatementRefCreated(StatementRef* ref) {
@@ -416,7 +424,7 @@ void Connection::StatementRefCreated(StatementRef* ref) {
 void Connection::StatementRefDeleted(StatementRef* ref) {
   StatementRefSet::iterator i = open_statements_.find(ref);
   if (i == open_statements_.end())
-    NOTREACHED();
+    DLOG(FATAL) << "Could not find statement";
   else
     open_statements_.erase(i);
 }
@@ -436,7 +444,7 @@ int Connection::OnSqliteError(int err, sql::Statement *stmt) {
   if (error_delegate_.get())
     return error_delegate_->OnError(err, this, stmt);
   // The default handling is to assert on debug and to ignore on release.
-  NOTREACHED() << GetErrorMessage();
+  DLOG(FATAL) << GetErrorMessage();
   return err;
 }