710183 - (SQLite3.7.9) Upgrade to SQLite 3.7.9

Status: RESOLVED WONTFIX

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

Description

[Marco Bonardo [:mak]]

This is mostly for bugfixes, stability and some qyuery planner optimization:

http://www.sqlite.org/releaselog/3_7_8.html
http://www.sqlite.org/releaselog/3_7_9.html

This should also remove the need for the fsync workaround we used on 3.7.7.1 for Android

Considered the current status of trees and that it's one week before the merge, it's likely we'll take this on FF12

Comment 1

[D. Richard Hipp]

There is a memory-alignment bug in 3.7.9, fixed by this patch:  http://www.sqlite.org/src/ci/54cc119811?sbs=0  That bug only effects machines that require 64-bit alignment of large objecdts.  x86 and arm do not require that and so are unaffected.  This problem appears only on machines like sparc64 and mips.

We (the SQLite team) can package the fix into a branch release, 3.7.9.1, if you want us to.

Comment 2

[Marco Bonardo [:mak]]

Thanks for the notification, we'll figure this out and let you know.
At first glance, our tier1 platforms are x86/x86_64/armv7 so should be unaffected, but some tier2 ports may be affected. I'm not aware of all of them though.

Comment 3

[Marco Bonardo [:mak]]

to follow-up, I don't see issues in the list of supported build configs:
https://developer.mozilla.org/en/Supported_build_configurations

Though, to be sure, I sent a mail to Shawn to figure if he is aware of eventual other issues that rose in the past.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

sdwilsh - ping?
bent - Bug 661877 made a bunch of changes to test_quota.c, but I don't see a corresponding patch checked into the tree for future sqlite upgrades nor any comments in README.mozilla indicating these changes. I assume that the changes need to be ported to any future versions of the file?

Comment 5

[Marco Bonardo [:mak]]

Bug 661877 also introduced a test_quota.h fwiw, even if that's a minor issue on upgrades. We will likely have to backport upstream improvements to test_quota.c and then get explicit review on them related to indexedDB. Anything we will do needs better upgrade documentation than the current.

Comment 6

[Marco Bonardo [:mak]]

Ryan offered to take the bug for now, so dropping to him

Comment 7

[Ryan VanderMeulen [:RyanVM]]

It appears that the changes to test_quota.c in bug 661877 are from the latest version from sqlite tip. When I download the latest test_quota.c and test_quota.h and replace the in-tree versions with the downloaded versions, the hg diff shows no differences.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Per drh, it looks like we're good to go on the test_quota.[ch] issue.
------------------------------
Mixing the "tip" versions of test_quota.[ch] with SQLite version 3.7.x should work just fine.  The test_quota.[ch] files are an add-on we do specifically for Mozilla - it should be portable across most versions of the SQLite core.

The new test_quota.[ch] files will be in the next SQLite release, of course, but we are still a few weeks out on that.  We do not anticipate any changes to test_quota.[ch] between now and the 3.7.10 release.  The only reason test_quota.[ch] might change is if you find a bug.  ;-)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade to SQLite 3.7.9 - SQLite changes

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade to SQLite 3.7.9 - Mozilla changes

These are running on try now.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade to SQLite 3.7.9 - Mozilla changes

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ryan VanderMeulen from comment #4)
> bent - Bug 661877 made a bunch of changes to test_quota.c

Yeah, sorry, the changes to test_quota.* were to upgrade just those files to SQLite tip. The README.mozilla file was updated to include test_quota.h but otherwise no changes were needed.

Comment 13

[Mozilla RelEng Bot]

Try run for 68ac44f53a35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=68ac44f53a35
Results (out of 274 total builds):
    success: 244
    warnings: 25
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68ac44f53a35

Comment 14

[Mozilla RelEng Bot]

Try run for 68ac44f53a35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=68ac44f53a35
Results (out of 274 total builds):
    success: 244
    warnings: 25
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68ac44f53a35

Comment 15

[Mozilla RelEng Bot]

Try run for 68ac44f53a35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=68ac44f53a35
Results (out of 274 total builds):
    success: 244
    warnings: 25
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68ac44f53a35

Comment 16

[Mozilla RelEng Bot]

Try run for 68ac44f53a35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=68ac44f53a35
Results (out of 274 total builds):
    success: 244
    warnings: 25
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68ac44f53a35

Comment 17

[Mozilla RelEng Bot]

Try run for 68ac44f53a35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=68ac44f53a35
Results (out of 274 total builds):
    success: 244
    warnings: 25
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68ac44f53a35

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 585612 [details] [diff] [review]
Upgrade to SQLite 3.7.9 - Mozilla changes

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

just one missing change, apart that looks good.

Things to do before landing:
- verify the supported platforms issue (see comment 1 and 3). Eventually we may land and eventual third parties ports may just apply the small patch from the SQLite ticket to their local tree. None of our official tiers are affected.
- verify Talos numbers. Unfortunately compare-talos is completely broken atm, we may have to check manually, or land and wait for numbers before merging to central.

::: db/sqlite3/README.MOZILLA
@@ +2,2 @@
>  
>  -- Marco Bonardo <[e-mail]>, 08/2011

you should change this comment with your name, mail and current date

Comment 19

[Marco Bonardo [:mak]]

Since compare-talos has just been fixed, I did some empirical comparison of try values against inbound ones, there is some slight variation in ts_shutdown for the dirty profiles, but those are really noisy and known to be a bit bogus. Anything else appears in noise.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Attachment: Upgrade to SQLite 3.7.9 - Mozilla changes v2

Updated README.mozilla per mak's comments. I thought about adding a note about test_quota.c being newer than 3.7.9, but decided against it since future SQLite releases should get us back in sync anyway. If test_quota.[ch] get updated to something newer before the tree is updated to 3.7.10+, I would expect a comment to be added making note of it to avoid future pain.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

FYI, I've been running with these patches applied locally for the last week and haven't had any issues.

sdwilsh: ping?

Comment 22

[Marco Bonardo [:mak]]

I think we could land. The only issue left is support for platforms that are not part of any of the known tiers. Those platforms likely already have their own set of patches to apply to the codebase, the small change here shouldn't be that much of a problem then. In the remote possibility we figure out we missed someone, we have an officially released patch from the SQLite team that will be in 3.7.10 we could apply, that wouldn't hurt/complicate in any way our next upgrades.
Also considering the time left before the next merge, it's better to land earlier for longer testing, I hope Shawn will be fine with this plan.
In any case, backout is always a possibility.

Comment 23

[Marco Bonardo [:mak]]

I decided to proceed on baking the update in tree.
If anybody has concerns, please raise them to be evaluated.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb55a6e7c14
https://hg.mozilla.org/integration/mozilla-inbound/rev/6529bd9386cb

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Philor had some concerns, so I backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/f295de5da279
https://hg.mozilla.org/integration/mozilla-inbound/rev/9283d65116ac

Comment 25

[Marco Bonardo [:mak]]

so, the upgrade caused a crash on Win PGO

 0  mozglue.dll!je_free [jemalloc.c:6529bd9386cb : 6580 + 0xb]
    eip = 0x73633741   esp = 0x002dc65c   ebp = 0x00000000   ebx = 0x00000001
    esi = 0x0094e720   edi = 0x0473adc0   eax = 0x00000000   ecx = 0x00000001
    edx = 0x72751144   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  mozsqlite3.dll!sqlite3_bind_text + 0xc5
    eip = 0x72708fdc   esp = 0x002dc674   ebp = 0x002dc6a4
    Found by: call frame info with scanning
 2  xul.dll!mozilla::storage::`anonymous namespace'::variantToSQLiteT<mozilla::storage::`anonymous namespace'::BindingColumnData>(mozilla::storage::A0x372f0a2b::BindingColumnData,nsIVariant *) [variantToSQLiteT_impl.h:6529bd9386cb : 109 + 0x1b]
    eip = 0x6c165b94   esp = 0x002dc6ac   ebp = 0x002dc7e8
    Found by: previous frame's frame pointer
 3  xul.dll!mozilla::storage::BindingParams::bind(sqlite3_stmt *) [mozStorageBindingParams.cpp:6529bd9386cb : 254 + 0x10]
    eip = 0x6c165eef   esp = 0x002dc7f0   ebp = 0x00000010
    Found by: previous frame's frame pointer
 4  xul.dll!mozilla::storage::Statement::ExecuteStep(bool *) [mozStorageStatement.cpp:6529bd9386cb : 606 + 0x13]
    eip = 0x6c148f11   esp = 0x002dc808   ebp = 0x002dc830
    Found by: call frame info with scanning
 5  xul.dll!nsPermissionManager::UpdateDB(nsPermissionManager::OperationType,mozIStorageStatement *,__int64,nsACString_internal const &,nsACString_internal const &,unsigned int,unsigned int,__int64) [nsPermissionManager.cpp:6529bd9386cb : 1128 + 0xc]
    eip = 0x6c2aa4c9   esp = 0x002dc824   ebp = 0x002dc830
    Found by: call frame info with scanning
 6  xul.dll!nsPermissionManager::AddInternal(nsCString const &,nsCString const &,unsigned int,__int64,unsigned int,__int64,nsPermissionManager::NotifyOperationType,nsPermissionManager::DBOperationType) [nsPermissionManager.cpp:6529bd9386cb : 530 + 0x27]
    eip = 0x6c1cf293   esp = 0x002dc838   ebp = 0x002dc8b8

Comment 26

[Ryan VanderMeulen [:RyanVM]]

My MSVC2010 PGO builds have been running with it fine, so I'm willing to blame MSVC2005.

Comment 27

[D. Richard Hipp]

We haven't had any memory problems with SQLite 3.7.9 (except on Sparc, but that is another story and was quickly fixed).  On the other hand, SQLite has, at various times, tickled bugs in gcc, msvc, and llvm.  So the theory that VC2005 is to blame here is plausible.

FWIW, SQLite version 3.7.10 should be out probably within 24 hours.  And Version 3.7.10 will have support for "PRAGMA shrink_memory" and the new sqlite3_db_filename() interface, both of which we added specifically for FF.  So maybe you just want to wait a few days to pick up 3.7.10 and try again with it?

Comment 28

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

Just curious why we still use MSVC2005, if we from "some" time don't support old timed systems from M$ like 95,98&ME, only >2000 systems and >SSE2 CPUs, so should be better to use always the newest MSVC like MSVC2010SP1 ? It there any reason why we still use this old component. Thanks in advice

Comment 29

[Ed Morley [:emorley]]

The switch is most definitely wanted, but means dropping support for win 2000 entirely and service pack one and lower of win XP. Waiting on metrics to show how many users this will affect. See bug 563318 for more details.

Comment 30

[Marco Bonardo [:mak]]

(In reply to D. Richard Hipp from comment #27)
> FWIW, SQLite version 3.7.10 should be out probably within 24 hours.  And
> Version 3.7.10 will have support for "PRAGMA shrink_memory" and the new
> sqlite3_db_filename() interface, both of which we added specifically for FF.
> So maybe you just want to wait a few days to pick up 3.7.10 and try again
> with it?

Yes, at this point it's likely we'll just upgrade to 3.7.10, thank you.
Ryan, willing to file the new bug and update the patches as soon as it's released?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

I was just going to repurpose this bug, but I'm happy filing a new bug if you prefer. And I'll be sure to run PGO builds on the next try push :)

Comment 32

[Marco Bonardo [:mak]]

(In reply to Ryan VanderMeulen from comment #31)
> I was just going to repurpose this bug, but I'm happy filing a new bug if
> you prefer. And I'll be sure to run PGO builds on the next try push :)

a new bug is better, we will wontfix this one then.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Bug 718455 is filed for upgrading to 3.7.10.