[sql] Use recover virtual table in sql::Recovery.

[sql] Use recover virtual table in sql::Recovery.

Load the recover virtual table as part of sql::Recovery::Begin(), and
implement basic unit tests that it correctly recovers valid and
corrupt tables.

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

[Scott Hess - ex-Googler, 2013-07-23 22:32:04]

https://codereview.chromium.org/20022006/diff/1/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/20022006/diff/1/sql/connection_unittest.cc#ne...
sql/connection_unittest.cc:428: file_util::ScopedFILE
file(file_util::OpenFile(db_path(), "rb+"));
I had a case in SQLRecoveryTest where reading a block and re-writing it didn't
work, because the first byte of a table page is 0x0D (CR).  Seemed reasonable to
make sure all of this code was using binary.

https://codereview.chromium.org/20022006/diff/1/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/20022006/diff/1/sql/recovery_unittest.cc#newc...
sql/recovery_unittest.cc:143: ExecuteWithResults(&db(), kXSql, "|", "\n"));
Noticed that the expectation was in the wrong place when auditing expectations
in the following code, so changed it.  Otherwise not really related.

[Scott Hess - ex-Googler, 2013-07-26 04:51:43]

ping?

https://chromiumcodereview.appspot.com/20022006/diff/6001/sql/sql.gyp
File sql/sql.gyp (right):

https://chromiumcodereview.appspot.com/20022006/diff/6001/sql/sql.gyp#newcode82
sql/sql.gyp:82: '../third_party/sqlite/sqlite.gyp:sqlite',
So that the test code can see USE_SYSTEM_SQLITE.

[Scott Hess - ex-Googler, 2013-07-26 14:32:52]

Ah, and the "documentation" on using the recover virtual table is in
third_party/sqlite/src/src/recover.c header comment.

[erikwright (departed), 2013-07-26 16:05:33]

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc#n...
sql/recovery_unittest.cc:238: db->reset_error_callback();
Is this something most clients would need to do? With the penalty for negligence
being infinite recursion when recovery fails?

Presumably a real error handler would also have to restore itself at the end?

If so could we find some way to deal with that? Maybe it makes sense to always
disable the error handler during recovery.

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc#n...
sql/recovery_unittest.cc:379: // TODO(shess): Figure out a query which causes
SQLite to notice
IIUC, this type of error (an extra value in the table) is something that SQLite
will continue to gloss over while returning inconsistent results, and thus the
client would be responsible for detecting this state and initiating recovery?

[Scott Hess - ex-Googler, 2013-07-26 17:15:58]

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc#n...
sql/recovery_unittest.cc:238: db->reset_error_callback();
On 2013/07/26 16:05:33, erikwright wrote:
> Is this something most clients would need to do? With the penalty for
negligence
> being infinite recursion when recovery fails?

Possibilities are probably endless.  It could recurse, or it could fail due to
conflict with the outer handler's setup, or it could fail and mess up the outer
handler while doing so.

> Presumably a real error handler would also have to restore itself at the end?
> 
> If so could we find some way to deal with that? Maybe it makes sense to always
> disable the error handler during recovery.

Still working through this.  In the context of sql::Recovery, once we're done
the handle will be closed.  Which I thought was the end of it, until ... now
that you asked, it occurs to me that the error handler could be considered an
attribute of the handle, not of the connection, so you could re-open things and
now be in a weird state.  Re-open of a previously-closed handle probably isn't
intended, but it works.

Right now my goal is to aim for something unlikely to provide unexpected results
when faced with real-world crazy.  sql::Recovery asserts that the connection
being recovered has no error callback, because needing to recover a database
probably implies that that database will throw errors.  But reviewing the calls
agains the original connection, I'm not sure any of them would ever throw errors
in practice.  Having the sql::Recovery reset/restore the error callback might
make sense, but could cause odd behavior if the calling code also resets the
callback.  I'm not enthusiastic about adding additional wiring across
sql::Recovery and sql::Connection until I'm sure it's actually helpful.

I expect things will become more clear as I grind through writing code for the
database users to recover things.  If you feel strongly that we should define
this earlier, let me know.  I've been intending to write the thumbnails_database
recovery handler today for about a week, now, so it should happen soon :-).

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc#n...
sql/recovery_unittest.cc:379: // TODO(shess): Figure out a query which causes
SQLite to notice
On 2013/07/26 16:05:33, erikwright wrote:
> IIUC, this type of error (an extra value in the table) is something that
SQLite
> will continue to gloss over while returning inconsistent results, and thus the
> client would be responsible for detecting this state and initiating recovery?

It's not glossing it over, it never even sees the problem, because it only looks
at the data necessary to satisfy the query.  Doing a query through the table
sees the table data without the constraints, doing a query through the index
works fine because what the index says is there is there.  The only way to
detect such things is to do a full scan of the database ("pragma
integrity_check" will do).  It might make sense to do that check automatically
every once and awhile.

This TODO isn't about making SQLite notice this case, it's about having a
corruption which SQLite will notice when querying the table.  The index test is
kind of a softball, since the recovery callback ignores indexes entirely.  I
want to have at least one test which demonstrates that invalid constraints can
be handled in a way which recovers some data.  I'm also hoping that it can be
simple, like restoring an old version of a page, rather than a complicated
sequence of mumbo-jumbo which is brittle in the face of minor SQLite changes.

[erikwright (departed), 2013-07-26 17:56:57]

LGTM.

https://codereview.chromium.org/20022006/diff/6001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/20022006/diff/6001/sql/recovery.cc#newcode85
sql/recovery.cc:85: int rc = recoverVtableInit(recover_db_.db_);
Any compelling reason for sqlite to not always initialize this module for every
database?

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/20022006/diff/6001/sql/recovery_unittest.cc#n...
sql/recovery_unittest.cc:238: db->reset_error_callback();
On 2013/07/26 17:15:58, shess wrote:
> On 2013/07/26 16:05:33, erikwright wrote:
> > Is this something most clients would need to do? With the penalty for
> negligence
> > being infinite recursion when recovery fails?
> 
> Possibilities are probably endless.  It could recurse, or it could fail due to
> conflict with the outer handler's setup, or it could fail and mess up the
outer
> handler while doing so.
> 
> > Presumably a real error handler would also have to restore itself at the
end?
> > 
> > If so could we find some way to deal with that? Maybe it makes sense to
always
> > disable the error handler during recovery.
> 
> Still working through this.  In the context of sql::Recovery, once we're done
> the handle will be closed.  Which I thought was the end of it, until ... now
> that you asked, it occurs to me that the error handler could be considered an
> attribute of the handle, not of the connection, so you could re-open things
and
> now be in a weird state.  Re-open of a previously-closed handle probably isn't
> intended, but it works.
> 
> Right now my goal is to aim for something unlikely to provide unexpected
results
> when faced with real-world crazy.  sql::Recovery asserts that the connection
> being recovered has no error callback, because needing to recover a database
> probably implies that that database will throw errors.  But reviewing the
calls
> agains the original connection, I'm not sure any of them would ever throw
errors
> in practice.  Having the sql::Recovery reset/restore the error callback might
> make sense, but could cause odd behavior if the calling code also resets the
> callback.  I'm not enthusiastic about adding additional wiring across
> sql::Recovery and sql::Connection until I'm sure it's actually helpful.
> 
> I expect things will become more clear as I grind through writing code for the
> database users to recover things.  If you feel strongly that we should define
> this earlier, let me know.  I've been intending to write the
thumbnails_database
> recovery handler today for about a week, now, so it should happen soon :-).

That's fine for now.

[Scott Hess - ex-Googler, 2013-07-26 18:04:08]

Thanks!

I'm OOT next week, so I won't be able to get anything into production until a
few weeks after.  But I'm still all excited :-).

https://codereview.chromium.org/20022006/diff/6001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/20022006/diff/6001/sql/recovery.cc#newcode85
sql/recovery.cc:85: int rc = recoverVtableInit(recover_db_.db_);
On 2013/07/26 17:56:57, erikwright wrote:
> Any compelling reason for sqlite to not always initialize this module for
every
> database?

Having SQLite initialize it would make it available to web content.  In the
abstract that's not a problem, but for now it felt like a line I didn't want to
cross.

[erikwright (departed), 2013-07-26 18:11:58]

Interesting. WebDB is a direct line into SQLite?

I'd be shocked if that's the only thing in SQLite that's inappropriate to
expose to web content.


On Fri, Jul 26, 2013 at 2:04 PM, <shess@chromium.org> wrote:

> Thanks!
>
> I'm OOT next week, so I won't be able to get anything into production
> until a
> few weeks after.  But I'm still all excited :-).
>
>
>
>
https://codereview.chromium.**org/20022006/diff/6001/sql/**recovery.cc<https:...
> File sql/recovery.cc (right):
>
> https://codereview.chromium.**org/20022006/diff/6001/sql/**
>
recovery.cc#newcode85<https://codereview.chromium.org/20022006/diff/6001/sql/recovery.cc#newcode85>
> sql/recovery.cc:85: int rc = recoverVtableInit(recover_db_.**db_);
> On 2013/07/26 17:56:57, erikwright wrote:
>
>> Any compelling reason for sqlite to not always initialize this module
>>
> for every
>
>> database?
>>
>
> Having SQLite initialize it would make it available to web content.  In
> the abstract that's not a problem, but for now it felt like a line I
> didn't want to cross.
>
>
https://codereview.chromium.**org/20022006/<https://codereview.chromium.org/2...
>

[commit-bot: I haz the power, 2013-07-26 18:37:01]

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

[Scott Hess - ex-Googler, 2013-07-26 19:27:28]

On 2013/07/26 18:11:58, erikwright wrote:
> Interesting. WebDB is a direct line into SQLite?

If it were enabled in SQLite (say in main.c), yes.  If it were enabled in
sql::Connection::Open(), then it would only be available in the browser.  I
think.

> I'd be shocked if that's the only thing in SQLite that's inappropriate to
> expose to web content.

:-).  Just saying "Like SQLite" allowed WebDB to get away without having to spec
out (or implement) a ton of stuff, but it does mean we have a huge implicit API
which nobody really committed to maintaining.  I think that's part of why WebDB
didn't get traction with other browser vendors.  Mostly I try to make sure we
don't add to it.

[commit-bot: I haz the power, 2013-07-27 23:49:51]

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

[commit-bot: I haz the power, 2013-07-28 01:04:21]

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

[commit-bot: I haz the power, 2013-07-28 03:04:15]

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

[commit-bot: I haz the power, 2013-08-05 20:42:51]

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

[commit-bot: I haz the power, 2013-08-05 21:41:10]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-05 21:46:35]

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

[commit-bot: I haz the power, 2013-08-06 02:37:42]

Change committed as 215764