Closed Bug 1188578 (SQLite3.8.11.1) Opened 4 years ago Closed 4 years ago

Upgrade to SQLite 3.8.11.1

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

()

Details

Attachments

(1 file, 2 obsolete files)

SQLite Release 3.8.11 On 2015-07-27

* Added the experimental RBU extension. Note that this extension is experimental and subject to change in incompatible ways.
* Added the experimental FTS5 extension. Note that this extension is experimental and subject to change in incompatible ways.
* Added the sqlite3_value_dup() and sqlite3_value_free() interfaces.
* Enhance the spellfix1 extension to support ON CONFLICT clauses.
* The IS operator is now able to drive indexes.
* Enhance the query planner to permit automatic indexing on FROM-clause subqueries that are implemented by co-routine.
* Disallow the use of "rowid" in common table expressions.
* Added the PRAGMA cell_size_check command for better and earlier detection of database file corruption.
* Added the matchinfo 'b' flag to the matchinfo() function in FTS3.
* Improved fuzz-testing of database files, with fixes for problems found.
* Add the fuzzcheck test program and automatically run this program using both SQL and database test cases on "make test".
* Added the SQLITE_MUTEX_STATIC_VFS1 static mutex and use it in the Windows VFS.
* The sqlite3_profile() callback is invoked (by sqlite3_reset() or sqlite3_finalize()) for statements that did not run to completion.
* Enhance the page cache so that it can preallocate a block of memory to use for the initial set page cache lines. Set the default preallocation to 100 pages. Yields about a 5% performance increase on common workloads.
* Miscellaneous micro-optimizations result in 22.3% more work for the same number of CPU cycles relative to the previous release. SQLite now runs twice as fast as version 3.8.0 and three times as fast as version 3.3.9. (Measured using cachegrind on the speedtest1.c workload on Ubuntu 14.04 x64 with gcc 4.8.2 and -Os. Your performance may vary.)
* Added the sqlite3_result_zeroblob64() and sqlite3_bind_zeroblob64() interfaces.

Important bug fixes:
* Fix CREATE TABLE AS so that columns of type TEXT never end up holding an INT value. Ticket f2ad7de056ab1dc9200
* Fix CREATE TABLE AS so that it does not leave NULL entries in the sqlite_master table if the SELECT statement on the right-hand side aborts with an error. Ticket 873cae2b6e25b
* Fix the skip-scan optimization so that it works correctly when the OR optimization is used on WITHOUT ROWID tables. Ticket 8fd39115d8f46
* Fix the sqlite3_memory_used() and sqlite3_memory_highwater() interfaces so that they actually do provide a 64-bit answer.
Alias: SQLite3.8.11
Oh dear, this is definitely hitting test failures.

https://treeherder.mozilla.org/logviewer.html#?job_id=9869817&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=9869791&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=9869792&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=9869805&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=9869805&repo=try

drh, we ran a Try push off the 14-July amalgamation without any issues. Any way to get an idea of what changed on your end since? Obviously, something could have changed on the Gecko side too in the last 2 weeks.
Flags: needinfo?(drh)
Complete list of changes here:

   https://www.sqlite.org/src/timeline?from=2015-07-14&to=release

I don't see any suspicious right away, but I'll dig deeper.

Usage hint:  click on any two of the check-in circles to get a diff between those two check-ins.
Flags: needinfo?(drh)
We're going to try bisecting this on the SQLite side against a common Gecko first.
From the log: WARNING: The SQL statement 'SELECT name FROM database' could not be compiled due to an error: database is locked: file /builds/slave/try-m64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1148
It appears that the problem originates with SQLite check-in https://www.sqlite.org/src/info/b79a4affe44bd0c8 - We will try to figure out what is wrong on our end.  In the meantime, I suppose FF should stick to using 3.8.10.2....
Just a guess, but the following might fix the problem:

--- a/storage/mozStorageConnection.cpp	Tue Jul 28 17:57:43 2015 -0700
+++ b/storage/mozStorageConnection.cpp	Wed Jul 29 07:14:23 2015 -0400
@@ -753,18 +753,16 @@
   nsAutoCString pageSizeQuery(MOZ_STORAGE_UNIQUIFY_QUERY_STR
                               "PRAGMA page_size = ");
   pageSizeQuery.AppendInt(pageSize);
   nsresult rv = ExecuteSimpleSQL(pageSizeQuery);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // Setting the cache_size forces the database open, verifying if it is valid
-  // or corrupt.  So this is executed regardless it being actually needed.
-  // The cache_size is calculated from the actual page_size, to save memory.
+  // Reading the cache_size forces the database open, verifying if it is valid
+  // or corrupt.
   nsAutoCString cacheSizeQuery(MOZ_STORAGE_UNIQUIFY_QUERY_STR
-                               "PRAGMA cache_size = ");
-  cacheSizeQuery.AppendInt(-MAX_CACHE_SIZE_KIBIBYTES);
+                               "PRAGMA cache_size");
   int srv = executeSql(mDBConn, cacheSizeQuery.get());
   if (srv != SQLITE_OK) {
     ::sqlite3_close(mDBConn);
     mDBConn = nullptr;
     return convertResultCode(srv);
   }

The fact that "PRAGMA cache_size=N" would (historically) cause the database schema to be read and parsed was undefined and unsupported behavior.  And that behavior changed in the https://www.sqlite.org/src/info/b79a4affe44bd0c8 check-in of SQLite (for important technical reasons which I will be happy to go into if someone asks but are not really relevant to this discussion).  Invoking "PRAGMA cache_size" to query the cache size but not change it still does read and parse the schema.  And, yes, that behavior is still undefined and unsupported, but I can change it to be defined and supported behavior if this turns out to fix the problem.
So basically, the error was catched by the PRAGMA before, while now it's catched by the first query we run.
We still need to set the cache_size though.

What we were doing before relying on cache_size, was a "SELECT * FROM sqlite_master" to check the connection was "sane" (force initing schema and checking the connection is usable), but anything cheaper is welcome.
Please consider my patch above as an "is this the problem?" test case.  If my patch really does fix it, we can probably figure out a way to change SQLite back to exhibit its old behavior (after which we will document the behavior to prevent it from changing again).  And so no changes to FF would be required.
Today is not my day, I mistakenly canceled Ryan's builds and had to retrigger them.
In addition I repushed my alternative "go back" patch along with the Sqlite upgrade.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1b3ea4c387f

(In reply to D. Richard Hipp from comment #12)
>  And so no changes to FF would be required.

We just need a cheap way to check the connection is working and the schema can be parsed, it doesn't have to be setting cache_size, changing this code is not a big deal if needed.
If FF was depending on "PRAGMA cache_size=N" to force a schema parse, then maybe some of the other million or so applications using SQLite do the same.  So it might be best to restore that behavior to SQLite.

I have one-line patch for SQLite 3.8.11 at https://www.sqlite.org/tmp/sqlite-3.8.11-patch1.zip that might do the trick.  If you try out that changes and it works in FF, then I'll run it through testing cycle and release it as SQLite 3.8.11.1 - and then document the formerly undocumented behavior of PRAGMA cache_size so that it won't change again.
(In reply to D. Richard Hipp from comment #14)
> I have one-line patch for SQLite 3.8.11 at
> https://www.sqlite.org/tmp/sqlite-3.8.11-patch1.zip that might do the trick.

Sorry, it returns NOT FOUND error.
SQLite version 3.8.11.1 containing the one-line patch to restore the undocumented side-effects of PRAGMA cache_size is currently being tested and should be released within 24 hours.
Thank you, the tests look good on our side.
Attachment #8640086 - Attachment is obsolete: true
Attachment #8640086 - Flags: review?(mak77)
Attachment #8640452 - Attachment is obsolete: true
Alias: SQLite3.8.11 → SQLite3.8.11.1
Summary: Upgrade to SQLite 3.8.11 → Upgrade to SQLite 3.8.11.1
Try's looking much better on this one :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=227abf1b4c53
Attachment #8640821 - Flags: review?(mak77)
Comment on attachment 8640821 [details] [diff] [review]
Upgrade to SQLite 3.8.11.1

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

Thanks to everyone involved, I'm excited to see so many micro optimizations.

Ryan, please remember to fix the commit message.
Attachment #8640821 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #22)
> Comment on attachment 8640821 [details] [diff] [review]
> Upgrade to SQLite 3.8.11.1
> 
> Review of attachment 8640821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks to everyone involved, I'm excited to see so many micro optimizations.
> 
> Ryan, please remember to fix the commit message.

Should we expect this to get pulled into 41?
(In reply to Arthur K. from comment #24)
> Should we expect this to get pulled into 41?

There's no reason to rush an uplift afaict, this can ride the train as usual, so it gets full testing.
https://hg.mozilla.org/mozilla-central/rev/59e6b29d756a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: SQLite3.9.1
You need to log in before you can comment on or make changes to this bug.