New issue
Advanced search Search tips

Issue 597785 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocking:
issue 109482



Sign in to add a comment

Build a more-automated recovery tool for sql::Recovery.

Project Member Reported by sh...@chromium.org, Mar 24 2016

Issue description

sql::Recovery was originally implemented to be conservative.  For instance, given a know-corrupt database, the schema is not implicitly trusted.  The idea was that if corrupt schema were common, the recover virtual table could be used over sqlite_master to recover as much as possible.

In practice, this hasn't been an issue.  Furthermore, at this point I'm finding it hard to imagine a form of schema corruption which would leave recoverable data.  The common forms of corruption involve valid btree pages with invalid relationships between pages.  Since the schema is a single btree, in most cases it should have a valid schema which may not match the actual data.

So it is probably reasonable to implement an automated helper to recover the entire database.  Looking at UMA stats, Shortcuts has a high proportion of SQLITE_CORRUPT, and also a fair amount of SQLITE_NOTADB and SQLITE_CANTOPEN, and it's a relatively simple database, so I think it's a reasonable first target for this.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97681440d6f680269022d9087fb524b8b75008a4

commit 97681440d6f680269022d9087fb524b8b75008a4
Author: shess <shess@chromium.org>
Date: Tue Jun 21 06:56:25 2016

[sql] sql::ScopedErrorIgnorer rename to sql::test::ScopedErrorExpecter

"Ignore" was a result of focussing on avoiding the DCHECK in the
sql::Connection implementation, while "expect" focusses on what the test
is trying to do.  Changing because "ignore" is ambiguous.

BUG=597785
TBR=sdefresne@chromium.org, jam@chromium.org

Review-Url: https://codereview.chromium.org/1991503002
Cr-Commit-Position: refs/heads/master@{#400910}

[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/components/history/core/browser/thumbnail_database.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/components/history/core/browser/thumbnail_database_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/components/history/core/browser/top_sites_database.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/components/history/core/browser/top_sites_database_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/components/password_manager/core/browser/affiliation_database.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/components/password_manager/core/browser/affiliation_database_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/content/browser/appcache/appcache_database.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/content/browser/appcache/appcache_database_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/content/browser/databases_table_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/content/browser/dom_storage/dom_storage_database_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/content/browser/quota/quota_database_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/BUILD.gn
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/connection.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/connection.h
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/connection_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/recovery_unittest.cc
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/sql.gyp
[modify] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/statement_unittest.cc
[add] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/test/scoped_error_expecter.cc
[add] https://crrev.com/97681440d6f680269022d9087fb524b8b75008a4/sql/test/scoped_error_expecter.h
[delete] https://crrev.com/8ca173a988171b43b6686335335f816a0ff27403/sql/test/scoped_error_ignorer.cc
[delete] https://crrev.com/8ca173a988171b43b6686335335f816a0ff27403/sql/test/scoped_error_ignorer.h

Comment 2 by sh...@chromium.org, Jun 30 2016

Blocking: 109482
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a402e75a80c13fab013100447b31a00ff6b3a0f5

commit a402e75a80c13fab013100447b31a00ff6b3a0f5
Author: shess <shess@chromium.org>
Date: Sat Jul 02 00:25:11 2016

[sql] Database recovery system for Shortcuts.

sql::Recovery provides a conservative recovery system which requires
some overhead in the client code.  Such code exists for the "Top Sites"
and Favicons databases, and has shown that the easy-to-handle cases
dominate.  Implement a helper function to recover an entire database,
and apply it to the Shortcuts database.

BUG=597785

Review-Url: https://codereview.chromium.org/1832173002
Cr-Commit-Position: refs/heads/master@{#403576}

[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/components/omnibox/browser/shortcuts_database.cc
[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/components/omnibox/browser/shortcuts_database_unittest.cc
[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/sql/connection.cc
[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/sql/recovery.cc
[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/sql/recovery.h
[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/sql/recovery_unittest.cc
[modify] https://crrev.com/a402e75a80c13fab013100447b31a00ff6b3a0f5/tools/metrics/histograms/histograms.xml

Comment 4 by sh...@chromium.org, Jul 2 2016

I'm going out of town for a week, so FYI a CL to disable this, if needed:
   https://codereview.chromium.org/2114823003/
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/631881144b032483cd60b26e054539fd819b6713

commit 631881144b032483cd60b26e054539fd819b6713
Author: shess <shess@chromium.org>
Date: Sat Aug 27 10:26:23 2016

[sql] Track SQLite error codes from sql::Recovery ATTACH.

The major reason for sql::Recover::Begin() to fail is in the statement
attaching the original database to the recovery database.  I believe
this is because of SQLITE_NOTADB.  This is to verify that hypothesis.
If the hypothesis is true, then the best course of action is to Raze()
the database, as SQLITE_NOTADB means that some or all of the SQLite
header data appears to be garbage.

BUG=597785
R=michaeln@chromium.org

Review-Url: https://codereview.chromium.org/2274883003
Cr-Commit-Position: refs/heads/master@{#414885}

[modify] https://crrev.com/631881144b032483cd60b26e054539fd819b6713/sql/recovery.cc
[modify] https://crrev.com/631881144b032483cd60b26e054539fd819b6713/sql/recovery_unittest.cc
[modify] https://crrev.com/631881144b032483cd60b26e054539fd819b6713/tools/metrics/histograms/histograms.xml

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00d65d49ff8e35b2658d0798fac66b1da040836a

commit 00d65d49ff8e35b2658d0798fac66b1da040836a
Author: shess <shess@chromium.org>
Date: Thu Mar 02 21:12:19 2017

[sql] RecoverDatabase() deletes SQLITE_NOTADB databases.

SQLITE_NOTADB happens when the SQLite header data is broken.  That data
includes meta-information about the file's structure, so any corruption
in the header would require manual intervention to recover - assuming
that the entire file is not broken.  SQLITE_NOTADB files are not even
partially readable through SQLite, so deletion unsticks the file going
forward.

BUG=597785

Review-Url: https://codereview.chromium.org/2710823005
Cr-Commit-Position: refs/heads/master@{#454374}

[modify] https://crrev.com/00d65d49ff8e35b2658d0798fac66b1da040836a/sql/recovery.cc
[modify] https://crrev.com/00d65d49ff8e35b2658d0798fac66b1da040836a/sql/recovery.h
[modify] https://crrev.com/00d65d49ff8e35b2658d0798fac66b1da040836a/sql/recovery_unittest.cc
[modify] https://crrev.com/00d65d49ff8e35b2658d0798fac66b1da040836a/tools/metrics/histograms/histograms.xml

Comment 7 by sh...@chromium.org, Jun 21 2017

Labels: ShessReview

Comment 8 by jrobb...@google.com, Sep 21 2017

Owner: pwnall@chromium.org

Sign in to add a comment