Scoped recovery module for sql/

[Closed because landed in:
  https://chromiumcodereview.appspot.com/19281002
]

Scoped recovery module for sql/

BUG=

[Scott Hess - ex-Googler, 2013-06-29 00:53:08]

Erik,

This is something I've been working up, but I'm going on vacation starting
tomorrow, so I'm posting it not with the intent to draw a real review, but to
draw a review-of-concept sort of review.  This is just the core API - I've got
other work which combines it with third_party/sqlite/src/src/recover-alt.c to
create something which can recover a corrupted database.  And tests to corrupt a
database, etc, so it's a deep hole.  Anyhow, I've pulled the core sense of
things out in hopes of getting it in the pipeline before I'm gone, so that when
I get back I can have some momentum prepared.

Thanks,
scott [back on 7/8.]

[erikwright (departed), 2013-07-05 19:02:30]

The main question I have is about how reliable and useful this recovery
mechanism is. Is it universally applicable or is it possibly bad depending on
the schema in question?

https://codereview.chromium.org/18180013/diff/6001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/18180013/diff/6001/sql/connection.cc#newcode384
sql/connection.cc:384: DLOG_IF(FATAL, !poisoned_) << "Cannot poison null db";
is this different from DCHECK(poisoned) << "..";?

https://codereview.chromium.org/18180013/diff/6001/sql/recovery.h
File sql/recovery.h (right):

https://codereview.chromium.org/18180013/diff/6001/sql/recovery.h#newcode41
sql/recovery.h:41: class SQL_EXPORT Recovery {
An idiom I've been using from time to time is like this:

class Recovery {
 public:
  static scoped_ptr<Recovery> Begin(db, path);
  sql::Connection* db() { return db_; }
  static bool Recovered(scoped_ptr<Recovery>);
  static void Unrecoverable(scoped_ptr<Recovery>);
 private:
  Recovery(...);
};

In plain English, you can only construct one on the heap, and the
Recoverable/Unrecoverable transitions explicitly require you to give up your
instance (which makes sense, since the instance methods are no longer valid at
that point).

[Scott Hess - ex-Googler, 2013-07-08 21:41:51]

On 2013/07/05 19:02:30, erikwright wrote:
> The main question I have is about how reliable and useful this recovery
> mechanism is. Is it universally applicable or is it possibly bad depending on
> the schema in question?

Anecdotally, most database corruption seems to be in the relationships between
pages rather than the data within a page.  Most of our databases have pretty
simple schema.  So, in most cases creating the new schema and pulling out the
readable data from the table itself is sufficient to "recover" the database. 
Here "recover" means something like "The database previously threw
SQLITE_CORRUPT errors, now it doesn't, and hopefully some of the previous data
is still present."

WRT reliability, my goal is "Does this reliably result in a not-corrupt
database?", and I think it should be able to do that, either by successfully
recovering data or by razing the database.

Really, the goal is to be a best-effort improvement over deleting all of the
data.  I assume that there will be some databases which cannot productively be
recovered in an automated fashion, but something like this provides a knob for
tuning where that cut-off point is.  For instance, the recovery module has some
support for handling even sqlite_master corruption, but I suspect that doing so
will not be worthwhile in practice.  Likewise, handling corruption across
multiple versions of a database may not be worthwhile.

https://codereview.chromium.org/18180013/diff/6001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/18180013/diff/6001/sql/connection.cc#newcode384
sql/connection.cc:384: DLOG_IF(FATAL, !poisoned_) << "Cannot poison null db";
On 2013/07/05 19:02:30, erikwright wrote:
> is this different from DCHECK(poisoned) << "..";?

Once Upon A Time, in a refactor we had a big knock-down drag-out argument about
this.  I do not remember why things ended up with DLOG(FATAL) in this code, but
we did.  I don't think there was a functional difference involved, I think it
hinged on some abstract sense of rightness.

[I also don't think I agreed with using DLOG(FATAL), at some point I just
stopped caring.]

https://codereview.chromium.org/18180013/diff/6001/sql/recovery.h
File sql/recovery.h (right):

https://codereview.chromium.org/18180013/diff/6001/sql/recovery.h#newcode41
sql/recovery.h:41: class SQL_EXPORT Recovery {
On 2013/07/05 19:02:30, erikwright wrote:
> An idiom I've been using from time to time is like this:
> 
> class Recovery {
>  public:
>   static scoped_ptr<Recovery> Begin(db, path);
>   sql::Connection* db() { return db_; }
>   static bool Recovered(scoped_ptr<Recovery>);
>   static void Unrecoverable(scoped_ptr<Recovery>);
>  private:
>   Recovery(...);
> };
> 
> In plain English, you can only construct one on the heap, and the
> Recoverable/Unrecoverable transitions explicitly require you to give up your
> instance (which makes sense, since the instance methods are no longer valid at
> that point).

I'll prototype that and see how it looks.  My main goal is to make it easier to
get right than wrong, but realistically code like this can't really enforce
strong assertions - you can call the API in the right order but still do
everything completely wrong in between and end up with garbage results.