Python 2.7.5 sqlite generates SQL that deviates from standard #22134

Closed
ptone opened this Issue Aug 27, 2013 · 28 comments

Projects

None yet

8 participants

@ptone
ptone commented Aug 27, 2013

https://code.djangoproject.com/ticket/20963

I'm not sure what the root of the issue is - but have verified that the issue Django has with this test is not present on 2.7.5 on Ubuntu, or the Mac version from python.org

@adamv
Contributor
adamv commented Aug 27, 2013

What about the system Python? Which version of OS X? Which version of SQLite?

@ptone
ptone commented Aug 27, 2013

system 2.7.3 on 10.8 does not have this issue, basically no other binary of Python that I tried other than 2.7.5 brew python

@mistydemeo
Contributor

system 2.7.3 is compiled against a different sqlite - maybe it's our sqlite, not our python? We package 3.7.17 with no patches, have you tested other pythons compiled against the same version?

@ptone
ptone commented Aug 27, 2013

@mistydemeo that is a good idea, I only tested against python installs on 2.7.5 on ubuntu, and on OS X as installed from python.org's binary installer. I will see if I can find some time to test that combo.

@ptone
ptone commented Aug 27, 2013

this is the config on my dev laptop where I've reproduced it - note that I know there are some errors, but I've also reproduced this on a couple clean machines running 10.6.8 - I can add those gists when I get back to the location where those machines are located.

https://gist.github.com/ptone/89802b0271ad9c2ff77b

@akaariai

The problem is very likely SQLite. The generated query has an INNER JOIN that removes rows it shouldn't. Changing the join type to LEFT OUTER JOIN keeps the rows, but LEFT JOIN shouldn't be needed as there are matching rows in the joined table. The test passes both on PostgreSQL and MySQL using Brew Python.

The summary of this issue isn't exactly correct, the SQL generated is correct, SQLite just doesn't execute it correctly.

@samueljohn
Contributor

@akaariai thanks for your input!
If this is a SQLite bug, has this been reported to SQLite, already? Or does any of our patches introduce this?

Perhaps we should upgrade sqlite to 3.8.0. Would that resolve this issue?

I am trying to reproduce ... does anyone have a python three-liner for me?

@samueljohn
Contributor

I can reproduce this.

@samueljohn
Contributor

So updated to SQLite 3.8.0 now and now test/runtest.py in django does no longer show that queries.tests.Queries1Tests error.

@samueljohn samueljohn was assigned Aug 27, 2013
@samueljohn samueljohn added a commit that closed this issue Aug 27, 2013
@samueljohn Stefan + samueljohn sqlite 3.8.0
Fixed version for extension_functions.c to match
what is on the official homepage. The sha1 remains
the same.

Closes #22132
Fixes #22134

Signed-off-by: Samuel John <github@SamuelJohn.de>
08de5da
@samueljohn
Contributor

Would you please test by brew update, brew upgrade sqlite, brew reinstall python and then running the django tests again?

@ptone
ptone commented Aug 27, 2013

Yes - I can verify that this fixes the issue - thanks for the super quick response to this.

@samueljohn
Contributor

Glad to hear and thanks for reporting.

@adamv
Contributor
adamv commented Aug 27, 2013

Would be interesting to know which point release of SQLite introduced the error, but glad this is working now.

@yarko
yarko commented Sep 4, 2013

I'm not sure this is an SQLite issue - and brew reinstalling python is a sort of nasty, upside-down fix.
Here's what I have installed:

python: stable 2.7.5, HEAD
http://www.python.org
/usr/local/Cellar/python/2.7.5 (5200 files, 80M) *
  Built from source
From: https://github.com/mxcl/homebrew/commits/master/Library/Formula/python.rb

sqlite: stable 3.8.0.1
http://sqlite.org/

This formula is keg-only.

Here's what I think the issue is:

When running from a virtual environment (I think the app is incidental, as I believe this is general):

    from _sqlite3 import *
ImportError: dlopen(/Users/yak007/workspace/nikola/Vnik/lib/python2.7/lib-dynload/_sqlite3.so, 2): Library not loaded: /usr/local/opt/sqlite/lib/libsqlite3.0.8.6.dylib
  Referenced from: /Users/yak007/workspace/nikola/Vnik/lib/python2.7/lib-dynload/_sqlite3.so
  Reason: image not found

(be sure to scroll right to see the full ImporError message)

And here's what is in /usr/local/opt/... :

$ ls -l /usr/local/opt/sqlite/lib/
total 3136
-r--r--r--  1 yak007  wheel  696808 Sep  3 20:42 libsqlite3.0.dylib
-r--r--r--  1 yak007  wheel  899312 Sep  3 20:42 libsqlite3.a
lrwxr-xr-x  1 yak007  wheel      18 Sep  3 20:42 libsqlite3.dylib -> libsqlite3.0.dylib
drwxr-xr-x  3 yak007  wheel     102 Sep  3 20:42 pkgconfig

WIthin /usr/local/opt/sqlite/lib/, a simple link of the "expected" (but detail irrelevant) dylib as such:

$ ln -s libsqlite3.0.dylib libsqlite3.0.8.6.dylib

also solves this without any re-installation, so it's clear all the python re-install is doing is linking with the current sqlite library.

I would expect, then (given the current sqlite library version numbers available to link to for python) that doing this once should solve it for the future too.

Or, you could continue to symlink-in the monkeypatch, as I've described.

Indeed, I just reinstalled python, and removed the monkeypatch/symlink, and all still works (and I expect it will continue to).

Does this help clarify that what is going on is in the python linking, and at one point, sqlite3 library dylib versioning got too specific (hint: 3.0.8.6)?

Regards,
Y.

@samueljohn
Contributor

Thanks @yarko that was very precise. However, the issue here had not directly to do with linking SQLite but with a version of SQLite that behaved strange (see the django test case).

Also note, that on my system Python's _sqlite.so seems to link the general libsqlite3.0.dylib and not to a very specific version. But then again, this is the current state and it seems fine (perhaps older SQLite did build that specific version 2.0.8.6).

xcrun otool -L opt/python/Frameworks/Python.framework/Versions/Current/lib/python2.7/lib-dynload/_sqlite3.so
opt/python/Frameworks/Python.framework/Versions/Current/lib/python2.7/lib-dynload/_sqlite3.so:
    /homebrew/opt/sqlite/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 169.3.0)

(I have my testing homebrew at /homebrew but that should not matter here)

Anybody willing to run that otool -L on his brewed Python's _sqlite.so, too?

@yarko
yarko commented Sep 4, 2013

Ah, yes @samueljohn - I see...

Well, this is my output, after reinstalling python:

$ xcrun otool -L /usr/local/opt/python/Frameworks/Python.framework/Versions/Current/lib/python2.7/lib-dynload/_sqlite3.so 
/usr/local/opt/python/Frameworks/Python.framework/Versions/Current/lib/python2.7/lib-dynload/_sqlite3.so:
    /usr/local/opt/sqlite/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 169.3.0)
@samueljohn
Contributor

Looks good. I think we don't see this breaking again for the next sqlite update.

@MikeMcQuaid
Member

@samueljohn We should really bottle the dependencies of the things we bottle to avoid this type of thing or, maybe even one day, include all dependencies in a bottle.

@samueljohn
Contributor

@mikemcquaid but neither sqlite nor python was bottles (so far). Therefore, I think this issue is unrelated. I don't know how the sqlite did not have the libsqlite3.0.dylib and why python linked against some crazy libsqlite3.0.x.y.dylib.
If linked to the "3.0" things should work with and without bottles and with mixing them, I guess.

@manphiz
Contributor
manphiz commented Sep 4, 2013

@samueljohn Actually this is how shared library versioning works, though I forgot the details. Basically both libsqlite3.dylib and libsqlite3.0.dylib used to be symlinks to the most versioned one libsqlite3.0.8.6.dylib, and when exported symbols changed, the version changed accordingly. This way, when sqlite3 updates, as long as the older libsqlite3.0.8.6.dylib stays there, other binaries or libraries dynamically linking to it will continue to work before they get rebuilt against new versions. Since homebrew works like a sandbox-like fashion, this is not practical.

IMHO when a package upgrades, homebrew should check its reverse dependencies and rebuild when necessary, which is what gentoo linux has been doing for some years. Or we could use @jacknagel's package versioning to force broken reverse dependencies to upgrade, and generate new bottles.

@samueljohn
Contributor

@manphiz I just did the update 3.8.0.1 to 3.8.0.2 of sqlite and nothing broke.
See my log https://gist.github.com/samueljohn/6447044

After the update, I still have:

tree opt/sqlite/lib/                   
opt/sqlite/lib/
├── libsqlite3.0.dylib
├── libsqlite3.a
├── libsqlite3.dylib -> libsqlite3.0.dylib
├── libsqlitefunctions.dylib
└── pkgconfig
    └── sqlite3.pc
@manphiz
Contributor
manphiz commented Sep 5, 2013

@samueljohn That means the (exported) symbols between these 2 versions have not changed. That usually happens (and may not) between major releases, for example in sqlite case, 3.7.x -> 3.8.x.

Some reference: http://tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html

@manphiz
Contributor
manphiz commented Sep 5, 2013

In my last message, I meant library version changes usually between major releases.

@samueljohn
Contributor

Sure. My point is more that it now looks as until 3.1 is out we won't have issues as there is currently no 3.0.x.y

@yarko
yarko commented Sep 5, 2013

Even with 3.1 out you are unlikely to have issues. The only symbols that
should matter in dynamic linking are the ones that python references.
On Sep 5, 2013 2:54 AM, "Samuel John" notifications@github.com wrote:

Sure. My point is more that it now looks as until 3.1 is out we won't have
issues as there is currently no 3.0.x.y


Reply to this email directly or view it on GitHubhttps://github.com/mxcl/homebrew/issues/22134#issuecomment-23849405
.

@samueljohn
Contributor

symbols yes, but the path to the libsqlite3.0.dylib is "stored" in the _sqlite.so of python, right?

@yarko
yarko commented Sep 5, 2013

I'm not sure, but certainly directly or indirectly.
You are correct that if the symlink in the lib directory were:
libsqlite3.0.dylib -> libsqlite3.dylib
instead of the other way around, this would be more convenient.

As it is, the "3.0" dylib is still there for sqlite 3.7.12 - a pretty fair
indicator of the mistake of the direction of the symlink, as I point out.
Someone decided it is now just easier to keep libsqlite3.0.dylib,
regardless.

Regards,

  • Yarko

On Thu, Sep 5, 2013 at 10:32 AM, Samuel John notifications@github.comwrote:

symbols yes, but the path to the libsqlite3.0.dylib is "stored" in the
_sqlite.so of python, right?


Reply to this email directly or view it on GitHubhttps://github.com/mxcl/homebrew/issues/22134#issuecomment-23876606
.

@manphiz
Contributor
manphiz commented Sep 5, 2013

@yarko Note this is the way shared library works. It uses version to track library compatibility. It is correct behavior to link to the most version specific dylib.

@handyman5 handyman5 pushed a commit to handyman5/homebrew that referenced this issue Oct 7, 2013
@samueljohn Stefan + samueljohn sqlite 3.8.0
Fixed version for extension_functions.c to match
what is on the official homepage. The sha1 remains
the same.

Closes #22132
Fixes #22134

Signed-off-by: Samuel John <github@SamuelJohn.de>
9f20803
@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.