[sqlite] Implement RecoverPager in terms of sqlite3_file.

[sqlite] Implement RecoverPager in terms of sqlite3_file.

SQLite's Pager structure is for internal use only, whereas sqlite3_file
is a documented interface available to SQLite clients or extensions.
Where Pager accesses the page cache with fallback to the disk file,
sqlite3_file only accesses the disk file.  This should not have
significant performance impact (recover.c only reads a given block
once), but the new version may not see modified pages in the page cache.
For this reason, a shared lock is taken to guarantee that the on-disk
format matches anything in the cache.

BUG=584407
Committed: https://crrev.com/47438a34859463b1304f6cbef9fdf7ab0dbdddd5
Cr-Commit-Position: refs/heads/master@{#374820}

[Scott Hess - ex-Googler, 2016-02-08 22:49:05]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[Scott Hess - ex-Googler, 2016-02-08 22:53:20]

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

[Scott Hess - ex-Googler, 2016-02-08 22:53:20]

https://codereview.chromium.org/1677193003/diff/1/third_party/sqlite/src/src/...
File third_party/sqlite/src/src/recover.c (right):

https://codereview.chromium.org/1677193003/diff/1/third_party/sqlite/src/src/...
third_party/sqlite/src/src/recover.c:412: const int kExcessSpace = 128;
No particular justification for 128, I just seem to recall it being the excess
last time I was looking (pages were 4096 + 128, etc).  I'm not sure how much
that kind of thing adds to the comment, though, it's pretty rando.

[Scott Hess - ex-Googler, 2016-02-08 22:53:46]

Hmm, like the outstanding review, recover.c is the meat, sqlite3.c should just
replicate it.

[commit-bot: I haz the power, 2016-02-08 22:55:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1677193003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1677193003/1

[commit-bot: I haz the power, 2016-02-09 00:52:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-09 00:52:08]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2016-02-10 19:44:53]

lgtm

[Scott Hess - ex-Googler, 2016-02-10 23:53:22]

Pager doesn't need to know reserved space.

[Scott Hess - ex-Googler, 2016-02-10 23:55:37]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2016-02-10 23:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1677193003/#ps20001
(title: "Pager doesn't need to know reserved space.")

[commit-bot: I haz the power, 2016-02-10 23:57:30]

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

[Scott Hess - ex-Googler, 2016-02-11 00:02:10]

Thanks!

https://codereview.chromium.org/1677193003/diff/1/third_party/sqlite/src/src/...
File third_party/sqlite/src/src/recover.c (right):

https://codereview.chromium.org/1677193003/diff/1/third_party/sqlite/src/src/...
third_party/sqlite/src/src/recover.c:437: pPage->pData =
sqlite3_malloc(pPage->nPageSize + kExcessSpace);
When doing a Really Last Chance Final Review, I noticed that this spot was
allocating based on the adjusted page size, but below it's doing a memset on the
pager's page size.  That made me think "This is confusing".  So I looked, and
the _only_ place the page-level nPageSize was used was the assert in the
PageData() helper.  So I just removed all of that.

I'm going to just land based on your existing LGTM, simply because the
nReservedSize thing is pretty cargo-cult in the first place, so backing it out
of this new code is The Right Thing To Do.  Also, I remembered a branch I had in
git which faked out that value to be non-zero, and tests worked in expected ways
(which is the level of thoroughness that support has ever managed, since I'm
never found a real vfs that requires it).

[commit-bot: I haz the power, 2016-02-11 01:12:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-16 22:33:38]

Description was changed from

==========
[sqlite] Implement RecoverPager in terms of sqlite3_file.

SQLite's Pager structure is for internal use only, whereas sqlite3_file
is a documented interface available to SQLite clients or extensions.
Where Pager accesses the page cache with fallback to the disk file,
sqlite3_file only accesses the disk file.  This should not have
significant performance impact (recover.c only reads a given block
once), but the new version may not see modified pages in the page cache.
For this reason, a shared lock is taken to guarantee that the on-disk
format matches anything in the cache.

BUG=584407
==========

to

==========
[sqlite] Implement RecoverPager in terms of sqlite3_file.

SQLite's Pager structure is for internal use only, whereas sqlite3_file
is a documented interface available to SQLite clients or extensions.
Where Pager accesses the page cache with fallback to the disk file,
sqlite3_file only accesses the disk file.  This should not have
significant performance impact (recover.c only reads a given block
once), but the new version may not see modified pages in the page cache.
For this reason, a shared lock is taken to guarantee that the on-disk
format matches anything in the cache.

BUG=584407

Committed: https://crrev.com/47438a34859463b1304f6cbef9fdf7ab0dbdddd5
Cr-Commit-Position: refs/heads/master@{#374820}
==========

[commit-bot: I haz the power, 2016-02-16 22:33:39]

Patchset 2 (id:??) landed as
https://crrev.com/47438a34859463b1304f6cbef9fdf7ab0dbdddd5
Cr-Commit-Position: refs/heads/master@{#374820}