Closed Bug 666420 (SQLite3.7.7.1) Opened 9 years ago Closed 8 years ago

Upgrade to SQLite 3.7.7.1

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 1 obsolete file)

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.
(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?
(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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Depends on: 666558
Attached patch Mozilla changes v1.0 (obsolete) — Splinter Review
tests results: http://tbpl.mozilla.org/?tree=Try&rev=78fe8779dc4a
talos compare: http://tinyurl.com/5s9e243

Everything seems fine or in the expected noise.
SQLite 3.7.7 has been officially released.
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.
Attachment #541353 - Flags: review?
Attachment #541353 - Flags: review? → review?(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 <me@shawnwilsher.com>, 03/2011

You are not me, nor is it 03/2011 ;)
Attachment #541353 - Flags: review?(sdwilsh) → review+
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.
Alias: SQLite3.7.7 → SQLite3.7.7.1
Summary: Upgrade to SQLite 3.7.7 → Upgrade to SQLite 3.7.7.1
Attachment #541353 - Attachment is obsolete: true
Blocks: 625981
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
This is needed since SQLite updated the vfs version
Attachment #554408 - Flags: review?(sdwilsh)
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
something like this, testing on try.
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
Attachment #554831 - Flags: review?(sdwilsh)
Blocks: SQLite3.7.8
(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 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
Attachment #554408 - Flags: review?(sdwilsh) → review+
Comment on attachment 554831 [details] [diff] [review]
Use fsync on Android

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

r=sdwilsh
Attachment #554831 - Flags: review?(sdwilsh) → review+
(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.
Depends on: 682676
Blocks: SQLite3.7.9
You need to log in before you can comment on or make changes to this bug.