[sqlite] Fix bug in how recover virtual table handles empty.

[sqlite] Fix bug in how recover virtual table handles empty.

Previously the code assumed that the current cell was always less than
the number of cells if it was a valid leaf page (see top of
leafCursorCellDecode).  Empty tables have a single root page with no
cells.

No bugs have been seen which trace to this.  In testing what actually
happens with a newly-created empty table, I found that
leafCursorCellDecode exits with a ValidateError.  By inspection, I
believe the most likely falsely valid result would be a single
previously-valid row which has been deleted from the table.

BUG=none

Committed: https://crrev.com/f4b7a92eaad7828cc0ba43c53cdebc6eb4788df6
Cr-Commit-Position: refs/heads/master@{#373853}

[Scott Hess - ex-Googler, 2016-02-05 01:25:26]

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

[commit-bot: I haz the power, 2016-02-05 01:27:16]

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

[Scott Hess - ex-Googler, 2016-02-05 01:35:47]

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

[Scott Hess - ex-Googler, 2016-02-05 01:35:48]

You get off easy, today!  I was preparing some code to start refactoring the
SQLite recover.c implementation, and discovered a bug.  I figure it's worth
fixing the bug.

How Things Work is something like:
 - leafCursorLoadPage loads sets up RecoverLeafCursor with the passed page to
start reading at cell 0.
 - leafCursorCellDecode decodes the current cell (0 in this case).
 - if leafCursorCellDecode fails due to something wrong with the cell,
leafCursorNextValidCell is called to move the cursor forward and call
leafCursorCellDecode on it.

That all worked fine for things like "Table consisting of corruption followed by
good stuff" and "Table consisting entirely of corruption", but didn't handle the
case of "Table has nothing at all."

Actually, mostly the handling is where it is because the "Not a leaf page"
handling directly above is presumably correct, so this should also be correct. 
leafCursorLoadPage() callers leafCursorNextPage() and leafCursorCreatePage()
handle the case of SQLITE_OK with a missing page to mean "keep going".

[commit-bot: I haz the power, 2016-02-05 02:32:01]

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

[commit-bot: I haz the power, 2016-02-05 02:32:02]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2016-02-05 17:51:26]

lgtm

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

https://codereview.chromium.org/1671773003/diff/1/third_party/sqlite/src/src/...
third_party/sqlite/src/src/recover.c:1175: }
This diff appears to be basically the same as the diff to the previous file. Is
one an auto-generated version of the other, or some such?

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

https://codereview.chromium.org/1671773003/diff/1/third_party/sqlite/src/src/...
third_party/sqlite/src/src/recover.c:1173: if( nCells<1 ){
The style... it burns my eyes!

[Scott Hess - ex-Googler, 2016-02-05 18:18:05]

thanks

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

https://codereview.chromium.org/1671773003/diff/1/third_party/sqlite/src/src/...
third_party/sqlite/src/src/recover.c:1175: }
On 2016/02/05 17:51:25, Ryan Hamilton wrote:
> This diff appears to be basically the same as the diff to the previous file.
Is
> one an auto-generated version of the other, or some such?

Oh, sorry, should have mentioned that, too.

In third_party/sqlite, the actual source data is in src/src, and there's a
SQLite-provided script which merges everything into amalgamation/sqlite3.c. 
It's a primitive approach to full-program optimization, and the standard source
SQLite ships.

Additionally, we keep patch files, to keep track of our actual changes.  This is
because we can't have individual patches as CLs, because it would break the
waterfall, but if we squash them then everything is all mixed together.  I
didn't regen the patch files in this case, because I plan to make other
recover.c changes.

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

https://codereview.chromium.org/1671773003/diff/1/third_party/sqlite/src/src/...
third_party/sqlite/src/src/recover.c:1173: if( nCells<1 ){
On 2016/02/05 17:51:25, Ryan Hamilton wrote:
> The style... it burns my eyes!

Yeah!  It's essentially opposite Google/Chromium style :-).  And
psuedo-Hungrarian notation is always a joy.

[Scott Hess - ex-Googler, 2016-02-05 18:18:10]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2016-02-05 18:18:50]

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

[commit-bot: I haz the power, 2016-02-05 18:25:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-05 18:26:47]

Description was changed from

==========
[sqlite] Fix bug in how recover virtual table handles empty.

Previously the code assumed that the current cell was always less than
the number of cells if it was a valid leaf page (see top of
leafCursorCellDecode).  Empty tables have a single root page with no
cells.

No bugs have been seen which trace to this.  In testing what actually
happens with a newly-created empty table, I found that
leafCursorCellDecode exits with a ValidateError.  By inspection, I
believe the most likely falsely valid result would be a single
previously-valid row which has been deleted from the table.

BUG=none
==========

to

==========
[sqlite] Fix bug in how recover virtual table handles empty.

Previously the code assumed that the current cell was always less than
the number of cells if it was a valid leaf page (see top of
leafCursorCellDecode).  Empty tables have a single root page with no
cells.

No bugs have been seen which trace to this.  In testing what actually
happens with a newly-created empty table, I found that
leafCursorCellDecode exits with a ValidateError.  By inspection, I
believe the most likely falsely valid result would be a single
previously-valid row which has been deleted from the table.

BUG=none

Committed: https://crrev.com/f4b7a92eaad7828cc0ba43c53cdebc6eb4788df6
Cr-Commit-Position: refs/heads/master@{#373853}
==========

[commit-bot: I haz the power, 2016-02-05 18:26:48]

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