Bug 166319

Summary: sqlite should be built with --enable-threadsafe
Product: [Fedora] Fedora Reporter: Nicholas Miell <nmiell>
Component: sqliteAssignee: Paul Nasrat <nobody+pnasrat>
Status: CLOSED RAWHIDE QA Contact:
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bojan, ivazqueznet, jeff
Target Milestone: ---Keywords: Patch
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 3.2.8-1 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-01-24 20:26:06 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 177422    
Attachments:
Description Flags
Patch sqlite.spec
none
Remove two-thread spinup in threading implementation test none

Description Nicholas Miell 2005-08-19 04:44:12 UTC
If sqlite isn't configured with --enable-threadsafe, threaded apps which access
the same sqlite database from multiples threads will corrupt the db.

(POSIX advisory file locks are completely useless in multi-threaded apps and
--enable-threadsafe turns on the internal workarounds to deal with this cruft.)

Comment 1 Nicholas Miell 2005-08-20 18:25:40 UTC
Created attachment 117948 [details]
Patch sqlite.spec

Comment 2 Jeffrey C. Ollie 2005-10-19 18:06:18 UTC
I'd like to second Nicholas' motion.  OpenPBX is replacing the internal copy of
DB1 that it inherited from Asterisk with SQLite, but OpenPBX needs a threadsafe
version of SQLite.

Comment 3 Bojan Smojver 2006-01-21 09:47:03 UTC
Yes, please. If we could have that in FC5, it would be great.

Devel list thread:

http://www.redhat.com/archives/fedora-devel-list/2006-January/msg01028.html

Comment 4 Bojan Smojver 2006-01-22 08:25:38 UTC
BTW, can we also see a bump to 3.2.8 at the same time. It's been out since 19
Dec 2005.

Comment 5 Bojan Smojver 2006-01-22 21:41:42 UTC
Created attachment 123549 [details]
Remove two-thread spinup in threading implementation test

SQLite tests if threads can overwrite each others locks by spinning up two
threads every time the library is initialized (in fact, on the first DB file
open). Since we know how is this going to end on Fedora (bar any arch
differences I didn't test for), we should set this value to 1 and be done with
it. Saves spinning up 2 threads.

I tested this with current Rawhide on an i686.

Comment 6 Bojan Smojver 2006-01-22 21:47:23 UTC
I briefly and completely unscientifically tested threaded v. non-threaded SQLite
on by box, when running one thread of execution (i.e. the case we would care
about if thread safetly interfered with performance). The test involved
inserting 3000 records into a table. I did not observe any noticeable slowdown
with the threading implementation.

Thread safe (--enable-threadsafe):

real    1m27.286s
user    0m4.244s
sys     0m23.841s

Thread unsafe:

real    1m27.571s
user    0m4.480s
sys     0m24.334s

Basically, SQLite simply creates a pthread_mutex and acquires a lock on it
before doing things with a file. With NPTL, this should not be a huge problem.

Comment 7 Bojan Smojver 2006-01-22 22:16:58 UTC
One more thing - can you also bump sqlite2 to 2.8.17? Fixes the same database
corruption 3.2.8 does.

Comment 8 Bojan Smojver 2006-01-22 23:38:40 UTC
This bug may be somewhat related to bug #177422.

Comment 9 Alexander Larsson 2006-01-23 08:18:50 UTC
This is also important for beagle/f-spot if we switch them to sqlite3.
I'm doing the sqlite2 update to 2.8.17.

Comment 10 Bojan Smojver 2006-01-23 09:14:12 UTC
Thanks Alex.

BTW, did you get my note from bug #177422 about the actual beagle situation - it
looks like only the alpha version 3.3.1 addresses this properly (i.e. use of
connections opened in one thread by another thread). I don't have much SQLite
experience, but it appears that current stable code checks the TID and refuses
to use the connection structure if it didn't originate in that thread...

Comment 11 Bojan Smojver 2006-01-24 10:16:38 UTC
FYI, 3.3.2 beta is out.

Comment 12 Paul Nasrat 2006-01-24 20:26:06 UTC
Sorry for delay - bumped to 3.2.8 and added thread safe.

Comment 13 Bojan Smojver 2006-01-24 23:36:57 UTC
What did you think of the "save two thread spinup" patch? Too risky?

Comment 14 Paul Nasrat 2006-01-25 00:34:43 UTC
Have you tried pushing that patch upstream at all? If so please link to the
relevent mailing list discussion. The general rule in Fedora is to try and get
things upstream.

Comment 15 Bojan Smojver 2006-01-25 01:12:51 UTC
No, but I think it wouldn't get accepted even if I tried in its current form.
This is a Fedora specific thing where we know things will be certain way at
runtime. SQLite, being a cross platform thing, probably would prefer to check
what's actually going on at runtime in its default build (i.e. the binary SQLite
folks ship may not know if it's going to be on a platform that supports this or
not).

A proper upstream fix would probably be something along the lines of:

- have a configure option for this (i.e. explicitly choose certain behaviour)
- have a configure test for this (i.e. determine things at build time, rather
than runtime)

No biggie. If I find time, I may propose one of the above upstream. I just
wanted to make Fedora SQLite a tad faster by not spinning up two unnecessary
threads, which, if you use SQLite from Apache for instance, ends up being two
threads in each preforked process. I know that NPTL is very efficient and all,
but...

Comment 16 Bojan Smojver 2006-01-31 19:03:36 UTC
I contacted the author of SQLite, D. Richard Hipp, and he doesn't mind the
patches along the lines of what I discussed in commend #15. In fact, he will
patch os_unix.c file himself and he asked me to work on the patch for configure.ac.

So, there is a good chance that we'll get --enable-threads-override-locks or
something to disable two thread spinup on startup.

Comment 17 Bojan Smojver 2006-02-01 02:00:26 UTC
The SQLite CVS now has --enable-threads-override-locks, which will do what I
talked about in comment #5. Hopefully, we'll see 3.3.4 released before FC5...