[sql] Additional Raze() unit tests.

[sql] Additional Raze() unit tests.

Verify that Raze() can handle:
 - empty databases.
 - non-database files.
 - databases overwritten with garbage.
 - databases which cannot be opened successfully.

BUG=none

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

[Scott Hess - ex-Googler, 2013-06-25 21:00:14]

Recent refactoring allows more thorough testing of these things, and
more-thorough testing exposes additional failure cases.  As with the other CL,
might be prudent to ACK rather than duplicating effort.  Thanks.

[Greg Billock, 2013-06-27 21:01:47]

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

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode69
sql/connection.cc:69: int BackupDatabase(sqlite3* src, sqlite3* dst, const char*
db_name) {
Docs say src and dst must be different. Want to DCHECK that?

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode74
sql/connection.cc:74: DLOG(FATAL) << "Unable to start sqlite3_backup().";
Print out dst->errmsg/errcode as well here?

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode80
sql/connection.cc:80: int pages = sqlite3_backup_pagecount(backup);
Useful if rc != SQLITE_DONE? Also, need to deal with SQLITE_BUSY/SQLITE_LOCKED?

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode349
sql/connection.cc:349: // SQLITE_NOTADB can happen if page 1 exists but is not
formatted
So this is page 1 of the destination?

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode352
sql/connection.cc:352: // truncate it before trying again.
Above it sounds like PRAGMA writeable_schema will achieve this. Is that not so?
That seems like a not-quite-so-low-level approach if it worked...

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode355
sql/connection.cc:355: if (rc == SQLITE_NOTADB || rc == SQLITE_IOERR_SHORT_READ)
{
Should this stanza go in the backup method?

[Scott Hess - ex-Googler, 2013-06-27 22:41:47]

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

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode69
sql/connection.cc:69: int BackupDatabase(sqlite3* src, sqlite3* dst, const char*
db_name) {
On 2013/06/27 21:01:47, Greg Billock wrote:
> Docs say src and dst must be different. Want to DCHECK that?

Done.

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode74
sql/connection.cc:74: DLOG(FATAL) << "Unable to start sqlite3_backup().";
On 2013/06/27 21:01:47, Greg Billock wrote:
> Print out dst->errmsg/errcode as well here?

OK.  Not entirely convinced it will be useful due to D, but also not convinced
anyone will ever see things if it was LOG(ERROR).  Also converted return code to
appropriate errorcode.

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode80
sql/connection.cc:80: int pages = sqlite3_backup_pagecount(backup);
On 2013/06/27 21:01:47, Greg Billock wrote:
> Useful if rc != SQLITE_DONE? Also, need to deal with
SQLITE_BUSY/SQLITE_LOCKED?

Honestly, I am not sure what to do in those cases.  Sleep and retry?  I'll add
some histograms to see which areas merit attention.

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode349
sql/connection.cc:349: // SQLITE_NOTADB can happen if page 1 exists but is not
formatted
On 2013/06/27 21:01:47, Greg Billock wrote:
> So this is page 1 of the destination?

Comment amended.

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode352
sql/connection.cc:352: // truncate it before trying again.
On 2013/06/27 21:01:47, Greg Billock wrote:
> Above it sounds like PRAGMA writeable_schema will achieve this. Is that not
so?
> That seems like a not-quite-so-low-level approach if it worked...

That case covers when the database in the filesystem is N*page_size, but the
header says that it should be N+1 or N-1 or something other than N pages. 
SQLITE_IOERR_SHORT_READ happens if the database is less than page_size.

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode355
sql/connection.cc:355: if (rc == SQLITE_NOTADB || rc == SQLITE_IOERR_SHORT_READ)
{
On 2013/06/27 21:01:47, Greg Billock wrote:
> Should this stanza go in the backup method?

That's what the TODO is about.  First it tries to do the backup straight up.  If
that works, grand, if it fails for a few specific reasons, it adjusts the file
and tries again.  Putting the error-handler in BackupDatabase() would duplicate
that code (once for first try, once for retry).

OTOH, if it _started_ with the below code to truncate the database in all cases,
the there wouldn't need to be duplication.  In fact, it could maybe just
truncate and _not_ do the backup trick, because AFAICT SQLite treats the
database as empty if it's zero-length.  AFAICT.

Right now, this code only does the truncate if SQLITE_NOTADB and
SQLITE_IOERR_SHORT_READ, specifically resulting from the cases tested, so I'm
pretty sure about them.  AFAICT, in both of those cases it would be impossible
for anyone else to have legitimately opened the database, so there is no scope
for confusion.

For other errors, I'm less certain whether truncating is the right thing to do. 
Certainly if the first backup attempt got SQLITE_BUSY, truncation would likely
cause interesting results.  I'm not sure what to do about it, though.

For background, the reason to do things in this subtle way rather than deleting
the file is because if anyone has the file open on Windows, delete won't work
but this can.  This can include things like anti-virus programs.  I considered
having this be the backup plan in case delete fails, but I thought it prudent to
make the subtle case the common case, so that it gets more thoroughly tested. 
I'm thinking of adding delete as a backup to this, but I'm not certain in which
cases that makes sense.

[Greg Billock, 2013-06-28 14:29:08]

lgtm

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

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode352
sql/connection.cc:352: // truncate it before trying again.
So we see both these on corruption?

On 2013/06/27 22:41:47, shess wrote:
> On 2013/06/27 21:01:47, Greg Billock wrote:
> > Above it sounds like PRAGMA writeable_schema will achieve this. Is that not
> so?
> > That seems like a not-quite-so-low-level approach if it worked...
> 
> That case covers when the database in the filesystem is N*page_size, but the
> header says that it should be N+1 or N-1 or something other than N pages. 
> SQLITE_IOERR_SHORT_READ happens if the database is less than page_size.

https://codereview.chromium.org/17752002/diff/1/sql/connection.cc#newcode355
sql/connection.cc:355: if (rc == SQLITE_NOTADB || rc == SQLITE_IOERR_SHORT_READ)
{
I see. The histogram ought to provide good guidance here.

On 2013/06/27 22:41:47, shess wrote:
> On 2013/06/27 21:01:47, Greg Billock wrote:
> > Should this stanza go in the backup method?
> 
> That's what the TODO is about.  First it tries to do the backup straight up. 
If
> that works, grand, if it fails for a few specific reasons, it adjusts the file
> and tries again.  Putting the error-handler in BackupDatabase() would
duplicate
> that code (once for first try, once for retry).
> 
> OTOH, if it _started_ with the below code to truncate the database in all
cases,
> the there wouldn't need to be duplication.  In fact, it could maybe just
> truncate and _not_ do the backup trick, because AFAICT SQLite treats the
> database as empty if it's zero-length.  AFAICT.
> 
> Right now, this code only does the truncate if SQLITE_NOTADB and
> SQLITE_IOERR_SHORT_READ, specifically resulting from the cases tested, so I'm
> pretty sure about them.  AFAICT, in both of those cases it would be impossible
> for anyone else to have legitimately opened the database, so there is no scope
> for confusion.
> 
> For other errors, I'm less certain whether truncating is the right thing to
do. 
> Certainly if the first backup attempt got SQLITE_BUSY, truncation would likely
> cause interesting results.  I'm not sure what to do about it, though.
> 
> For background, the reason to do things in this subtle way rather than
deleting
> the file is because if anyone has the file open on Windows, delete won't work
> but this can.  This can include things like anti-virus programs.  I considered
> having this be the backup plan in case delete fails, but I thought it prudent
to
> make the subtle case the common case, so that it gets more thoroughly tested. 
> I'm thinking of adding delete as a backup to this, but I'm not certain in
which
> cases that makes sense.

[Scott Hess - ex-Googler, 2013-06-28 22:51:14]

Sheriff-type people: I am headed OOT tomorrow, so if the c-q is super slow or
something and this takes a long time to clear, I won't be responsive.  Feel free
to revert in that case.

[commit-bot: I haz the power, 2013-06-28 22:51:50]

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

[commit-bot: I haz the power, 2013-06-29 01:22:12]

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

[Scott Hess - ex-Googler, 2013-06-29 01:29:04]

Oops, need owner for histograms.xml.

[jar (doing other things), 2013-06-29 02:15:11]

lgtm

[commit-bot: I haz the power, 2013-06-29 02:16:15]

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

[commit-bot: I haz the power, 2013-06-29 02:51:30]

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

[Scott Hess - ex-Googler, 2013-06-29 03:22:54]

On 2013/06/29 02:51:30, I haz the power (commit-bot) wrote:
> Retried try job too often on ios_dbg_simulator for step(s) sql_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

Yeah, not going to fix that before leaving.  Oh well!

[Scott Hess - ex-Googler, 2013-07-09 20:33:54]

Truncating the file by 1024 bytes did not cause iOS to notice that the file was
too short.  I see that the page_size is 4096, but on OSX the page_size doesn't
matter because the pagerPagecount() function in SQLite calculates nPage as
fileSize/pageSize, so truncating by any amount works.  On iOS we
USE_SYSTEM_SQLITE, so I can't really say why it didn't work there.

I'm not sure how productively I can comment this kind of ambiguous finding in
the test itself, and truncating by page_size seems modestly more sensible than
using a constant, so I'm just going with that.

[commit-bot: I haz the power, 2013-07-09 20:34:38]

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

[commit-bot: I haz the power, 2013-07-09 20:42:50]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-07-10 19:03:34]

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

[commit-bot: I haz the power, 2013-07-10 20:46:17]

Change committed as 210923