551260 - (SQLite3.6.23) Upgrade to SQLite 3.6.23

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: moz changes v1.0 [Checkin: Comment 12]

Adds a bunch of better configure checks that we can now use with the new version.  The SQLite changes can be seen at http://hg.mozilla.org/users/sdwilsh_shawnwilsher.com/storage-async/file/59432453eb51/sqlite.upgrade

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

On Fedora 12 with sqlite 3.6.23-1.fc14 installed from the rawhide repo, this properly detects that SQLITE_SECURE_DELETE is not enabled.  (The sqlite.spec file indicates they only do: -DSQLITE_ENABLE_COLUMN_METADATA=1 -DSQLITE_DISABLE_DIRSYNC=1 -DSQLITE_ENABLE_FTS3=3 -DSQLITE_ENABLE_RTREE=1)

checking for SQLITE_SECURE_DELETE support in system SQLite... no
configure: error: System SQLite library is not compiled with SQLITE_SECURE_DELETE.
*** Fix above errors and then restart with               "/usr/bin/make -f client.mk build"

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 432631 [details] [diff] [review]
moz changes v1.0
[Checkin: Comment 12]

on file: configure.in line 6452
>     AC_MSG_RESULT($ac_cv_sqlite_enable_fts3)
>     CFLAGS="$_SAVE_CFLAGS"
>     LIBS="$_SAVE_LIBS"
>     if test "x$ac_cv_sqlite_enabled_fts3" = "xno"; then

typo alert: ac_cv_sqlite_enabled_fts3 is not like the others. (extra 'd')

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> typo alert: ac_cv_sqlite_enabled_fts3 is not like the others. (extra 'd')
will fix.  Also just found out from drh that what we actually want is sqlite3_compileoption_used("THREADSAFE=1") and the like.  Will update, but won't attach a new patch unless ted wants one.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #2)
> On Fedora 12 with sqlite 3.6.23-1.fc14 installed from the rawhide repo, this
> properly detects that SQLITE_SECURE_DELETE is not enabled.  (The sqlite.spec
> file indicates they only do: -DSQLITE_ENABLE_COLUMN_METADATA=1
> -DSQLITE_DISABLE_DIRSYNC=1 -DSQLITE_ENABLE_FTS3=3 -DSQLITE_ENABLE_RTREE=1)

For completeness, they also do pass some stuff to configure, although nothing that enables secure delete (but yes to thread safety):

%build
autoconf
export CFLAGS="$RPM_OPT_FLAGS -DSQLITE_ENABLE_COLUMN_METADATA=1 -DSQLITE_DISABLE_DIRSYNC=1 -DSQLITE_ENABLE_FTS3=3 -DSQLITE_ENABLE_RTREE=1 -Wall -fno-strict-aliasing"
%configure %{!?with_tcl:--disable-tcl} \
           --enable-threadsafe \
           --enable-threads-override-locks \
           --enable-load-extension \
           %{?with_tcl:TCLLIBDIR=%{tcl_sitearch}/sqlite3}

Comment 6

[Shawn Wilsher :sdwilsh]

er, clarification (from drh):
So sqlite3_compileoption_used("THREADSAFE") and sqlite3_compileoption_used("THREADSAFE=1") both return 1 if THREADSAFE is 1.  The first would be true and the second false if compiled with THREADSAFE=2.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> For completeness, they also do pass some stuff to configure, although nothing
> that enables secure delete (but yes to thread safety):
Of course, there are three values that control THREADSAFE (0, 1, 2), and I'm not sure what those configure options actually do...

Comment 8

[Reed Loden [:reed]]

Is Fedora shipping "Firefox", or is it rebranded as something else?

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #8)
> Is Fedora shipping "Firefox", or is it rebranded as something else?

"Firefox" using xulrunner.  Using system sqlite for Firefox 3.5.8 release 1.fc12 on Fedora 12 from the updates repo.  (Checked via /proc/PID/maps)

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 432631 [details] [diff] [review]
moz changes v1.0
[Checkin: Comment 12]

This is fine, but you might be better served in the long run by making that an actual autoconf macro so you could write these like:
MOZ_CHECK_SQLITE_OPTION([SQLITE_ENABLE_FTS3])

It mostly consists of writing the ugly m4 bits once, in a .m4 file in build/autoconf, ala:
http://mxr.mozilla.org/mozilla-central/source/build/autoconf/moznbytetype.m4

then just calling them from configure, ala:
http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#2600

If you don't want to do that now, can you file a followup bug to do it?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> If you don't want to do that now, can you file a followup bug to do it?
Let's do a follow-up for now.  Filed bug 554115.

Comment 12

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/2ba69e5220c4
http://hg.mozilla.org/mozilla-central/rev/4bb83011e890

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-CC) Copy it to comm-central [Checkin: Comment 25]

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

No need to do anything about it, but as an FYI apparently the 3.6.23 FTS3 offsets() implementation is a bit more picky and is returning errors when confronted with Thunderbird's custom fts3 tokenizer.  This has been filed as bug 554215 (against our tokenizer) and will be addressed in comm-central sometime tomorrow.

Comment 15

[Walter Meinl]

Shawn, running configure on my Gentoo-build with native sqlite breaks, though all requirements are met.
sqlite3_compileoption_used returns for sqlite_secure_delete SQLITE_SECURE_DELETE and for sqlite_enable_fts3 SQLITE_ENABLE_FTS3. Note, there is no '=1' in the return statements for these two options (in contrast to THREADSAFE, which indeed returns THREADSAFE=1). So the configure checks for sqlite_secure_delete and sqlite_enable_fts3 should be like in your attachment here. Open a new bug?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Shawn, running configure on my Gentoo-build with native sqlite breaks, though
> all requirements are met.
> sqlite3_compileoption_used returns for sqlite_secure_delete
> SQLITE_SECURE_DELETE and for sqlite_enable_fts3 SQLITE_ENABLE_FTS3. Note, there
> is no '=1' in the return statements for these two options (in contrast to
> THREADSAFE, which indeed returns THREADSAFE=1). So the configure checks for
> sqlite_secure_delete and sqlite_enable_fts3 should be like in your attachment
> here. Open a new bug?
Open a new bug as this is not the behavior that was described to me by drh.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Attachment: fts3-offsets-asplode.sql

Ugh, the problem in comment 14 was not Thunderbird's tokenizer's fault.  Using fts3 binaries downloaded from sqlite.org:

$ cat fts3-offsets-asplode.sql
CREATE VIRTUAL TABLE ft USING fts3(tokenize=porter, fulltextOne, fulltextTwo);
INSERT INTO ft VALUES("", "foo");
INSERT INTO ft VALUES("foo", "foo");
SELECT offsets(ft) FROM ft WHERE ft MATCH "foo";

$ sqlite3-3.6.22 < fts3-offsets-asplode.sql 
1 0 0 3
0 0 0 3 1 0 0 3

$ sqlite3-3.6.23 < fts3-offsets-asplode.sql 
Error: near line 4: database disk image is malformed

My summary thus far has identified the sqlite3Fts3Offsets implementation as the source of the error.  It seems upset that the first column contains an empty string and the tokenizer returns SQLITE_DONE.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

I reported what I'm claiming is the bug on the sqlite-users list.

I am unclear whether it is appropriate to back 3.6.23 out.  mozilla-central does not use FTS3, so there's nothing in-tree that gets broken by this.  Thunderbird's "Thunderbird" tinderbox tree that builds against mozilla-central is just a (currently orange on unit tests) canary to help keep up with trunk and is not the active development series (that builds against mozilla-1.9.2).

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

The SQLite people agreed that what I thought a bug was in fact a bug and fixed it:
http://www.sqlite.org/src/ci/d37034f7fc

If they're not already going to spin a new release in the near future I think we should request a one-off release.

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> The SQLite people agreed that what I thought a bug was in fact a bug and fixed
> it:
> http://www.sqlite.org/src/ci/d37034f7fc
> 
> If they're not already going to spin a new release in the near future I think
> we should request a one-off release.
Or backout for the time being.  I'm OK with whichever since we don't really need anything from 3.6.23 (other than the cofigure checks)

Comment 21

[Shawn Wilsher :sdwilsh]

I just talked to drh and he said they'll have a 3.6.23.1 release early next week for us to upgrade to.  Let's keep this in the tree for now.

asuth - do you want to own that upgrade?

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

Sure, should I just use the standing rs for SQLite upgrades?  (I know to modify the configure.in and the mozilla readme in addition to overwriting the SQLite files...)

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Sure, should I just use the standing rs for SQLite upgrades?  (I know to modify
> the configure.in and the mozilla readme in addition to overwriting the SQLite
> files...)
Yup, but please file a new bug for it! :)

Comment 24

[Justin Wood (:Callek)]

Comment on attachment 434098 [details] [diff] [review]
(Cv1-CC) Copy it to comm-central
[Checkin: Comment 25]

note to asuth, rs+ on adjusting this var on the c-c side with your upgrade.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 434098 [details] [diff] [review]
(Cv1-CC) Copy it to comm-central
[Checkin: Comment 25]


http://hg.mozilla.org/comm-central/rev/78563f185a8e