[sql] Validate database files before enabling memory-mapping.

[sql] Validate database files before enabling memory-mapping.

With regular I/O, filesystem corruption would cause SQLite to return
SQLITE_IOERR codes.  With memory-mapping, filesystem errors can cause a
crash when accessed.  There are databases with filesystem corruption,
this CL only enables memory-mapped I/O for parts of the database which
have been successfully read at some point using regular I/O.

BUG=537742
Committed: https://crrev.com/de8784cf209f5c254f6c65718373985eb662b36f
Cr-Commit-Position: refs/heads/master@{#358672}

[Scott Hess - ex-Googler, 2015-11-04 23:20:09]

Uninit variable warning.

[Scott Hess - ex-Googler, 2015-11-04 23:39:56]

shess@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Scott Hess - ex-Googler, 2015-11-04 23:39:56]

I was trying to get the VFS option to this working sanely, but I kept finding
bits and pieces which were hard to justify.  It honestly may be easier to just
rewrite it as a full VFS rather than a wrapper.  Meanwhile, this should greatly
reduce the problem and the histograms should start to get a handle on the scope
of the problem.

[rmcilroy, 2015-11-05 15:34:46]

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.cc#newco...
sql/connection.cc:938: amount = 0;
If the database got truncated between the previous check and this one, leading
to amount < 0 then we will never update mmap_status to be kMmapSuccess. (also we
will send EVENT_MMAP_SUCCESS_NO_PROGRESS, when we should probably send
EVENT_MMAP_SUCCESS_NEW in this case). WDYT?

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.h#newcod...
sql/connection.h:713: size_t MmapHelper();
I'd call this something specific to it's return value - i.e.,
GetAppropriateMmapSize or similar

[Scott Hess - ex-Googler, 2015-11-05 18:21:19]

Rename helper and address truncate case.

[Scott Hess - ex-Googler, 2015-11-05 18:25:07]

PTAL?  Thanks.

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.cc#newco...
sql/connection.cc:938: amount = 0;
On 2015/11/05 15:34:46, rmcilroy wrote:
> If the database got truncated between the previous check and this one, leading
> to amount < 0 then we will never update mmap_status to be kMmapSuccess. (also
we
> will send EVENT_MMAP_SUCCESS_NO_PROGRESS, when we should probably send
> EVENT_MMAP_SUCCESS_NEW in this case). WDYT?

Good catch!

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/1426743006/diff/20001/sql/connection.h#newcod...
sql/connection.h:713: size_t MmapHelper();
On 2015/11/05 15:34:46, rmcilroy wrote:
> I'd call this something specific to it's return value - i.e.,
> GetAppropriateMmapSize or similar

Acknowledged.

https://codereview.chromium.org/1426743006/diff/40001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1426743006/diff/40001/sql/connection.cc#newco...
sql/connection.cc:929: if (SQLITE_OK != GetSqlite3FileAndSize(db_, &file,
&db_size)) {
I made this change because I found myself pondering whether the change to enter
the read loop with amount==0 would have any impact.  |rc| there clearly needed
to be local, but this |rc| doesn't need to stick around.

https://codereview.chromium.org/1426743006/diff/40001/sql/connection.cc#newco...
sql/connection.cc:950: if (amount <= 0 && mmap_ofs < db_size) {
Took this approach because replicating a subset of the else clause seems
brittle.

[rmcilroy, 2015-11-09 17:31:09]

lgtm, thanks.

[Scott Hess - ex-Googler, 2015-11-09 18:03:32]

rebase

[Scott Hess - ex-Googler, 2015-11-09 18:30:00]

shess@chromium.org changed reviewers:
+ isherman@chromium.org

[Scott Hess - ex-Googler, 2015-11-09 18:30:00]

isherman@ for histograms.xml.

The "success" case wasn't logged because that's the expected end point for all
users.

[Ilya Sherman, 2015-11-09 19:17:58]

histograms.xml lgtm

On 2015/11/09 18:30:00, Scott Hess wrote:
> The "success" case wasn't logged because that's the expected end point for all
> users.

I'm not sure what this means -- I see entries in histograms.xml for the success
case.  Could you please clarify?

[Scott Hess - ex-Googler, 2015-11-09 20:08:10]

On 2015/11/09 19:17:58, Ilya Sherman wrote:
> histograms.xml lgtm
> 
> On 2015/11/09 18:30:00, Scott Hess wrote:
> > The "success" case wasn't logged because that's the expected end point for
all
> > users.
> 
> I'm not sure what this means -- I see entries in histograms.xml for the
success
> case.  Could you please clarify?

[Bridging an offline discussion]

I meant the case where the success has been persisted so that it enables mmap
without doing any validation.  >99% of databases should end up there, because
most users won't have errors during validation and many users who do will
probably only see them for a single database (if they're seeing endemic problems
with I/O errors, it's unclear how Chrome works for them at all).

WRT comparing failures to successes, I expect to see relative numbers like 1
failure to a million successes, but that might still represent millions of
databases, which seems actionable.  I expect primarily MISSING (new databases)
then SUCCESS_NEW, then the failure cases should be a fraction of that, except
for FAILED which will gradually accumulate users (at a rate which FAILED_NEW
should help predict).

Hmm, also I guess comparing SUCCESS_NEW with FAILED_NEW probably gives an
approximation of overall occurrence in the fleet.

Mostly I want to see that users are making progress through the new code and not
getting stuck at an intermediate point.  Plain SUCCESS is stable, so it can
always be added in later if need be.


Thanks!

[Scott Hess - ex-Googler, 2015-11-09 20:08:16]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2015-11-09 20:08:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1426743006/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-09 20:08:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1426743006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1426743006/60001

[commit-bot: I haz the power, 2015-11-09 22:05:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-09 22:06:47]

Patchset 4 (id:??) landed as
https://crrev.com/de8784cf209f5c254f6c65718373985eb662b36f
Cr-Commit-Position: refs/heads/master@{#358672}

[robliao, 2015-11-10 00:09:38]

On 2015/11/09 22:06:47, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/de8784cf209f5c254f6c65718373985eb662b36f
> Cr-Commit-Position: refs/heads/master@{#358672}

A revert of this CL has been created:
https://codereview.chromium.org/1432953002/