[sql] Log tag with sqlite errors.

[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

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

[Scott Hess - ex-Googler, 2013-09-25 18:32:02]

There are cases where code uses un-tagged connections, which will get a bonus
space, but I'm not sure I care enough to make that pretty.  Also, I would prefer
we didn't have any such un-tagged connections.

[Scott Hess - ex-Googler, 2013-09-25 18:32:07]

There are cases where code uses un-tagged connections, which will get a bonus
space, but I'm not sure I care enough to make that pretty.  Also, I would prefer
we didn't have any such un-tagged connections.

[Greg Spencer (Chromium), 2013-09-25 18:36:30]

https://codereview.chromium.org/24638002/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/24638002/diff/1/sql/connection.cc#newcode1030
sql/connection.cc:1030: LOG(ERROR) << histogram_tag_ << " sqlite error " << err
Would it make sense to use sqlite3_column_database_name to get the database name
from the statement?

Otherwise, I agree with you that we don't want untagged ops.

[Scott Hess - ex-Googler, 2013-09-25 19:37:04]

Added logging of the involved sql code.  PTAL?

The only downside I can think of is that this info could be verbose.  OTOH, in
theory this stuff should never happen in the first place :-).

https://codereview.chromium.org/24638002/diff/1/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/24638002/diff/1/sql/connection.cc#newcode1030
sql/connection.cc:1030: LOG(ERROR) << histogram_tag_ << " sqlite error " << err
On 2013/09/25 18:36:30, Greg Spencer (Chromium) wrote:
> Would it make sense to use sqlite3_column_database_name to get the database
name
> from the statement?
> 
> Otherwise, I agree with you that we don't want untagged ops.

http://www.sqlite.org/c3ref/column_database_name.html says they are only enabled
if compiled the right way.  Also, thinking on it, I bet there isn't even a
statement for these errors, it's probably failing even accessing the schema
info.

More achievable might be to add a line for sqlite3_sql() if stmt is non-NULL. 
That would not cover calls to Execute(), though... but it would probably be easy
to modify things to hit that case, too.  That should make it pretty easy to
backtrack to which caller is calling it, for all the good that will do.

OK, I'm going to tweak things a little more.

[Greg Spencer (Chromium), 2013-09-25 20:01:17]

LGTM

https://codereview.chromium.org/24638002/diff/1002/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/24638002/diff/1002/sql/connection.cc#newcode1038
sql/connection.cc:1038: LOG(ERROR) << "sql: " << sql;
nits: You might want to also output "sqlite error" as part of this message so
that they're easier to grep for.  It might also be nice to merge this with the
line above so that all information is on one line to keep from having
interleaved messages from other threads.  That might be too much on one line
though.

[Scott Hess - ex-Googler, 2013-09-25 20:22:35]

ptal

https://codereview.chromium.org/24638002/diff/1002/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/24638002/diff/1002/sql/connection.cc#newcode1038
sql/connection.cc:1038: LOG(ERROR) << "sql: " << sql;
On 2013/09/25 20:01:18, Greg Spencer (Chromium) wrote:
> nits: You might want to also output "sqlite error" as part of this message so
> that they're easier to grep for.  It might also be nice to merge this with the
> line above so that all information is on one line to keep from having
> interleaved messages from other threads.  That might be too much on one line
> though.

I'm moving it up to the same line, just to be safe.  Possibly things will get
trimmed for very long lines, but it should narrow things down a lot even in
those cases.

[Greg Spencer (Chromium), 2013-09-25 20:27:40]

Still LGTM.

[commit-bot: I haz the power, 2013-09-25 20:37:53]

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

[commit-bot: I haz the power, 2013-09-25 21:59:39]

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

[commit-bot: I haz the power, 2013-09-26 18:37:10]

Change committed as 225518