666420 - (SQLite3.7.7.1) Upgrade to SQLite 3.7.7.1

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We may evaluate to skip 3.7.6 and rather take 3.7.7, that should be out at the end of June.

The advantage is mostly for Mobile, where we can force strict size limits for WAL journals.

As for 3.7.6 this brings on query planner changes, but measurements did not show interesting changes.
As for 3.7.6 this increases randomness in bug 610789, but I have a slightly more knowledge of some differences regarding this failure, and I'm testing a patch to check these theories (see https://bugzilla.mozilla.org/show_bug.cgi?id=610789#c54 and following), I'm also in contact with SQLite team about this issue.

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> As for 3.7.6 this brings on query planner changes, but measurements did not
> show interesting changes.
Have we tested this now that ANALYZE has been run on the MIN/MED/MAX profiles by chance?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #1)
> (In reply to comment #0)
> > As for 3.7.6 this brings on query planner changes, but measurements did not
> > show interesting changes.
> Have we tested this now that ANALYZE has been run on the MIN/MED/MAX
> profiles by chance?

I just got a new snapshot with a new compile-time option to test (For bug 610789), so I'll also run Talos on it and check again.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Mozilla changes v1.0

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

tests results: http://tbpl.mozilla.org/?tree=Try&rev=78fe8779dc4a
talos compare: http://tinyurl.com/5s9e243

Everything seems fine or in the expected noise.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

SQLite 3.7.7 has been officially released.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 541353 [details] [diff] [review]
Mozilla changes v1.0

asking rs for the upgrade.
Notice it will need bug 666558 to be landed at the same time.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 541353 [details] [diff] [review]
Mozilla changes v1.0

Review of attachment 541353 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

::: db/sqlite3/README.MOZILLA
@@ +2,2 @@
>  
>  -- Shawn Wilsher <[e-mail]>, 03/2011

You are not me, nor is it 03/2011 ;)

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

moving to 3.7.7.1 that has a small fix for case_sensitive_like. we don't use it internally but some addon may use it and the fix is small enough. So far SQLite trunk timeline seems stable enough.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Mozilla changes v1.1

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, looks like also the telemtry vfs needs to be updated, vfs iVersion has been bumped from 2 to 3, the next wrappers have to be added:
xSetSystemCall
xGetSystemCall
xNextSystemCall

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Telemetry VFS shim update

This is needed since SQLite updated the vfs version

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

looks like the new android ndk doesn't like this much

sqlite3.o: In function `full_fsync':
/builds/slave/try-lnx-andrd-dbg/build/db/sqlite3/src/sqlite3.c:27698: undefined reference to `fdatasync'

Ah, I just found someone changed sqlite3.c without getting review from a Storage peer
http://hg.mozilla.org/mozilla-central/rev/b854ffeef0d1

ted requested this to be upstreamed in bug 617115 but I don't see if it was ever done, btw to me looks like it could have been handled at a Makefile level by defining fdatasync=fsync on Android

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Use fsync on Android

something like this, testing on try.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 554831 [details] [diff] [review]
Use fsync on Android

I'd say it works fine.
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=5071fbce7acb

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to Marco Bonardo [:mak] from comment #12)
> Ah, I just found someone changed sqlite3.c without getting review from a
> Storage peer
> http://hg.mozilla.org/mozilla-central/rev/b854ffeef0d1
Dammit.  I yelled in the bug.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 554408 [details] [diff] [review]
Telemetry VFS shim update

Review of attachment 554408 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

::: storage/src/TelemetryVFS.cpp
@@ +434,4 @@
>    return orig_vfs->xCurrentTimeInt64(orig_vfs, piNow);
>  }
>  
> +static int xSetSystemCall(sqlite3_vfs *vfs, const char *zName,

nit: `static` and `int` should be on their own line

@@ +440,5 @@
> +  sqlite3_vfs *orig_vfs = static_cast<sqlite3_vfs*>(vfs->pAppData);
> +  return orig_vfs->xSetSystemCall(orig_vfs, zName, pFunc);
> +}
> +
> +static sqlite3_syscall_ptr xGetSystemCall(sqlite3_vfs *vfs, const char *zName)

same with here

@@ +446,5 @@
> +  sqlite3_vfs *orig_vfs = static_cast<sqlite3_vfs*>(vfs->pAppData);
> +  return orig_vfs->xGetSystemCall(orig_vfs, zName);
> +}
> +
> +static const char *xNextSystemCall(sqlite3_vfs *vfs, const char *zName)

and here

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 554831 [details] [diff] [review]
Use fsync on Android

Review of attachment 554831 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Shawn Wilsher :sdwilsh from comment #16)
> Comment on attachment 554408 [details] [diff] [review]
> Telemetry VFS shim update
> 
> Review of attachment 554408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=sdwilsh
> 
> ::: storage/src/TelemetryVFS.cpp
> @@ +434,4 @@
> >    return orig_vfs->xCurrentTimeInt64(orig_vfs, piNow);
> >  }
> >  
> > +static int xSetSystemCall(sqlite3_vfs *vfs, const char *zName,
> 
> nit: `static` and `int` should be on their own line

right, I din't change those because this shim is practically a 1:1 copy of the sqlite example shim and it was simpler to backport changes, btw doesn't matter that much, so I'll fix those.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2fd69e4236ea
http://hg.mozilla.org/integration/mozilla-inbound/rev/8a285afca9c9
http://hg.mozilla.org/integration/mozilla-inbound/rev/43cb0ce4fe9e
http://hg.mozilla.org/integration/mozilla-inbound/rev/19dff2b200b9

Comment 20

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/19dff2b200b9
http://hg.mozilla.org/mozilla-central/rev/43cb0ce4fe9e
http://hg.mozilla.org/mozilla-central/rev/8a285afca9c9
http://hg.mozilla.org/mozilla-central/rev/2fd69e4236ea