[sql] Scoped recovery framework.

[sql] Scoped recovery framework.

sql::Recovery is intended to be used within a sql::Connection error
callback to either recover the database (*) or indicate that the
database is unrecoverable and should be razed.  The intend is that the
code should either progress to a valid database which is composed of
data recovered from the original (likely corrupt) database, or a valid
database which is empty.

This is just the framework without the SQLite-level data-recovery
virtual table.

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

[Scott Hess - ex-Googler, 2013-07-15 21:10:34]

Apologies if there are big holes in here - I'm finding myself repeatedly
circling and not really making progress, so at this point I'm probably blind to
this code.

The earlier version of this was:
  https://codereview.chromium.org/18180013
That version was derived from a git client where I had integrated the entire
stack, including the SQLite recovery virtual table, and some tests which crafted
corrupt databases and recovered them.  I believe that this version will not
require changes to support that, though I haven't yet rebased that client onto
this, so I might be missing something.  I will do that presently.

https://codereview.chromium.org/19281002/diff/1/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/19281002/diff/1/sql/connection.h#newcode382
sql/connection.h:382: friend class Recovery;
Specifically for sqlite3_backup stuff.  Given that, it might be reasonable to
pull OpenTemporary(), Poison() and RollbackAllTransactions() down into private:
instead of making them publicly available.  In fact, if this all works out, even
Raze() and RazeAndClose() could be made private.

[erikwright (departed), 2013-07-17 00:06:03]

LGTM. Only question is whether it's really the case for all of our databases
that recovering some subset of the data is better than recovering none.

I know you said people could at least turn this feature on or off, but I am
afraid it will just be applied to all DBs without consideration.

[Scott Hess - ex-Googler, 2013-07-17 00:31:32]

On 2013/07/17 00:06:03, erikwright wrote:
> LGTM. Only question is whether it's really the case for all of our databases
> that recovering some subset of the data is better than recovering none.
> 
> I know you said people could at least turn this feature on or off, but I am
> afraid it will just be applied to all DBs without consideration.

Each database client is going to have to provide an error callback to re-create
the schema, read what it can of the existing data (coming), and (hopefully)
verify validity.  There's zero likelihood that someone will just flip the
"recover me automagically" flag and end up with a bunch of junk because their
data is too subtle.  In fact, that's an explicit non-goal for me, if a
database's schema is simple enough that an automated system could recover it,
then just write the few lines of SQL code and be done with it.

[Scott Hess - ex-Googler, 2013-07-17 00:35:16]

On 2013/07/17 00:31:32, shess wrote:
> On 2013/07/17 00:06:03, erikwright wrote:
> > LGTM. Only question is whether it's really the case for all of our databases
> > that recovering some subset of the data is better than recovering none.
> > 
> > I know you said people could at least turn this feature on or off, but I am
> > afraid it will just be applied to all DBs without consideration.
> 
> Each database client is going to have to provide an error callback to
re-create
> the schema, read what it can of the existing data (coming), and (hopefully)
> verify validity.  There's zero likelihood that someone will just flip the
> "recover me automagically" flag and end up with a bunch of junk because their
> data is too subtle.  In fact, that's an explicit non-goal for me, if a
> database's schema is simple enough that an automated system could recover it,
> then just write the few lines of SQL code and be done with it.

It is also entirely possible that this has all been a waste of time :-).

Something to keep in mind is that before embarking on recovery, clients are
freely using possibly-broken data without any validation.  So while it is
possible that this code could be used to fix things at the SQLite level but
leave them broken at higher levels, I don't believe that would be adding a new
form of brokenness to the system which previously didn't exist.

[erikwright (departed), 2013-07-17 01:41:11]

Sgtm. Thanks for the explanation.
On Jul 16, 2013 5:35 PM, <shess@chromium.org> wrote:

> On 2013/07/17 00:31:32, shess wrote:
>
>> On 2013/07/17 00:06:03, erikwright wrote:
>> > LGTM. Only question is whether it's really the case for all of our
>> databases
>> > that recovering some subset of the data is better than recovering none.
>> >
>> > I know you said people could at least turn this feature on or off, but
>> I am
>> > afraid it will just be applied to all DBs without consideration.
>>
>
>  Each database client is going to have to provide an error callback to
>>
> re-create
>
>> the schema, read what it can of the existing data (coming), and
>> (hopefully)
>> verify validity.  There's zero likelihood that someone will just flip the
>> "recover me automagically" flag and end up with a bunch of junk because
>> their
>> data is too subtle.  In fact, that's an explicit non-goal for me, if a
>> database's schema is simple enough that an automated system could recover
>> it,
>> then just write the few lines of SQL code and be done with it.
>>
>
> It is also entirely possible that this has all been a waste of time :-).
>
> Something to keep in mind is that before embarking on recovery, clients are
> freely using possibly-broken data without any validation.  So while it is
> possible that this code could be used to fix things at the SQLite level but
> leave them broken at higher levels, I don't believe that would be adding a
> new
> form of brokenness to the system which previously didn't exist.
>
>
https://codereview.chromium.**org/19281002/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-07-17 16:38:34]

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

[commit-bot: I haz the power, 2013-07-17 18:05:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Scott Hess - ex-Googler, 2013-07-17 19:21:14]

On 2013/07/17 18:05:14, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on android_dbg.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

Merged with Open() retry logic on other development thread, coming around for
another pass.

[commit-bot: I haz the power, 2013-07-17 19:37:24]

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

[commit-bot: I haz the power, 2013-07-18 00:52:21]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Scott Hess - ex-Googler, 2013-07-19 16:39:32]

On 2013/07/18 00:52:21, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_chromeos for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Dunno, those failures don't look related.  Pushing it again.

[commit-bot: I haz the power, 2013-07-19 16:39:42]

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

[commit-bot: I haz the power, 2013-07-19 18:25:31]

Change committed as 212607