581000 - (SQLite3.7.0.1) Upgrade to SQLite 3.7.0.1

Status: RESOLVED WONTFIX

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Marco Bonardo [:mak]]

The most interesting things from this release:
- WAL journaling can reduce fsyncs, we want it for Places and probably urlclassifier. It can help I/O.
- query planner enhancements can speed up queries globally

Comment 1

[Marco Bonardo [:mak]]

forgot to add reasoning.
This should be blocking because it goes to the right direction of I/O and database fragmentation reduction. and perf++. Removing our temp tables will improve statements performances but will increase fsyncs for visits, this should compensate for it plus giving wins on fragmentation (Taras discovered that also vacuum cannot fight gragmentation too well on highly written databases).
It also has an enhancement for memory measure that could be useful for about:memory (heap memory used by cache, per connection).
Risk is the usual for sqlite upgrades, they have nice tests and QA.

Comment 2

[Shawn Wilsher :sdwilsh]

I'll work on this once it gets blocking status...

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: Mozilla Changes

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: SQLite Changes

Oddly enough, this is small enough to upload...

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

If we're going to do this, we'll want it in the next beta so we can live with it for long enough that any unforseen problems will appear.

Comment 6

[Shawn Wilsher :sdwilsh]

Pushed to the try server in changeset 64bbbfa74973

Comment 7

[Shawn Wilsher :sdwilsh]

Try hated that one and did not build a thing.  I pushed it again in changeset 1209077b6e8a

Comment 8

[John D.]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=581374

60 lines of C to add sqlite compression, shrinks db 50-80% at a low cycle cost.

Comment 9

[Shawn Wilsher :sdwilsh]

Try looked OK, even if it only ran win2003 unit tests.  I ran an experimental build before and it worked out just fine, so I don't expect any issues.

Will land this once the tree greens up a bit.

Comment 10

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/7a3ecace6732
http://hg.mozilla.org/mozilla-central/rev/18127ac02bf2

Comment 11

[Boris Zbarsky [:bzbarsky]]

This caused a "Regression: Ts Shutdown, MAX Dirty Profile increase 21.3% on Linux Firefox" according to the graphs bot.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> This caused a "Regression: Ts Shutdown, MAX Dirty Profile increase 21.3% on
> Linux Firefox" according to the graphs bot.
Right, and as commented on the threads for that, the benefits we'll get with enabling WAL on places should correct that.  We've also had massive wins on that benchmark for Firefox 4.0, and last I checked we are still far ahead of Firefox 3.6.

Comment 13

[Daniel C]

(In reply to comment #12)
> (In reply to comment #11)
> > This caused a "Regression: Ts Shutdown, MAX Dirty Profile increase 21.3% on
> > Linux Firefox" according to the graphs bot.
> Right, and as commented on the threads for that, the benefits we'll get with
> enabling WAL on places should correct that.  We've also had massive wins on
> that benchmark for Firefox 4.0, and last I checked we are still far ahead of
> Firefox 3.6.

Where are those threads? I'm not sure where to look, but I can only find http://groups.google.co.uk/group/mozilla.dev.tree-management/browse_thread/thread/9c4a31c6eb105aca?hl=en#

Comment 14

[Shawn Wilsher :sdwilsh]

Weird.  I know I responded to that, but I don't see it anywhere. :(

Comment 15

[Daniel C]

It's unclear to me from comment 12 whether the regression was expected or not, and if so, why. If it wasn't, then regardless of whether bug 573492 cancels out the difference or not, shouldn't it be looked into?

Comment 16

[Shawn Wilsher :sdwilsh]

It is not expected, but the argument I am making is that the benefits outweigh the costs here.

Comment 17

[Shawn Wilsher :sdwilsh]

This just got backed out.  People are seeing 3.7.0 reporting some databases as corrupted when running queries on them, but then going back to 3.6.22 and not seeing these same errors.  I've sent one database to the SQLite team to take a look.  This may end up being WONTFIXed, or we might reland.  Results pending.

http://hg.mozilla.org/mozilla-central/rev/e5e6583bd137

(also no reason to block beta 3 on this; it just needs beta coverage although beltzner may disagree)

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

New blocking status is fine, thanks for letting us (and more importantly laura mesa!) know.

Comment 19

[[not reading bugmail]]

places.sqlite was one that got badly corrupted on my machine that i finally deleted it per advise from others, and after that the browser has appeared to be ok.  Didn't think to try to save it though.

Comment 20

[Shawn Wilsher :sdwilsh]

It looks like the corruption isn't actually corruption.  The file size stored in the database header is wrong in the one database file I sent to the SQLite team.  As a result, 3.7.0 was missing pages from the end of the file, causing the corrupt error messages.

The databases should work fine with SQLite 3.6.23.1.  Not WONTFIXing yet until we learn more, but I suspect that that will be the case here.

Comment 21

[[not reading bugmail]]

Basically for me, History was not working properly.  The UI didn't work the way it should if there was no problem.  For example, I would open a web page from session restore, click on link to the new page, and there was no page to go back to as the back button turned into the disabled state and history didn't record itself in the history list.

Comment 22

[Shawn Wilsher :sdwilsh]

So, we'll want to take 3.7.0.1 which has a fix for the corruption issue.  Details here:
http://www.sqlite.org/src/info/51ae9cad31

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: Mozilla Changes

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: SQLite Changes

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we have tests for the corruption issue that we caught so that if it happens again we can catch it again before landing?

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Do we have tests for the corruption issue that we caught so that if it happens
> again we can catch it again before landing?
The SQLite guys have tests for this now (and they run their full test suite before releasing), but in order for us to test it, I'd have to have a copy of SQLite before 3.7.0 in the tree and one after, and build both, and then have them both write to the database.

Comment 28

[Shawn Wilsher :sdwilsh]

This time, it appears to have landed without any performance regressions (which is likely fixed by the query plan optimization bug they mention in the list of changes).

http://hg.mozilla.org/mozilla-central/rev/280b993408e7
http://hg.mozilla.org/mozilla-central/rev/b840177c952a

Comment 29

[Shawn Wilsher :sdwilsh]

Hmm, still have Ts Shutdown regressions of about the same size.  Again though, we are still under Firefox 3.6, and have other fixes in the work that depend on this that will improve the shutdown (and startup, and Tp) time.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #29)
> Hmm, still have Ts Shutdown regressions of about the same size.  Again though,
> we are still under Firefox 3.6, and have other fixes in the work that depend on
> this that will improve the shutdown (and startup, and Tp) time.

Sounds fine to me.

Comment 31

[Daniel Holbert [:dholbert]]

Backed out this bug's changesets, for causing bug 583821.
 backout: http://hg.mozilla.org/mozilla-central/rev/431901d7a495
 merge: http://hg.mozilla.org/mozilla-central/rev/bf982bac3ccd
 backout: http://hg.mozilla.org/mozilla-central/rev/f069efb502f6
 merge: http://hg.mozilla.org/mozilla-central/rev/a9b21fa2b7ab

See in particular bug 583821 comment 36 and bug 583821 comment 40.  Looks like we need to wait for SQLite 3.7.1.

Comment 32

[Shawn Wilsher :sdwilsh]

Thanks a ton!

Comment 33

[Shawn Wilsher :sdwilsh]

We are not going to take this fix now, and will wait for SQLite 3.7.1 due to comment 31.

Will carry over blocking flags to there.

Comment 34

[Daniel Holbert [:dholbert]]

FWIW, the tbpl cycle on this backout had a test-failure due to a storage-related NS_ABORT_IF_FALSE failure, on a Win2003 Md Oth cycle.  The NS_ABORT_IF_FALSE failure was:
{
###!!! ABORT: Getting value failed, wrong column index?: 'NS_SUCCEEDED(rv)', file e:\builds\moz2_slave\mozilla-central-win32-debug\build\obj-firefox\dist\include\mozIStorageStatement.h, line 320
}
Log is:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282016084.1282018131.8619.gz
WINNT 5.2 mozilla-central debug test mochitest-other on 2010/08/16 20:34:44

Comment 31 was just a clean backout of this bug's changesets, in reverse order (hg backout b840177c952a, hg merge, hg commit; repeat for changeset 280b993408e7).  So I don't think it was at fault. Feel free to audit it if you suspect it, though.

Comment 35

[Daniel Holbert [:dholbert]]

The ABORT_IF_FALSE from comment 34 is actually preceeded by this (non-fatal) assertion (visible in full log but not in shortlog):
###!!! ASSERTION: hasResult is false but the call succeeded?: 'hasResult', file e:/builds/moz2_slave/mozilla-central-win32-debug/build/toolkit/components/places/src/nsNavHistory.cpp, line 1941

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> The ABORT_IF_FALSE from comment 34 is actually preceeded by this (non-fatal)
> assertion (visible in full log but not in shortlog):
> ###!!! ASSERTION: hasResult is false but the call succeeded?: 'hasResult', file
> e:/builds/moz2_slave/mozilla-central-win32-debug/build/toolkit/components/places/src/nsNavHistory.cpp,
> line 1941
I believe that is a known intermittent failure.

Comment 37

[Daniel Holbert [:dholbert]]

Yeah -- in any case, it went green later, so it looks like it was just a particularly coincidental intermittent orange.  Sorry for the bug-spam -- disregard comment 34 through this one.