Upstream sqlite gyp changes for Android.

Upstream sqlite gyp changes for Android.

Includes a change to sql/Connection to propagate the auto_vacuum pragma across a raze operation. Also updates test expectations.

BUG=137853
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150128

[Yaron, 2012-07-27 21:34:22]

I'm not entirely confident about the change to the test case. I could also just
hard-code a new constant and if-def it for Android, or I'm open to suggestions
that you might have.

I know that the change stems from the use of SQLITE_DEFAULT_AUTOVACUUM. Looking
through source I couldn't determine why but I know that the first call to PRAGMA
page_count returns 3 as a result of that change.

cc andreip who selected these flags originally (seemingly based on Android
compilation flgs)

[Scott Hess - ex-Googler, 2012-07-27 22:23:29]

On 2012/07/27 21:34:22, Yaron wrote:
> I'm not entirely confident about the change to the test case. I could also
just
> hard-code a new constant and if-def it for Android, or I'm open to suggestions
> that you might have.

It might be worthwhile to get the value of "PRAGMA AUTO_VACUUM", and condition
the test on that.  That would be scalable to any platform using that setting, or
if we change the system to use that automatically.

> I know that the change stems from the use of SQLITE_DEFAULT_AUTOVACUUM.
Looking
> through source I couldn't determine why but I know that the first call to
PRAGMA
> page_count returns 3 as a result of that change.

AUTO_VACUUM maintains a freelist, which is what's in that other page.

http://codereview.chromium.org/10829062/diff/4001/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

http://codereview.chromium.org/10829062/diff/4001/sql/connection_unittest.cc#...
sql/connection_unittest.cc:159: EXPECT_EQ(page_count_before_raze,
s.ColumnInt(3));
Hmm.  Maybe toss in a comment to the end of "Table foo is stored in the last
page of the file.

http://codereview.chromium.org/10829062/diff/4001/sql/connection_unittest.cc#...
sql/connection_unittest.cc:168: EXPECT_LT(s.ColumnInt(0),
page_count_before_raze);
And then here, add a comment that the last page of the file went away.

Since you're moving to dynamic values, maybe you should also snag the page count
before the table was created, so you can use _EQ here.  Raze() is kind of
outside-the-box, so I used very explicit values to assert that if things
changed, we'd pay really close attention to it.  Here it should have the same
page_count as a clean newly-created database.

Actually, given your changes in the gyp file, you might want to add a test that
AUTO_VACUUM retains state across the Raze().  Basically it should have the same
value before and after.

[Scott Hess - ex-Googler, 2012-07-27 22:26:27]

BTW - I'll be out of reach of electronics this weekend to next weekend.  I don't
think there's a ton for me to add, but if you have other questions, now's the
time to fire them off!

[Yaron, 2012-07-28 00:27:07]

On 2012/07/27 22:26:27, shess wrote:
> BTW - I'll be out of reach of electronics this weekend to next weekend.  I
don't
> think there's a ton for me to add, but if you have other questions, now's the
> time to fire them off!

I think you've got it covered. I think the explanation of the freelist is
sufficient to just adjust the constants conditionally. I won't be able to get
back to you today so I guess this'll be in your hopper when you get back.

[Yaron, 2012-07-30 20:57:13]

Ok, updated per your suggestions. Now also preserves auto_vacuum across a Raze

[andreip3000, 2012-08-01 11:46:06]

LGTM (but you should probably wait for shess@ to LGTM too)

One question below.

http://codereview.chromium.org/10829062/diff/4/third_party/sqlite/sqlite.gyp
File third_party/sqlite/sqlite.gyp (right):

http://codereview.chromium.org/10829062/diff/4/third_party/sqlite/sqlite.gyp#...
third_party/sqlite/sqlite.gyp:133: 'DSQLITE_DEFAULT_FILE_FORMAT=4',
Should we match the flags that Android uses?

https://android.googlesource.com/platform/external/sqlite/+/2d758ba484351cd86...

I remember that was our intention originally but I think we diverged over time.

One notable setting is powersave overwrite which was enabled in this change:

https://android.googlesource.com/platform/external/sqlite/+/2d758ba484351cd86...

Looking at the SQLite documentation, it seems the default is 'on' anyway.
However, maybe it's better to be explicit about it too (like Android is).

[Yaron, 2012-08-04 01:14:43]

http://codereview.chromium.org/10829062/diff/4/third_party/sqlite/sqlite.gyp
File third_party/sqlite/sqlite.gyp (right):

http://codereview.chromium.org/10829062/diff/4/third_party/sqlite/sqlite.gyp#...
third_party/sqlite/sqlite.gyp:133: 'DSQLITE_DEFAULT_FILE_FORMAT=4',
On 2012/08/01 11:46:06, andreip3000 wrote:
> Should we match the flags that Android uses?
> 
>
https://android.googlesource.com/platform/external/sqlite/+/2d758ba484351cd86...
> 
> I remember that was our intention originally but I think we diverged over
time.
> 
> One notable setting is powersave overwrite which was enabled in this change:
> 
>
https://android.googlesource.com/platform/external/sqlite/+/2d758ba484351cd86...
> 
> Looking at the SQLite documentation, it seems the default is 'on' anyway.
> However, maybe it's better to be explicit about it too (like Android is).

I took the set that we had previously and just removed redundant ones (from the
shared block on L12 above)

[Scott Hess - ex-Googler, 2012-08-05 15:34:46]

LGTM, with addition indicated.

Thanks for wiring up the auto-vacuum propagation.  Raze() is not used much at
this point, but over time it will be used in any db recovery system that is
implemented.

http://codereview.chromium.org/10829062/diff/4/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

http://codereview.chromium.org/10829062/diff/4/sql/connection_unittest.cc#new...
sql/connection_unittest.cc:150: }
2 is a valid auto_vacuum result.  I don't know how things are setup in that
case, but if the following tests succeed, it would be coincidental.  I think it
would be reasonable to have an ASSERT_TRUE(auto_vacuum == 0 || auto_vacuum == 1)
here.  ASSERT_TRUE because if it's false, then the assumptions in the test are
not valid.

[Yaron, 2012-08-06 17:23:43]

Thanks for reviews

http://codereview.chromium.org/10829062/diff/4/sql/connection_unittest.cc
File sql/connection_unittest.cc (right):

http://codereview.chromium.org/10829062/diff/4/sql/connection_unittest.cc#new...
sql/connection_unittest.cc:150: }
On 2012/08/05 15:34:46, shess wrote:
> 2 is a valid auto_vacuum result.  I don't know how things are setup in that
> case, but if the following tests succeed, it would be coincidental.  I think
it
> would be reasonable to have an ASSERT_TRUE(auto_vacuum == 0 || auto_vacuum ==
1)
> here.  ASSERT_TRUE because if it's false, then the assumptions in the test are
> not valid.

Done.

[commit-bot: I haz the power, 2012-08-06 17:23:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10829062/14001

[commit-bot: I haz the power, 2012-08-06 17:44:25]

Try job failure for 10829062-14001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-06 18:09:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10829062/14001

[commit-bot: I haz the power, 2012-08-06 19:22:43]

Change committed as 150128