Bug 47859 – sqlite: show extended error codes in error logs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Evan Martin]

sqlite: show extended error codes in error logs

[Evan Martin]

Created attachment 71098 [details]
Patch

[Evan Martin]

This helped me track down
http://code.google.com/p/chromium/issues/detail?id=59420

[Tony Chang]

Comment on attachment 71098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review

> WebCore/platform/sql/SQLiteDatabase.cpp:76
> +    m_lastError = sqlite3_extended_result_codes(m_db, 1);

I'm not sure why m_lastError is a member variable.  It doesn't appear to be used other than in the open function.

Also, it looks like we only get here when m_lastError == SQLITE_OK.  Will sqlite3_extended_result_codes provide information when SQLITE_OK?  It seems weird that sqlite3_extended_result_codes returns the previous error.

[David Kilzer (:ddkilzer)]

Comment on attachment 71098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review

r=me, but please address the questions below before committing.

>> WebCore/platform/sql/SQLiteDatabase.cpp:76
>> +    m_lastError = sqlite3_extended_result_codes(m_db, 1);
> 
> I'm not sure why m_lastError is a member variable.  It doesn't appear to be used other than in the open function.
> 
> Also, it looks like we only get here when m_lastError == SQLITE_OK.  Will sqlite3_extended_result_codes provide information when SQLITE_OK?  It seems weird that sqlite3_extended_result_codes returns the previous error.

Making m_lastError a local variable should be a separate patch, but I agree that it doesn't need to be a member variable.

The sqlite3_extended_result_codes() function enables extended error codes for future database operations.

My only concern is whether enabling extended error codes has any performance impact.  (I suspect it doesn't because the error codes are just integers.)

> WebCore/platform/sql/SQLiteStatement.cpp:108
> +            error, m_query.ascii().data(), error, sqlite3_errmsg(m_database.sqlite3Handle()));

Isn't printing 'error' twice here redundant?

[Evan Martin]

Created attachment 71313 [details]
Patch

[Tony Chang]

Comment on attachment 71313 [details]
Patch

Carrying over ddkilzer's previous r+.

[WebKit Commit Bot]

Comment on attachment 71313 [details]
Patch

Clearing flags on attachment: 71313

Committed r70219: <http://trac.webkit.org/changeset/70219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Comment on attachment 71313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71313&action=review

> WebCore/platform/sql/SQLiteDatabase.cpp:80
> +    if (sqlite3_extended_result_codes(m_db, 1) != SQLITE_OK) {
> +        LOG_ERROR("SQLite database error when enabling extended errors - %s", sqlite3_errmsg(m_db));
> +        sqlite3_close(m_db);
> +        m_db = 0;
> +        return false;
> +    }

I'm not sure if a failure to enable extended result codes should cause the database open to fail, but I suppose it will be interesting to know when it doesn't work.

No change is required--I just noticed this after Tony re-added my r+ to this patch.

[Evan Martin]

Heh, just looked at the sqlite source.

int sqlite3_extended_result_codes(sqlite3 *db, int onoff){
  sqlite3_mutex_enter(db->mutex);
  db->errMask = onoff ? 0xffffffff : 0xff;
  sqlite3_mutex_leave(db->mutex);
  return SQLITE_OK;
}

So I guess that is dead code.  :\
I imagine they return the error code to leave it open to potentially returning errors in the future.  I wonder what sort of errors they could be.