[sql] Change DoesStuffExist() to work with ScopedErrorIgnorer.

[sql] Change DoesStuffExist() to work with ScopedErrorIgnorer.

This not working was preventing certain corruption-testing tests from
being written.

Also modified Does{Table,Index,Column}Exist() to reflect that these
names are _not_ case sensitive.  It is not possible to have distinct
tables [Foo] and [foo].

BUG=none

Committed: https://crrev.com/92a2ab1f1d0cdff39e6d506c4cb6a80038baeeb3
Cr-Commit-Position: refs/heads/master@{#324337}

[Scott Hess - ex-Googler, 2015-04-08 21:17:18]

shess@chromium.org changed reviewers:
+ maniscalco@chromium.org

[Scott Hess - ex-Googler, 2015-04-08 21:17:19]

I guess this makes sense.  The case-sensitivity change was because I tried
rewriting with a different SQLite API, but unfortunately turning it on adds
footprint to SQLite to I decided against that, for now.

[maniscalco, 2015-04-08 22:40:27]

Thanks for doing this!  I've verified that this patch enables me to write a
corruption detection test for sync without having to resort to
SetLogAssertHandler hacks.

LGTM with one question about some comments.

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

https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode727
sql/connection.cc:727: // This is evidence of a syntax error in the incoming
SQL.
I'm thinking about this comment and the one in GetUntrackedStatement below. 
Does it make sense to expand it to something like:

// This is evidence of a syntax error in the incoming SQL or database
corruption.

?

[Scott Hess - ex-Googler, 2015-04-08 22:49:20]

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

https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode727
sql/connection.cc:727: // This is evidence of a syntax error in the incoming
SQL.
On 2015/04/08 22:40:27, maniscalco wrote:
> I'm thinking about this comment and the one in GetUntrackedStatement below. 
> Does it make sense to expand it to something like:
> 
> // This is evidence of a syntax error in the incoming SQL or database
> corruption.
> 
> ?

Maybe, it depends.  Corruption can cause almost any problem you can imagine, so
I've generally not made a stink about noting potential problems.  The reason I
mentioned it below is because the code at that point will work fine without the
additional check in a Release build, and will never be reached in a Debug build.
 Since the syntax is broadly correct, it should only ever fire in the corruption
case.

I think :-).

[Scott Hess - ex-Googler, 2015-04-08 22:49:23]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2015-04-08 22:51:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1069313004/1

[commit-bot: I haz the power, 2015-04-09 02:00:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-09 02:00:55]

Patchset 1 (id:??) landed as
https://crrev.com/92a2ab1f1d0cdff39e6d506c4cb6a80038baeeb3
Cr-Commit-Position: refs/heads/master@{#324337}

[maniscalco, 2015-04-09 15:11:08]

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

https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode727
sql/connection.cc:727: // This is evidence of a syntax error in the incoming
SQL.
On 2015/04/08 22:49:20, Scott Hess wrote:
> On 2015/04/08 22:40:27, maniscalco wrote:
> > I'm thinking about this comment and the one in GetUntrackedStatement below. 
> > Does it make sense to expand it to something like:
> > 
> > // This is evidence of a syntax error in the incoming SQL or database
> > corruption.
> > 
> > ?
> 
> Maybe, it depends.  Corruption can cause almost any problem you can imagine,
so
> I've generally not made a stink about noting potential problems.  The reason I
> mentioned it below is because the code at that point will work fine without
the
> additional check in a Release build, and will never be reached in a Debug
build.
>  Since the syntax is broadly correct, it should only ever fire in the
corruption
> case.
> 
> I think :-).

That makes sense to me.

[maniscalco, 2015-04-09 20:40:52]

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

https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode728
sql/connection.cc:728: DLOG_IF(FATAL, !ShouldIgnoreSqliteError(rc))
I just realized that ShouldIgnoreSqliteError is not a pure function since it
calls ScopedErrorIgnorer::ShouldIgnore which modifies the underlying
errors_ignored_ set.

In Debug builds, the second argument to DLOG_IF (ShouldIgnoreSqliteError) will
be evaluated as expected.

However, in Release builds, DLOG_IF is replaced with EAT_STREAM_PARAMETERS and
so the second argument won't be evaluated, which means that rc will never be
added to the errors_ignored_ set in Release builds.

WDYT about pulling the call to ShouldIgnoreSqliteError out of the DLOG_IF?  I'm
thinking it would look something like this:

    const bool should_ignore = ShouldIgnoreSqliteError(rc);
    ALLOW_UNUSED_LOCAL(should_ignore);
    DLOG_IF(FATAL, !should_ignore) << "SQL compile error " << GetErrorMessage();

[Scott Hess - ex-Googler, 2015-04-09 20:46:30]

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

https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode728
sql/connection.cc:728: DLOG_IF(FATAL, !ShouldIgnoreSqliteError(rc))
On 2015/04/09 20:40:52, maniscalco wrote:
> I just realized that ShouldIgnoreSqliteError is not a pure function since it
> calls ScopedErrorIgnorer::ShouldIgnore which modifies the underlying
> errors_ignored_ set.
> 
> In Debug builds, the second argument to DLOG_IF (ShouldIgnoreSqliteError) will
> be evaluated as expected.
> 
> However, in Release builds, DLOG_IF is replaced with EAT_STREAM_PARAMETERS and
> so the second argument won't be evaluated, which means that rc will never be
> added to the errors_ignored_ set in Release builds.
> 
> WDYT about pulling the call to ShouldIgnoreSqliteError out of the DLOG_IF? 
I'm
> thinking it would look something like this:
> 
>     const bool should_ignore = ShouldIgnoreSqliteError(rc);
>     ALLOW_UNUSED_LOCAL(should_ignore);
>     DLOG_IF(FATAL, !should_ignore) << "SQL compile error " <<
GetErrorMessage();

At this site, OnSqliteError() should call ShouldIgnoreSqliteError().  But it
does look like the form in that code is maybe usable here:
  if (!ShouldIgnoreSqliteError(err))
    DLOG(FATAL) << "SQL compile error " << GetErrorMessage();

The other case won't have that backup.  I can make a CL, but just for my info,
are you seeing this actually fail, or just hypothetical?

[maniscalco, 2015-04-09 21:17:42]

On 2015/04/09 20:46:30, Scott Hess wrote:
> https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc
> File sql/connection.cc (right):
> 
> https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode728
> sql/connection.cc:728: DLOG_IF(FATAL, !ShouldIgnoreSqliteError(rc))
> On 2015/04/09 20:40:52, maniscalco wrote:
> > I just realized that ShouldIgnoreSqliteError is not a pure function since it
> > calls ScopedErrorIgnorer::ShouldIgnore which modifies the underlying
> > errors_ignored_ set.
> > 
> > In Debug builds, the second argument to DLOG_IF (ShouldIgnoreSqliteError)
will
> > be evaluated as expected.
> > 
> > However, in Release builds, DLOG_IF is replaced with EAT_STREAM_PARAMETERS
and
> > so the second argument won't be evaluated, which means that rc will never be
> > added to the errors_ignored_ set in Release builds.
> > 
> > WDYT about pulling the call to ShouldIgnoreSqliteError out of the DLOG_IF? 
> I'm
> > thinking it would look something like this:
> > 
> >     const bool should_ignore = ShouldIgnoreSqliteError(rc);
> >     ALLOW_UNUSED_LOCAL(should_ignore);
> >     DLOG_IF(FATAL, !should_ignore) << "SQL compile error " <<
> GetErrorMessage();
> 
> At this site, OnSqliteError() should call ShouldIgnoreSqliteError().  But it
> does look like the form in that code is maybe usable here:
>   if (!ShouldIgnoreSqliteError(err))
>     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
> 
> The other case won't have that backup.  I can make a CL, but just for my info,
> are you seeing this actually fail, or just hypothetical?

Actually fail.  Check out the trybot failures on
https://codereview.chromium.org/1072093002/ (patch set 4).

[Scott Hess - ex-Googler, 2015-04-09 21:29:44]

On 2015/04/09 21:17:42, maniscalco wrote:
> On 2015/04/09 20:46:30, Scott Hess wrote:
> > https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc
> > File sql/connection.cc (right):
> > 
> >
https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode728
> > sql/connection.cc:728: DLOG_IF(FATAL, !ShouldIgnoreSqliteError(rc))
> > On 2015/04/09 20:40:52, maniscalco wrote:
> > > I just realized that ShouldIgnoreSqliteError is not a pure function since
it
> > > calls ScopedErrorIgnorer::ShouldIgnore which modifies the underlying
> > > errors_ignored_ set.
> > > 
> > > In Debug builds, the second argument to DLOG_IF (ShouldIgnoreSqliteError)
> will
> > > be evaluated as expected.
> > > 
> > > However, in Release builds, DLOG_IF is replaced with EAT_STREAM_PARAMETERS
> and
> > > so the second argument won't be evaluated, which means that rc will never
be
> > > added to the errors_ignored_ set in Release builds.
> > > 
> > > WDYT about pulling the call to ShouldIgnoreSqliteError out of the DLOG_IF?

> > I'm
> > > thinking it would look something like this:
> > > 
> > >     const bool should_ignore = ShouldIgnoreSqliteError(rc);
> > >     ALLOW_UNUSED_LOCAL(should_ignore);
> > >     DLOG_IF(FATAL, !should_ignore) << "SQL compile error " <<
> > GetErrorMessage();
> > 
> > At this site, OnSqliteError() should call ShouldIgnoreSqliteError().  But it
> > does look like the form in that code is maybe usable here:
> >   if (!ShouldIgnoreSqliteError(err))
> >     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
> > 
> > The other case won't have that backup.  I can make a CL, but just for my
info,
> > are you seeing this actually fail, or just hypothetical?
> 
> Actually fail.  Check out the trybot failures on
> https://codereview.chromium.org/1072093002/ (patch set 4).

I suspect my change won't fix that, since it's reporting a bunch of failures
with error 11, so it should be reaching the OnSqliteError() code in there.  Do
you have an error callback?  The error-ignorer stuff was geared towards
non-error-callback connections (which is most of them).  I'm not sure where that
would take things, like maybe the untracked statements need some error-callback
magic, too.

[The untracked statements are a bit weird, they exist to prevent a particular
statement-caching problem which I cannot recall the details of.]

[maniscalco, 2015-04-09 21:34:32]

On 2015/04/09 21:29:44, Scott Hess wrote:
> On 2015/04/09 21:17:42, maniscalco wrote:
> > On 2015/04/09 20:46:30, Scott Hess wrote:
> > > https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc
> > > File sql/connection.cc (right):
> > > 
> > >
> https://codereview.chromium.org/1069313004/diff/1/sql/connection.cc#newcode728
> > > sql/connection.cc:728: DLOG_IF(FATAL, !ShouldIgnoreSqliteError(rc))
> > > On 2015/04/09 20:40:52, maniscalco wrote:
> > > > I just realized that ShouldIgnoreSqliteError is not a pure function
since
> it
> > > > calls ScopedErrorIgnorer::ShouldIgnore which modifies the underlying
> > > > errors_ignored_ set.
> > > > 
> > > > In Debug builds, the second argument to DLOG_IF
(ShouldIgnoreSqliteError)
> > will
> > > > be evaluated as expected.
> > > > 
> > > > However, in Release builds, DLOG_IF is replaced with
EAT_STREAM_PARAMETERS
> > and
> > > > so the second argument won't be evaluated, which means that rc will
never
> be
> > > > added to the errors_ignored_ set in Release builds.
> > > > 
> > > > WDYT about pulling the call to ShouldIgnoreSqliteError out of the
DLOG_IF?
> 
> > > I'm
> > > > thinking it would look something like this:
> > > > 
> > > >     const bool should_ignore = ShouldIgnoreSqliteError(rc);
> > > >     ALLOW_UNUSED_LOCAL(should_ignore);
> > > >     DLOG_IF(FATAL, !should_ignore) << "SQL compile error " <<
> > > GetErrorMessage();
> > > 
> > > At this site, OnSqliteError() should call ShouldIgnoreSqliteError().  But
it
> > > does look like the form in that code is maybe usable here:
> > >   if (!ShouldIgnoreSqliteError(err))
> > >     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
> > > 
> > > The other case won't have that backup.  I can make a CL, but just for my
> info,
> > > are you seeing this actually fail, or just hypothetical?
> > 
> > Actually fail.  Check out the trybot failures on
> > https://codereview.chromium.org/1072093002/ (patch set 4).
> 
> I suspect my change won't fix that, since it's reporting a bunch of failures
> with error 11, so it should be reaching the OnSqliteError() code in there.  Do
> you have an error callback?  The error-ignorer stuff was geared towards
> non-error-callback connections (which is most of them).  I'm not sure where
that
> would take things, like maybe the untracked statements need some
error-callback
> magic, too.
> 
> [The untracked statements are a bit weird, they exist to prevent a particular
> statement-caching problem which I cannot recall the details of.]

Hmm.  Yes, I have an error callback installed.  I'm re-running the trybots with
your patch merged in (https://codereview.chromium.org/1072093002/ patch set 5),
which I think will all pass.