1188578 - (SQLite3.8.11.1) Upgrade to SQLite 3.8.11.1

Status: RESOLVED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade to SQLite 3.8.11

https://treeherder.mozilla.org/#/jobs?repo=try&revision=348e580c09c3

Comment 2

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 3

[D. Richard Hipp]

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.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

We're going to try bisecting this on the SQLite side against a common Gecko first.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 6

[D. Richard Hipp]

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....

Comment 7

[D. Richard Hipp]

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.

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5ba68e4df8

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1

This reverts the check to the original one, we might not need all of this complication or we might find a cheaper query to run to obtain the same. But let's start by checking Try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=408144ca1f9b

Comment 11

[Marco Bonardo [:mak]]

sigh, I forgot to push the Sqlite upgrade with it :( let me kill the builds. We can use Ryan's try run btw, it should have the same outcome.

Comment 12

[D. Richard Hipp]

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.

Comment 13

[Marco Bonardo [:mak]]

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.

Comment 14

[D. Richard Hipp]

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.

Comment 15

[Marco Bonardo [:mak]]

(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.

Comment 16

[D. Richard Hipp]

Typo.  Should be:  https://www.sqlite.org/tmp/sqlite3-3.8.11-patch1.zip

Comment 17

[Marco Bonardo [:mak]]

Thank you, pushed to
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f0f98ead2a

Comment 18

[D. Richard Hipp]

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.

Comment 19

[Marco Bonardo [:mak]]

Thank you, the tests look good on our side.

Comment 20

[NVD]

https://www.sqlite.org/releaselog/3_8_11_1.html

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade to SQLite 3.8.11.1

Try's looking much better on this one :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=227abf1b4c53

Comment 22

[Marco Bonardo [:mak]]

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.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59e6b29d756a

Comment 24

[Arthur K. (he/him)]

(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?

Comment 25

[Marco Bonardo [:mak]]

(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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59e6b29d756a