Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
797983003:
Implement AffiliationDatabase to cache affiliation information in a SQLite DB. (Closed)
DescriptionImplement AffiliationDatabase to cache affiliation information in a SQLite DB.
BUG=437865
Committed: https://crrev.com/57b2c6c8543cc5b2dd5dd6844da381adf9e50db3
Cr-Commit-Position: refs/heads/master@{#312419}
Patch Set 1 : First #
Total comments: 10
Patch Set 2 : Addressed comments. #Patch Set 3 : Fix typo. #
Total comments: 6
Patch Set 4 : Nits #
Total comments: 16
Patch Set 5 : Addressed most comments by shess@. #Patch Set 6 : Use foreign keys with 'on delete cascade'. #Patch Set 7 : Fix nits. #
Total comments: 13
Patch Set 8 : Addressed comments from shess@. #Messages
Total messages: 29 (5 generated)
engedy@chromium.org changed reviewers: + gcasto@chromium.org, shess@chromium.org
Patchset #1 (id:1) has been deleted
@Garrett, please take a look. @Scott, in case you are back, could you please look at the SQLite specifics, and tell me if anything seems out of place?
I don't remember if I brought this up before, but have you given any thought to using leveldb + protocol buffers instead of SQL? It doesn't seem like we really need relational semantics, and in fact it would be simpler to deal with the database if all facets for an affiliation were grouped together instead of the facets and the mapping being split across tables. The biggest problem looks like generating unique keys for the data since we don't have one to start with. Otherwise it _seems_ like using components/leveldb_proto would be less code and less complicated. I don't necessarily have a very strong preference here, but just wanted to make sure we considered the option. https://codereview.chromium.org/797983003/diff/20001/components/password_mana... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:19: const int kCompatibleVersion = 2; Why are these both not 1? https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:63: return false; Nit: What about doing while (statement.Step()) { ... } return !result.empty(); ? https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:70: } while (statement.Step()); Nit: Newline here. https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:153: "(SELECT id from eq_classes WHERE last_update_time < ?)")); Shouldn't all classes with updates_time < cuttoff be deleted by this point? https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:226: affiliation.facets)) { Indentation
Hmm, I am not sure I understand your concern about generating unique keys. Could
you please elaborate?
Mostly I am using SQL for its indexing. If we used Level DB, how would we find
the affiliates of B after having prefetched A, yielding A -> {A, B, C}?
On 2014/12/12 23:46:34, engedy wrote:
> Hmm, I am not sure I understand your concern about generating unique keys.
Could
> you please elaborate?
>
Level DB maps strings -> strings. The abstractions that Chrome has easily allows
for string -> PB mappings. When we Store(), we want to add a new entry but we
don't actually care what the key is. Right now SQL does this for you by creating
a unique key, but we would have to do this ourselves in Level DB. One
alternative would be to store the protocol buffer as they key, but we lose some
nice abstractions and I think that keys aren't compressed nicely.
> Mostly I am using SQL for its indexing. If we used Level DB, how would we find
> the affiliates of B after having prefetched A, yielding A -> {A, B, C}?
Are A, B, and C all facets? Are you asking how to verify that B isn't in another
affiliation other than the one we have found for A? I suppose we could just
verify that no facet already exists in the database (either on Store() or
GetAffiliations...()), though that's not super fast (O(total_facets *
current_affiliation_facets)). I suppose it also depends on how much you trust
the server. Is that what you are asking or am I misinterpreting the question?
It's certainly possible that this, or reasons like it, makes using Level DB
unpalatable. I haven't spent a lot of time thinking about it, just wanted to
bring it up as a possibility. I think that the current code looks pretty good,
but as someone that has only a passable understanding of SQL, there are a few
things that took me a while to parse (e.g. the GetAffiliationsForFacet()
statement). I just wanted to make sure that there wasn't a way to simplify this
further.
Patchset #2 (id:40001) has been deleted
On 2014/12/13 00:13:05, Garrett Casto wrote:
> On 2014/12/12 23:46:34, engedy wrote:
> > Hmm, I am not sure I understand your concern about generating unique keys.
> Could
> > you please elaborate?
> >
>
> Level DB maps strings -> strings. The abstractions that Chrome has easily
allows
> for string -> PB mappings. When we Store(), we want to add a new entry but we
> don't actually care what the key is. Right now SQL does this for you by
creating
> a unique key, but we would have to do this ourselves in Level DB. One
> alternative would be to store the protocol buffer as they key, but we lose
some
> nice abstractions and I think that keys aren't compressed nicely.
>
> > Mostly I am using SQL for its indexing. If we used Level DB, how would we
find
> > the affiliates of B after having prefetched A, yielding A -> {A, B, C}?
>
> Are A, B, and C all facets? Are you asking how to verify that B isn't in
another
> affiliation other than the one we have found for A? I suppose we could just
> verify that no facet already exists in the database (either on Store() or
> GetAffiliations...()), though that's not super fast (O(total_facets *
> current_affiliation_facets)). I suppose it also depends on how much you trust
> the server. Is that what you are asking or am I misinterpreting the question?
>
> It's certainly possible that this, or reasons like it, makes using Level DB
> unpalatable. I haven't spent a lot of time thinking about it, just wanted to
> bring it up as a possibility. I think that the current code looks pretty good,
> but as someone that has only a passable understanding of SQL, there are a few
> things that took me a while to parse (e.g. the GetAffiliationsForFacet()
> statement). I just wanted to make sure that there wasn't a way to simplify
this
> further.
Yes, they are facet URIs. What I meant is that once we have prefetched and
stored {A, B, C}, the subsequent look-up might be keyed either by A, B, or C.
With LevelDB, we would have to store something for each of those facet URIs (as
keys) unless we are willing to do a full linear scan for each lookup.
We can either duplicate all data (inefficient), or store indirect references
(hacky), and this latter case is exactly what a relational DB solves nicely, so
I would go with that.
https://codereview.chromium.org/797983003/diff/20001/components/password_mana... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:19: const int kCompatibleVersion = 2; On 2014/12/12 21:48:29, Garrett Casto wrote: > Why are these both not 1? Arg, apologies, I was playing around with the version numbers and forgot them like this. https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:63: return false; On 2014/12/12 21:48:29, Garrett Casto wrote: > Nit: What about doing > > while (statement.Step()) { > ... > } > > return !result.empty(); > > ? Done. https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:70: } while (statement.Step()); On 2014/12/12 21:48:29, Garrett Casto wrote: > Nit: Newline here. Done. https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:153: "(SELECT id from eq_classes WHERE last_update_time < ?)")); On 2014/12/12 21:48:29, Garrett Casto wrote: > Shouldn't all classes with updates_time < cuttoff be deleted by this point? But of course. Fixed. https://codereview.chromium.org/797983003/diff/20001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:226: affiliation.facets)) { On 2014/12/12 21:48:29, Garrett Casto wrote: > Indentation Done.
On 2014/12/15 14:30:09, engedy wrote:
> On 2014/12/13 00:13:05, Garrett Casto wrote:
> > On 2014/12/12 23:46:34, engedy wrote:
> > > Hmm, I am not sure I understand your concern about generating unique keys.
> > Could
> > > you please elaborate?
> > >
> >
> > Level DB maps strings -> strings. The abstractions that Chrome has easily
> allows
> > for string -> PB mappings. When we Store(), we want to add a new entry but
we
> > don't actually care what the key is. Right now SQL does this for you by
> creating
> > a unique key, but we would have to do this ourselves in Level DB. One
> > alternative would be to store the protocol buffer as they key, but we lose
> some
> > nice abstractions and I think that keys aren't compressed nicely.
> >
> > > Mostly I am using SQL for its indexing. If we used Level DB, how would we
> find
> > > the affiliates of B after having prefetched A, yielding A -> {A, B, C}?
> >
> > Are A, B, and C all facets? Are you asking how to verify that B isn't in
> another
> > affiliation other than the one we have found for A? I suppose we could just
> > verify that no facet already exists in the database (either on Store() or
> > GetAffiliations...()), though that's not super fast (O(total_facets *
> > current_affiliation_facets)). I suppose it also depends on how much you
trust
> > the server. Is that what you are asking or am I misinterpreting the
question?
> >
> > It's certainly possible that this, or reasons like it, makes using Level DB
> > unpalatable. I haven't spent a lot of time thinking about it, just wanted to
> > bring it up as a possibility. I think that the current code looks pretty
good,
> > but as someone that has only a passable understanding of SQL, there are a
few
> > things that took me a while to parse (e.g. the GetAffiliationsForFacet()
> > statement). I just wanted to make sure that there wasn't a way to simplify
> this
> > further.
>
> Yes, they are facet URIs. What I meant is that once we have prefetched and
> stored {A, B, C}, the subsequent look-up might be keyed either by A, B, or C.
> With LevelDB, we would have to store something for each of those facet URIs
(as
> keys) unless we are willing to do a full linear scan for each lookup.
>
> We can either duplicate all data (inefficient), or store indirect references
> (hacky), and this latter case is exactly what a relational DB solves nicely,
so
> I would go with that.
All SQLite has is b-trees, in this case a b-tree for each table and an
additional b-tree for each index. leveldb would involve more code in the
client, but there's no reason to expect it to be less efficient, and there is a
certain upside to exposing your assumptions directly rather than having them
abstracted away where they aren't obvious.
On 2014/12/15 17:01:21, Scott Hess - On Leave wrote:
> On 2014/12/15 14:30:09, engedy wrote:
> > On 2014/12/13 00:13:05, Garrett Casto wrote:
> > > On 2014/12/12 23:46:34, engedy wrote:
> > > > Hmm, I am not sure I understand your concern about generating unique
keys.
> > > Could
> > > > you please elaborate?
> > > >
> > >
> > > Level DB maps strings -> strings. The abstractions that Chrome has easily
> > allows
> > > for string -> PB mappings. When we Store(), we want to add a new entry but
> we
> > > don't actually care what the key is. Right now SQL does this for you by
> > creating
> > > a unique key, but we would have to do this ourselves in Level DB. One
> > > alternative would be to store the protocol buffer as they key, but we lose
> > some
> > > nice abstractions and I think that keys aren't compressed nicely.
> > >
> > > > Mostly I am using SQL for its indexing. If we used Level DB, how would
we
> > find
> > > > the affiliates of B after having prefetched A, yielding A -> {A, B, C}?
> > >
> > > Are A, B, and C all facets? Are you asking how to verify that B isn't in
> > another
> > > affiliation other than the one we have found for A? I suppose we could
just
> > > verify that no facet already exists in the database (either on Store() or
> > > GetAffiliations...()), though that's not super fast (O(total_facets *
> > > current_affiliation_facets)). I suppose it also depends on how much you
> trust
> > > the server. Is that what you are asking or am I misinterpreting the
> question?
> > >
> > > It's certainly possible that this, or reasons like it, makes using Level
DB
> > > unpalatable. I haven't spent a lot of time thinking about it, just wanted
to
> > > bring it up as a possibility. I think that the current code looks pretty
> good,
> > > but as someone that has only a passable understanding of SQL, there are a
> few
> > > things that took me a while to parse (e.g. the GetAffiliationsForFacet()
> > > statement). I just wanted to make sure that there wasn't a way to simplify
> > this
> > > further.
> >
> > Yes, they are facet URIs. What I meant is that once we have prefetched and
> > stored {A, B, C}, the subsequent look-up might be keyed either by A, B, or
C.
> > With LevelDB, we would have to store something for each of those facet URIs
> (as
> > keys) unless we are willing to do a full linear scan for each lookup.
> >
> > We can either duplicate all data (inefficient), or store indirect references
> > (hacky), and this latter case is exactly what a relational DB solves nicely,
> so
> > I would go with that.
>
> All SQLite has is b-trees, in this case a b-tree for each table and an
> additional b-tree for each index. leveldb would involve more code in the
> client, but there's no reason to expect it to be less efficient, and there is
a
> certain upside to exposing your assumptions directly rather than having them
> abstracted away where they aren't obvious.
I am not saying that either solution is necessarily less efficient. All I am
saying is that if we wanted to make the LevelDB implementation efficient, we
would have to manually implement some things (namely: generating unique record
IDs, joining on foreign keys, distinguishing between two tables in one database)
that is already provided by a relational DB.
Happy to use LevelDB if you think it increases readability enough to offset
these drawbacks.
On 2014/12/15 17:20:36, engedy wrote:
> On 2014/12/15 17:01:21, Scott Hess - On Leave wrote:
> > On 2014/12/15 14:30:09, engedy wrote:
> > > On 2014/12/13 00:13:05, Garrett Casto wrote:
> > > > On 2014/12/12 23:46:34, engedy wrote:
> > > > > Hmm, I am not sure I understand your concern about generating unique
> keys.
> > > > Could
> > > > > you please elaborate?
> > > > >
> > > >
> > > > Level DB maps strings -> strings. The abstractions that Chrome has
easily
> > > allows
> > > > for string -> PB mappings. When we Store(), we want to add a new entry
but
> > we
> > > > don't actually care what the key is. Right now SQL does this for you by
> > > creating
> > > > a unique key, but we would have to do this ourselves in Level DB. One
> > > > alternative would be to store the protocol buffer as they key, but we
lose
> > > some
> > > > nice abstractions and I think that keys aren't compressed nicely.
> > > >
> > > > > Mostly I am using SQL for its indexing. If we used Level DB, how would
> we
> > > find
> > > > > the affiliates of B after having prefetched A, yielding A -> {A, B,
C}?
> > > >
> > > > Are A, B, and C all facets? Are you asking how to verify that B isn't in
> > > another
> > > > affiliation other than the one we have found for A? I suppose we could
> just
> > > > verify that no facet already exists in the database (either on Store()
or
> > > > GetAffiliations...()), though that's not super fast (O(total_facets *
> > > > current_affiliation_facets)). I suppose it also depends on how much you
> > trust
> > > > the server. Is that what you are asking or am I misinterpreting the
> > question?
> > > >
> > > > It's certainly possible that this, or reasons like it, makes using Level
> DB
> > > > unpalatable. I haven't spent a lot of time thinking about it, just
wanted
> to
> > > > bring it up as a possibility. I think that the current code looks pretty
> > good,
> > > > but as someone that has only a passable understanding of SQL, there are
a
> > few
> > > > things that took me a while to parse (e.g. the GetAffiliationsForFacet()
> > > > statement). I just wanted to make sure that there wasn't a way to
simplify
> > > this
> > > > further.
> > >
> > > Yes, they are facet URIs. What I meant is that once we have prefetched and
> > > stored {A, B, C}, the subsequent look-up might be keyed either by A, B, or
> C.
> > > With LevelDB, we would have to store something for each of those facet
URIs
> > (as
> > > keys) unless we are willing to do a full linear scan for each lookup.
> > >
> > > We can either duplicate all data (inefficient), or store indirect
references
> > > (hacky), and this latter case is exactly what a relational DB solves
nicely,
> > so
> > > I would go with that.
> >
> > All SQLite has is b-trees, in this case a b-tree for each table and an
> > additional b-tree for each index. leveldb would involve more code in the
> > client, but there's no reason to expect it to be less efficient, and there
is
> a
> > certain upside to exposing your assumptions directly rather than having them
> > abstracted away where they aren't obvious.
>
> I am not saying that either solution is necessarily less efficient. All I am
> saying is that if we wanted to make the LevelDB implementation efficient, we
> would have to manually implement some things (namely: generating unique record
> IDs, joining on foreign keys, distinguishing between two tables in one
database)
> that is already provided by a relational DB.
>
> Happy to use LevelDB if you think it increases readability enough to offset
> these drawbacks.
In the comment above I'm trying to be neutral, I'm not sure I'm excited by the
notion of a bunch of ad-hoc leveldb clients distributed around the system. But
I am also wondering about whether we should shift to something more centralized
than ad-hoc storage, and whether SQLite is that thing, or whether leveldb would
make things more manageable.
I may have had a slightly different idea than what Scott described, but my notion for LevelDB was just to store one table that was (unique_key -> Proto of Affiliations). Getting all affiliations for a facet would now require more work, but assuming you sorted the facets it would still be sub linear in the number of facets. Given that number of affiliations we are likely to have and the fact that LevelDB is more than an order of magnitude faster at scans than SQLite (http://leveldb.googlecode.com/svn/trunk/doc/benchmark.html), I assumed performance wouldn't be much of an issue. That being said, after thinking it over some more I'm not sure if LevelDB is particularly more compelling. So LGTM with some nits. https://codereview.chromium.org/797983003/diff/80001/components/password_mana... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/80001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:242: "last_update_time INTEGER)")) Nit: Generally on multi-line condition statements, braces are still used. https://codereview.chromium.org/797983003/diff/80001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:252: "set_id INTEGER NOT NULL)")) As above. https://codereview.chromium.org/797983003/diff/80001/components/password_mana... File components/password_manager/core/browser/affiliation_database.h (right): https://codereview.chromium.org/797983003/diff/80001/components/password_mana... components/password_manager/core/browser/affiliation_database.h:31: // that in odd cases basic database invariants will not hold. So, in what cases can the invariants not be true and what invariants can be broken? If this comment is going to be here it should prescribe what clients should do. If there isn't anything for right now, I would just remove this.
Argh -- I am afraid I have somewhat misunderstood some aspects of SQLite vs LevelDB debate; sorry if my responses did not make too much sense. All in all, it seems all of us is now OK with using SQLite, at least for now, so let us just do that. @Scott, could you please review for any potential SQL smells? (Plus also one question in particular, see my responses inline.) https://codereview.chromium.org/797983003/diff/80001/components/password_mana... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/80001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:242: "last_update_time INTEGER)")) On 2014/12/16 07:58:31, Garrett Casto wrote: > Nit: Generally on multi-line condition statements, braces are still used. Done. https://codereview.chromium.org/797983003/diff/80001/components/password_mana... components/password_manager/core/browser/affiliation_database.cc:252: "set_id INTEGER NOT NULL)")) On 2014/12/16 07:58:31, Garrett Casto wrote: > As above. Done. https://codereview.chromium.org/797983003/diff/80001/components/password_mana... File components/password_manager/core/browser/affiliation_database.h (right): https://codereview.chromium.org/797983003/diff/80001/components/password_mana... components/password_manager/core/browser/affiliation_database.h:31: // that in odd cases basic database invariants will not hold. On 2014/12/16 07:58:31, Garrett Casto wrote: > So, in what cases can the invariants not be true and what invariants can be > broken? If this comment is going to be here it should prescribe what clients > should do. If there isn't anything for right now, I would just remove this. Yeah, I had concerns about this comment myself. The reason I added it is because I realized the following. Assume the database gets corrupt (or the disk fills up, etc) and all Store() operations fail. In that case, any callers relying on that they would be able to Get() what they have just Store()-d to the DB will be surprised. In the worst case, this may mean that the AffiliationBackend will flood the server by fetching, over and over again, affiliation information that it is unable to store to the DB. I mostly wanted to highlight this (and remind myself to put in safeguards in the AffiliationBackend later). I am not certain this is the correct approach to error handling. But immediately giving up and calling it a day after seeing the first DB error seems a bit heavy handed. @Scott, WDYT?
Friendly ping.
On 2015/01/05 10:02:09, engedy wrote: > Friendly ping. Grr, sorry, I had some SQLite-side comments queued up, unsent. I just went back over the code, sorry about that. Nothing really major except the test request. https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:30: sql_connection_->set_histogram_tag("Affiliation"); This implies a corresponding change in tools/metrics/histograms/histograms.xml to add a new name to suffix SqliteDatabases. Then all the metrics dashboards should automatically fill. https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:159: void AffiliationDatabase::DeleteAllAffiliations() { This call seems unlikely to happen often enough to warrant caching the statements. [Yes, I agree that sql::Connection should just manage the caching internally and let client code just say "statement, ..."] https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:241: "id INTEGER PRIMARY KEY," For these two tables, is there any downside to having the id be re-used? With this phrasing, SQLite will choose MAX(id)+1 until it runs out of integers. If the highest-id rows are deleted, then next insert can give an id in that range. If it does matter, put in AUTOINCREMENT. AUTOINCREMENT adds a utility table to track things, so generally don't add it unless it actually matters. [AFAICT, the only way it could matter is if a commit wasn't atomic, so I wouldn't add AUTOINCREMENT.] https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:252: // SQLite build does not support the feature. Really? It should, I thought foreign-key support was added earlier. Maybe more has been added since :-). https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:278: DLOG(FATAL) << sql_connection_->GetErrorMessage(); What will ever cause this to resolve itself? As a general rule, database errors can be binned into two groups. One group is the set of errors caused by coding mistakes such as an error in the SQL statement, which are all fixable, so IMHO crashing in production is reasonable. The other group is the set of errors which is being injected by the environment, such as filesystem corruption, which the case above handles. The reason I bring this up now is because once you've shipped this, it's plausible/likely that if you attempt to tighten things up to be more pedantic, you'll find out that you've had tons of masked errors happening. Hmm, to be clear, what I'm saying is that in case of error, you should either blow away the database or send up a crash dump to signal that there's a bug to fix. https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database_unittest.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database_unittest.cc:233: } Suggest you put in some code that exercises the error handler. There is some helper code in sql/test, and a couple examples of injecting corruption around the codebase ("git gs CorruptSizeInHeader" finds some). You've got the right boilerplate in place, AFAICT, but my experience is that people's sql error handlers rot quickly. Hmm, the two cases I'd aim to test are "Does the handler blow away a corrupt database?" and "How does Init() work when given a corrupt database?" There have been cases where corruption at open time wasn't handled right so it persisted.
On 2014/12/16 14:56:34, engedy wrote: > On 2014/12/16 07:58:31, Garrett Casto wrote: > > So, in what cases can the invariants not be true and what invariants can be > > broken? If this comment is going to be here it should prescribe what clients > > should do. If there isn't anything for right now, I would just remove this. > > Yeah, I had concerns about this comment myself. The reason I added it is because > I realized the following. > > Assume the database gets corrupt (or the disk fills up, etc) and all Store() > operations fail. In that case, any callers relying on that they would be able to > Get() what they have just Store()-d to the DB will be surprised. In the worst > case, this may mean that the AffiliationBackend will flood the server by > fetching, over and over again, affiliation information that it is unable to > store to the DB. > > I mostly wanted to highlight this (and remind myself to put in safeguards in the > AffiliationBackend later). I am not certain this is the correct approach to > error handling. But immediately giving up and calling it a day after seeing the > first DB error seems a bit heavy handed. > > @Scott, WDYT? Oops, sorry I missed this. You got some flavor in my other comment, I suppose. WRT flooding the server, I doubt you're going to flood things. To my knowledge Chrome crashes cannot lead to SQLite corruption (unless you're using a dubious combination of SQLite pragmas). OS-level crashes are more of a mixed bag, since the OS is the one implementing the basic operations, but AFAICT the only really reliable way to manage corruption is to lose power unexpectedly. Anyhow, this is a one-in-a-million-users-per-day type of problem, so a successful RazeAndClose() should generate far less traffic than regular working users generate. There is a subset of users who's system is so messed up that self-recovery doesn't do the job. Full disks, broken disks, network disks, profiles not owned by the user running Chrome, anything goes. IMHO best-effort is the way to go - try, fail, try to raze, but defer real recovery until next launch by poisoning things (RazeAndClose() does this at the sql/ level, going forward that connection should fail at everything). Mostly I'd expect that users in this kind of situation don't manage to run Chrome for long periods between launches, but to some extent I think they are past where we can really help them out. All that said - you should consider having systems in place to detect and react to such flooding, like exponential backoffs and ways for server responses to request backoff.
@Scott, many thanks for the thorough review. PTAL. There is only one outstanding question regarding the handling of non-critical errors, please see inline. https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:30: sql_connection_->set_histogram_tag("Affiliation"); On 2015/01/05 21:14:45, Scott Hess wrote: > This implies a corresponding change in tools/metrics/histograms/histograms.xml > to add a new name to suffix SqliteDatabases. Then all the metrics dashboards > should automatically fill. Thanks! Done. https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:159: void AffiliationDatabase::DeleteAllAffiliations() { On 2015/01/05 21:14:45, Scott Hess wrote: > This call seems unlikely to happen often enough to warrant caching the > statements. > > [Yes, I agree that sql::Connection should just manage the caching internally and > let client code just say "statement, ..."] Done. https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:241: "id INTEGER PRIMARY KEY," On 2015/01/05 21:14:45, Scott Hess wrote: > For these two tables, is there any downside to having the id be re-used? With > this phrasing, SQLite will choose MAX(id)+1 until it runs out of integers. If > the highest-id rows are deleted, then next insert can give an id in that range. > > If it does matter, put in AUTOINCREMENT. AUTOINCREMENT adds a utility table to > track things, so generally don't add it unless it actually matters. > > [AFAICT, the only way it could matter is if a commit wasn't atomic, so I > wouldn't add AUTOINCREMENT.] Unless SQLite does something very strange and transactions are not atomic in practice, the ID being reused should be fine. It is never exposed to outside of this class; and all methods in this class encapsulate operations within transactions. https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:252: // SQLite build does not support the feature. On 2015/01/05 21:14:45, Scott Hess wrote: > Really? It should, I thought foreign-key support was added earlier. Maybe more > has been added since :-). Hmm, strange. I must have messed up something with the PRAGMA before, because that statement always failed for me. But now it works. I have adjusted the code. https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:278: DLOG(FATAL) << sql_connection_->GetErrorMessage(); On 2015/01/05 21:14:45, Scott Hess wrote: > What will ever cause this to resolve itself? > > As a general rule, database errors can be binned into two groups. One group is > the set of errors caused by coding mistakes such as an error in the SQL > statement, which are all fixable, so IMHO crashing in production is reasonable. > The other group is the set of errors which is being injected by the environment, > such as filesystem corruption, which the case above handles. > > The reason I bring this up now is because once you've shipped this, it's > plausible/likely that if you attempt to tighten things up to be more pedantic, > you'll find out that you've had tons of masked errors happening. > > Hmm, to be clear, what I'm saying is that in case of error, you should either > blow away the database or send up a crash dump to signal that there's a bug to > fix. Hmm, I am not sure I understand what you mean by "tighten things up to be more pedantic". My understanding was that unit tests should be extensive enough to catch any SQL programming errors in the debug build tests. Then, in production, we should only encounter errors induced by the environment. We would learn of these through UMA; and locally take care of by razing if the error is catastrophic. I am the most unclear about "not catastrophic" errors, which seems to be everything except _CORRUPT and _NOTADB. Are you saying that we should blow away the DB even in these cases (which sounds reasonable to me as this is just a cache anyway)? Or that we should just notify the user of AffiliationDatabase that the DB is kaputt for now, and we will try again on next startup? https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database_unittest.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database_unittest.cc:233: } On 2015/01/05 21:14:45, Scott Hess wrote: > Suggest you put in some code that exercises the error handler. There is some > helper code in sql/test, and a couple examples of injecting corruption around > the codebase ("git gs CorruptSizeInHeader" finds some). You've got the right > boilerplate in place, AFAICT, but my experience is that people's sql error > handlers rot quickly. > > Hmm, the two cases I'd aim to test are "Does the handler blow away a corrupt > database?" and "How does Init() work when given a corrupt database?" There have > been cases where corruption at open time wasn't handled right so it persisted. Done.
On 2015/01/14 18:06:55, engedy wrote: > @Scott, many thanks for the thorough review. PTAL. > > There is only one outstanding question regarding the handling of non-critical > errors, please see inline. > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > File components/password_manager/core/browser/affiliation_database.cc (right): > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_database.cc:30: > sql_connection_->set_histogram_tag("Affiliation"); > On 2015/01/05 21:14:45, Scott Hess wrote: > > This implies a corresponding change in tools/metrics/histograms/histograms.xml > > to add a new name to suffix SqliteDatabases. Then all the metrics dashboards > > should automatically fill. > > Thanks! Done. > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_database.cc:159: void > AffiliationDatabase::DeleteAllAffiliations() { > On 2015/01/05 21:14:45, Scott Hess wrote: > > This call seems unlikely to happen often enough to warrant caching the > > statements. > > > > [Yes, I agree that sql::Connection should just manage the caching internally > and > > let client code just say "statement, ..."] > > Done. > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_database.cc:241: "id > INTEGER PRIMARY KEY," > On 2015/01/05 21:14:45, Scott Hess wrote: > > For these two tables, is there any downside to having the id be re-used? With > > this phrasing, SQLite will choose MAX(id)+1 until it runs out of integers. If > > the highest-id rows are deleted, then next insert can give an id in that > range. > > > > If it does matter, put in AUTOINCREMENT. AUTOINCREMENT adds a utility table > to > > track things, so generally don't add it unless it actually matters. > > > > [AFAICT, the only way it could matter is if a commit wasn't atomic, so I > > wouldn't add AUTOINCREMENT.] > > Unless SQLite does something very strange and transactions are not atomic in > practice, the ID being reused should be fine. It is never exposed to outside of > this class; and all methods in this class encapsulate operations within > transactions. > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_database.cc:252: // SQLite > build does not support the feature. > On 2015/01/05 21:14:45, Scott Hess wrote: > > Really? It should, I thought foreign-key support was added earlier. Maybe > more > > has been added since :-). > > Hmm, strange. I must have messed up something with the PRAGMA before, because > that statement always failed for me. But now it works. I have adjusted the code. > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_database.cc:278: > DLOG(FATAL) << sql_connection_->GetErrorMessage(); > On 2015/01/05 21:14:45, Scott Hess wrote: > > What will ever cause this to resolve itself? > > > > As a general rule, database errors can be binned into two groups. One group > is > > the set of errors caused by coding mistakes such as an error in the SQL > > statement, which are all fixable, so IMHO crashing in production is > reasonable. > > The other group is the set of errors which is being injected by the > environment, > > such as filesystem corruption, which the case above handles. > > > > The reason I bring this up now is because once you've shipped this, it's > > plausible/likely that if you attempt to tighten things up to be more pedantic, > > you'll find out that you've had tons of masked errors happening. > > > > Hmm, to be clear, what I'm saying is that in case of error, you should either > > blow away the database or send up a crash dump to signal that there's a bug to > > fix. > > Hmm, I am not sure I understand what you mean by "tighten things up to be more > pedantic". > > My understanding was that unit tests should be extensive enough to catch any SQL > programming errors in the debug build tests. Then, in production, we should only > encounter errors induced by the environment. We would learn of these through > UMA; and locally take care of by razing if the error is catastrophic. > > I am the most unclear about "not catastrophic" errors, which seems to be > everything except _CORRUPT and _NOTADB. Are you saying that we should blow away > the DB even in these cases (which sounds reasonable to me as this is just a > cache anyway)? Or that we should just notify the user of AffiliationDatabase > that the DB is kaputt for now, and we will try again on next startup? > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > File components/password_manager/core/browser/affiliation_database_unittest.cc > (right): > > https://codereview.chromium.org/797983003/diff/100001/components/password_man... > components/password_manager/core/browser/affiliation_database_unittest.cc:233: } > On 2015/01/05 21:14:45, Scott Hess wrote: > > Suggest you put in some code that exercises the error handler. There is some > > helper code in sql/test, and a couple examples of injecting corruption around > > the codebase ("git gs CorruptSizeInHeader" finds some). You've got the right > > boilerplate in place, AFAICT, but my experience is that people's sql error > > handlers rot quickly. > > > > Hmm, the two cases I'd aim to test are "Does the handler blow away a corrupt > > database?" and "How does Init() work when given a corrupt database?" There > have > > been cases where corruption at open time wasn't handled right so it persisted. > > Done. Friendly ping.
https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:241: "id INTEGER PRIMARY KEY," On 2015/01/14 18:06:55, engedy wrote: > On 2015/01/05 21:14:45, Scott Hess wrote: > > For these two tables, is there any downside to having the id be re-used? With > > this phrasing, SQLite will choose MAX(id)+1 until it runs out of integers. If > > the highest-id rows are deleted, then next insert can give an id in that > range. > > > > If it does matter, put in AUTOINCREMENT. AUTOINCREMENT adds a utility table > to > > track things, so generally don't add it unless it actually matters. > > > > [AFAICT, the only way it could matter is if a commit wasn't atomic, so I > > wouldn't add AUTOINCREMENT.] > > Unless SQLite does something very strange and transactions are not atomic in > practice, the ID being reused should be fine. It is never exposed to outside of > this class; and all methods in this class encapsulate operations within > transactions. Great, SQLite should do the right thing, just wanted to make sure someone else didn't depend on specific semantics. https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:278: DLOG(FATAL) << sql_connection_->GetErrorMessage(); On 2015/01/14 18:06:55, engedy wrote: > On 2015/01/05 21:14:45, Scott Hess wrote: > > What will ever cause this to resolve itself? > > > > As a general rule, database errors can be binned into two groups. One group > is > > the set of errors caused by coding mistakes such as an error in the SQL > > statement, which are all fixable, so IMHO crashing in production is > reasonable. > > The other group is the set of errors which is being injected by the > environment, > > such as filesystem corruption, which the case above handles. > > > > The reason I bring this up now is because once you've shipped this, it's > > plausible/likely that if you attempt to tighten things up to be more pedantic, > > you'll find out that you've had tons of masked errors happening. > > > > Hmm, to be clear, what I'm saying is that in case of error, you should either > > blow away the database or send up a crash dump to signal that there's a bug to > > fix. > > Hmm, I am not sure I understand what you mean by "tighten things up to be more > pedantic". > > My understanding was that unit tests should be extensive enough to catch any SQL > programming errors in the debug build tests. Then, in production, we should only > encounter errors induced by the environment. We would learn of these through > UMA; and locally take care of by razing if the error is catastrophic. > > I am the most unclear about "not catastrophic" errors, which seems to be > everything except _CORRUPT and _NOTADB. Are you saying that we should blow away > the DB even in these cases (which sounds reasonable to me as this is just a > cache anyway)? Or that we should just notify the user of AffiliationDatabase > that the DB is kaputt for now, and we will try again on next startup? Mostly I'm worrying (perhaps excessively) about whether there's a gap between test code coverage and production code coverage. There have been cases in the past where a module's tests never exercised some code, so in production there were odd results which were hard to track down because the errors were simply being dropped. I'm trying to work out a decent way to handle that case. The most pedantic solution would be to CHECK() on any errors which aren't specifically addressed, then someone will see the crash reports and get on it. Hmm, my confidence level is fading as I worry about whether there are edge cases. I know some people would damn the torpedoes on the theory that if we don't know about the edge cases, they are dangerous. Another option is to monitor the histograms, but that data is less actionable than crash dumps, and usually histograms aren't well-monitored in the long term. https://codereview.chromium.org/797983003/diff/160001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:38: return false; Suggest writing a low-level unit test to verify that this actually works as your code requires. I would be surprised if anyone will think to specifically verify something like this if they're doing an import of a new SQLite version, or mucking with compile-time flags, etc, and this is the only user of foreign keys I can think of in the codebase. Since it's currently a singleton, you could either put the test in this file's unit tests, or up in sql/sqlite_features_unittest.cc. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:133: return; Explicit transaction shouldn't be needed any longer. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:150: return; Explicit transaction shouldn't be needed any longer. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:224: "CREATE TABLE eq_classes(" If you aren't checking that the existing schema matches, then CREATE TABLE IF NOT EXISTS. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:232: "CREATE TABLE eq_class_members(" Also here. https://codereview.chromium.org/797983003/diff/160001/components/password_man... File components/password_manager/core/browser/affiliation_database_unittest.cc (right): https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database_unittest.cc:250: TEST_F(AffiliationDatabaseTest, CorruptDBIsRazedThenOpened) { Thanks! https://codereview.chromium.org/797983003/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/797983003/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62454: + <suffix name="Affiliation" label="Activity"/> Change the label too. I don't remember what it means but everyone else has same name and label.
https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:278: DLOG(FATAL) << sql_connection_->GetErrorMessage(); On 2015/01/20 23:21:38, Scott Hess wrote: > On 2015/01/14 18:06:55, engedy wrote: > > On 2015/01/05 21:14:45, Scott Hess wrote: > > > What will ever cause this to resolve itself? > > > > > > As a general rule, database errors can be binned into two groups. One group > > is > > > the set of errors caused by coding mistakes such as an error in the SQL > > > statement, which are all fixable, so IMHO crashing in production is > > reasonable. > > > The other group is the set of errors which is being injected by the > > environment, > > > such as filesystem corruption, which the case above handles. > > > > > > The reason I bring this up now is because once you've shipped this, it's > > > plausible/likely that if you attempt to tighten things up to be more > pedantic, > > > you'll find out that you've had tons of masked errors happening. > > > > > > Hmm, to be clear, what I'm saying is that in case of error, you should > either > > > blow away the database or send up a crash dump to signal that there's a bug > to > > > fix. > > > > Hmm, I am not sure I understand what you mean by "tighten things up to be more > > pedantic". > > > > My understanding was that unit tests should be extensive enough to catch any > SQL > > programming errors in the debug build tests. Then, in production, we should > only > > encounter errors induced by the environment. We would learn of these through > > UMA; and locally take care of by razing if the error is catastrophic. > > > > I am the most unclear about "not catastrophic" errors, which seems to be > > everything except _CORRUPT and _NOTADB. Are you saying that we should blow > away > > the DB even in these cases (which sounds reasonable to me as this is just a > > cache anyway)? Or that we should just notify the user of AffiliationDatabase > > that the DB is kaputt for now, and we will try again on next startup? > > Mostly I'm worrying (perhaps excessively) about whether there's a gap between > test code coverage and production code coverage. There have been cases in the > past where a module's tests never exercised some code, so in production there > were odd results which were hard to track down because the errors were simply > being dropped. I'm trying to work out a decent way to handle that case. The > most pedantic solution would be to CHECK() on any errors which aren't > specifically addressed, then someone will see the crash reports and get on it. > > Hmm, my confidence level is fading as I worry about whether there are edge > cases. I know some people would damn the torpedoes on the theory that if we > don't know about the edge cases, they are dangerous. Another option is to > monitor the histograms, but that data is less actionable than crash dumps, and > usually histograms aren't well-monitored in the long term. I share your concerns, but I cannot see any obvious ways to achieve this. The problem is that even with the error code in hand, it seems quite difficult to tell apart cases that are caused by programming errors from cases that are environment-induced. For instance, I imagine we would be interested in situations with SQLITE_CONSTRAINT and SQLITE_ERROR that could be indicative of programming errors, but based on UMA data these seem to be ridiculously common. I do not have any better ideas than trying to get good testing coverage and looking at UMA histograms. Then, if we discover something very suspicious, we can add DumpWithoutCrashing() calls. https://codereview.chromium.org/797983003/diff/160001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:38: return false; On 2015/01/20 23:21:38, Scott Hess wrote: > Suggest writing a low-level unit test to verify that this actually works as your > code requires. I would be surprised if anyone will think to specifically verify > something like this if they're doing an import of a new SQLite version, or > mucking with compile-time flags, etc, and this is the only user of foreign keys > I can think of in the codebase. Since it's currently a singleton, you could > either put the test in this file's unit tests, or up in > sql/sqlite_features_unittest.cc. I have added a new SQLiteFeaturesTest, please see: https://codereview.chromium.org/856343002/. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:133: return; On 2015/01/20 23:21:38, Scott Hess wrote: > Explicit transaction shouldn't be needed any longer. Done. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:150: return; On 2015/01/20 23:21:38, Scott Hess wrote: > Explicit transaction shouldn't be needed any longer. Done. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:224: "CREATE TABLE eq_classes(" On 2015/01/20 23:21:38, Scott Hess wrote: > If you aren't checking that the existing schema matches, then CREATE TABLE IF > NOT EXISTS. Done. https://codereview.chromium.org/797983003/diff/160001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:232: "CREATE TABLE eq_class_members(" On 2015/01/20 23:21:38, Scott Hess wrote: > Also here. Done. https://codereview.chromium.org/797983003/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/797983003/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:62454: + <suffix name="Affiliation" label="Activity"/> On 2015/01/20 23:21:38, Scott Hess wrote: > Change the label too. I don't remember what it means but everyone else has same > name and label. Oh, wops, I'm blind... Done.
engedy@chromium.org changed reviewers: + asvitkine@chromium.org
@Alexei: could you PTAL at the one-liner in histograms.xml?
lgtm
lgtm! https://codereview.chromium.org/797983003/diff/100001/components/password_man... File components/password_manager/core/browser/affiliation_database.cc (right): https://codereview.chromium.org/797983003/diff/100001/components/password_man... components/password_manager/core/browser/affiliation_database.cc:278: DLOG(FATAL) << sql_connection_->GetErrorMessage(); On 2015/01/21 15:47:47, engedy wrote: > On 2015/01/20 23:21:38, Scott Hess wrote: > > On 2015/01/14 18:06:55, engedy wrote: > > > On 2015/01/05 21:14:45, Scott Hess wrote: > > > > What will ever cause this to resolve itself? > > > > > > > > As a general rule, database errors can be binned into two groups. One > group > > > is > > > > the set of errors caused by coding mistakes such as an error in the SQL > > > > statement, which are all fixable, so IMHO crashing in production is > > > reasonable. > > > > The other group is the set of errors which is being injected by the > > > environment, > > > > such as filesystem corruption, which the case above handles. > > > > > > > > The reason I bring this up now is because once you've shipped this, it's > > > > plausible/likely that if you attempt to tighten things up to be more > > pedantic, > > > > you'll find out that you've had tons of masked errors happening. > > > > > > > > Hmm, to be clear, what I'm saying is that in case of error, you should > > either > > > > blow away the database or send up a crash dump to signal that there's a > bug > > to > > > > fix. > > > > > > Hmm, I am not sure I understand what you mean by "tighten things up to be > more > > > pedantic". > > > > > > My understanding was that unit tests should be extensive enough to catch any > > SQL > > > programming errors in the debug build tests. Then, in production, we should > > only > > > encounter errors induced by the environment. We would learn of these through > > > UMA; and locally take care of by razing if the error is catastrophic. > > > > > > I am the most unclear about "not catastrophic" errors, which seems to be > > > everything except _CORRUPT and _NOTADB. Are you saying that we should blow > > away > > > the DB even in these cases (which sounds reasonable to me as this is just a > > > cache anyway)? Or that we should just notify the user of AffiliationDatabase > > > that the DB is kaputt for now, and we will try again on next startup? > > > > Mostly I'm worrying (perhaps excessively) about whether there's a gap between > > test code coverage and production code coverage. There have been cases in the > > past where a module's tests never exercised some code, so in production there > > were odd results which were hard to track down because the errors were simply > > being dropped. I'm trying to work out a decent way to handle that case. The > > most pedantic solution would be to CHECK() on any errors which aren't > > specifically addressed, then someone will see the crash reports and get on it. > > > > Hmm, my confidence level is fading as I worry about whether there are edge > > cases. I know some people would damn the torpedoes on the theory that if we > > don't know about the edge cases, they are dangerous. Another option is to > > monitor the histograms, but that data is less actionable than crash dumps, and > > usually histograms aren't well-monitored in the long term. > > I share your concerns, but I cannot see any obvious ways to achieve this. > > The problem is that even with the error code in hand, it seems quite difficult > to tell apart cases that are caused by programming errors from cases that are > environment-induced. For instance, I imagine we would be interested in > situations with SQLITE_CONSTRAINT and SQLITE_ERROR that could be indicative of > programming errors, but based on UMA data these seem to be ridiculously common. > > I do not have any better ideas than trying to get good testing coverage and > looking at UMA histograms. Then, if we discover something very suspicious, we > can add DumpWithoutCrashing() calls. If an uncommon induced error is also not persistent, any CHECK we did would just cause the inevitable crash to come sooner. But I'm scared of uncommon persistent errors. In most cases, that means "Someone wrote incorrect code and should fix it", but what if that's not the case? Unfortunately sql/ is too low in the stack to provide DumpWithoutCrashing() as a service without some sort of injection from above.
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797983003/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/57b2c6c8543cc5b2dd5dd6844da381adf9e50db3 Cr-Commit-Position: refs/heads/master@{#312419} |
