Psycopg psycopg

Comments and changes to this ticket

• 

  Daniele Varrazzo May 3rd, 2011 @ 11:35 PM

  • Tag set to “adapt, decimal”

  The same bug was reported for mod_python a few years ago and was fixed, so I don't know why it's bugging again - or it is something similar.

  The problem is that the decimal module may be reloaded but there is no way to notify that to psycopg. Psycopg has a mapping that says how to adapt every class, but if the class gets redefined it gets a new identity. A test isinstance(obj, Decimal) fails if obj is a decimal created before reloading: the problem psycopg has is probably similar. Why decimal gets reloaded is something I don't know: it's likely to be related to having many processes. Try to trace id(Decimal) and see when it changes: it may give you a clue of why it does.

  A way to force the problem (but this is not the same as reproducing the bug: this is totally expected):

  In [4]: import decimal
  
  In [7]: psycopg2.extensions.adapt(decimal.Decimal('0')).getquoted()
  Out[7]: '0'
  
  In [8]: reload(decimal)
  Out[8]: <module 'decimal' from '/usr/lib/python2.6/decimal.pyc'>
  
  In [9]: psycopg2.extensions.adapt(decimal.Decimal('0')).getquoted()
  ---------------------------------------------------------------------------
  ProgrammingError                          Traceback (most recent call last)
  
  /home/piro/dev/psycopg2/build/lib.2.6/<ipython console> in <module>()
  
  ProgrammingError: can't adapt type 'Decimal'
  

  From this condition, a way to fix the problem is to register again the adapter, that will bind to the newly defined class:

  In [15]: psycopg2.extensions.register_adapter(decimal.Decimal, psycopg2._psycopg.Decimal)
  
  In [16]: psycopg2.extensions.adapt(decimal.Decimal('0')).getquoted()
  Out[16]: '0'
  

  The original problem was related to the subinterpreters (mis)feature. This was the fix. I don't believe Graham is doing this stuff anymore though, so I don't think it's really the same bug.

  I suggest you to detect the bug and work around it using the above technique, but better if you trace the evolution of the Decimal class' id. If you come with something that deserves being fixed in Psycopg we will do it. I'm leaving the bug open for now.

• You flagged this item as spam.
  

  Marti Raudsepp May 4th, 2011 @ 03:26 PM

  Interesting. I tried this reload() case with pg8000 and PyGreSQL, both also fail with a similar error.

  Upon closer inspection, turns out that this has only occured on one of our testing servers and not during the last 2 weeks. It's still not clear how its environment differs from all the other servers.

• 

  Daniele Varrazzo May 13th, 2011 @ 11:30 AM

  • State changed from “new” to “invalid”

  Closing the ticket, but please keep us informed in case you manage to reproduce it consistently. Thank you!

• You flagged this item as spam.
  

  Marti Raudsepp October 10th, 2011 @ 05:28 PM

  I have traced down the cause and attached a reproducible test case. Please re-open this bug.

  A few weeks ago I was re-reading the mod_wsgi documentation and noticed this: https://code.google.com/p/modwsgi/wiki/ConfigurationDirectives#WSGI...

  Default: WSGIApplicationGroup %{RESOURCE}

  The application group name will be set to the server hostname and port as for the %{SERVER} variable, to which the value of WSGI environment variable SCRIPT_NAME is appended separated by the file separator character

  The effect of using the %{RESOURCE} variable expansion is for each application on any server to be isolated from all others by being mapped to its own Python sub interpreter.

  So apparently, if you serve your application(s) on different VirtualHosts/port numbers/paths, each will get a separate sub-interpreter. I didn't realize this before, but I had configured the app to run on 2 different ports, one of which was used very rarely, hence why the problem occurred so sporadically.

  The workaround is to make sure there's only one interpreter. The problem hasn't occured for ~2 weeks now, after I made this addition:

  WSGIApplicationGroup %{GLOBAL}

  ---

  So, I wrote a testcase and tried it with psycopg2, PyGreSQL, pg8000 and MySQLdb adapters. The only adapter that failed this test is psycopg2.

  Tested on Ubuntu 10.10 with psycopg2 installed via pip.
  psycopg2.version is '2.4.2 (dt dec pq3 ext)'

  ---

  The attached testcase is a WSGI script with installation instructions for Apache+mod_wsgi. After configuring it, access http://127.0.0.1/foo/ and then http://127.0.0.1/bar/ after it. With psycopg2, the latter access will give you an error traceback.

  Uncomment the lines in the script to test other adapters.

  • decimalwsgi.py 1.7 KB
• 

  Daniele Varrazzo October 10th, 2011 @ 05:40 PM

  • State changed from “invalid” to “new”

  I thought the apache guy had learned to not mess up with sub-interpreter. They are a bloody implementation detail, crap. Isn't there any deploy method not involving multiple sub-interpreters per process? I thought there was.

  Putting the ticket state to new, I'll review your test case. I can't guarantee we are going to fix it though: sub-interpreters and C extensions are a toxic combination.

• You flagged this item as spam.
  

  Marti Raudsepp October 12th, 2011 @ 04:10 PM

  • Tag changed from “adapt, decimal” to “adapt, decimal, django, mod_wsgi”

  Are you working on a fix for this, or should I give it a shot myself?

  Unfortunately, I think we'll have to live with subinterpreters. Changing the default WSGIApplicationGroup now would open up lots of compatibility headaches. And even then, it would be nice for psycopg2 to be able to run in any mod_wsgi configuration.

  The fact that this works out of the box with MySQLdb, but can fail with psycopg2, can also shine a bad light on PostgreSQL.

• 

  Daniele Varrazzo October 12th, 2011 @ 04:38 PM

  I am planning to work at it, but don't have anything in the calendar. 2.4.3 is already to be released, waiting for fog, but I don't mind including a fix for it if we can sneak it in.

  If you are able to do it yourself we would be happy to accept the patch of course. Just warn me if you try that so we don't do twice the work.

  If the fix also includes a test case, either involving compiling a C program spawning subints to reproduce the bug condition, or calling into the libpython via ctypes and thrashing a script process, it would be awesome.

• 

  Daniele Varrazzo October 13th, 2011 @ 05:58 PM

  • State changed from “new” to “open”

  I've taken a look at the issue. Thank you for the test case: it worked fine and allowed me to debug a little bit.

  About the above approach of testing using ctypes: failed. The interpreter bombs with

  Fatal Python error: PyThreadState_Get: no current thread
  

  Quick look around and found this: http://docs.python.org/c-api/init.html#bugs-and-caveats "Furthermore, extensions (such as ctypes) using these APIs to allow calling of Python code from non-Python created threads will probably be broken when using sub-interpreters". I'm out of polite word to describe the idea of using sub-interpreters.

  After venting and having instrumented psycopg with inspection functions, I can see that it works "as expected": the function psyco_GetDecimalType returns a different result for any subinterpreter. This means that this scenario is different from the one we put the guard in production, that was related to mod_python. In what is different I can't properly say.

  The problem in itself is that the adapters map only contains the first decimal. Because of this, the easy fix is to add to your script at import time:

  psycopg2.extensions.register_adapter(decimal.Decimal, psycopg2._psycopg.Decimal)
  

  so that all the sub-interpreters will have their own adapter.

  I was going to write "I doubt it is possible to fix it in psycopg as the c module is imported only once..." but actually the psycopg2 module itself (i mean __init__.py) is imported once per subinterpreter. I think this sorts everything out: I've tested and adding the above recipe to __init__.py fixes the problem. Not only that: we may be able to drop a few of the crufts we had to implement in the C library to fight the problem.

  A preliminary patch is the following:

  diff --git a/lib/__init__.py b/lib/__init__.py
  index 3831210..3ca2fe7 100644
  --- a/lib/__init__.py
  +++ b/lib/__init__.py
  @@ -52,6 +52,7 @@ if sys.version_info >= (2, 3):
   if sys.version_info >= (2, 4):
       try:
           import decimal as _psycopg_needs_decimal
  +        from psycopg2._psycopg import Decimal as _DecimalAdapter
       except:
           warnings.warn(
               "can't import decimal module probably needed by _psycopg",
  @@ -85,5 +86,14 @@ import psycopg2.extensions as _ext
   _ext.register_adapter(tuple, _ext.SQL_IN)
   _ext.register_adapter(type(None), _ext.NoneAdapter)
  
  +# Register the Decimal adapter again. This allows ...
  +try:
  +    from decimal import Decimal
  +except ImportError:
  +    pass
  +else:
  +    _ext.register_adapter(Decimal, _DecimalAdapter)
  +    del Decimal
  +
   __all__ = filter(lambda k: not k.startswith('_'), locals().keys())
  

  I will see if I can clean up further the module after this (when we made the previous solution there wasn't any registration in the python layer, but now we already have a couple). The problem seems fixable though.

• You flagged this item as spam.
  

  Marti Raudsepp October 13th, 2011 @ 06:03 PM

  Cool. Are there any other built-in adapters we have to worry about?

  Have you checked what PyGreSQL and MySQLdb are doing to solve this problem?

• 

  Daniele Varrazzo October 13th, 2011 @ 06:08 PM

  After a quick review, the psyco_GetDecimalType is still required for the other half of the story: in order to return from Postgres to Python a Decimal type whose class identity is the one of the requesting sub-interpreter. So there's probably not so much dead code to cut out of the library. I'm undecided whether to leave the Decimal registration both in C and Python (redundant) or to leave it only in Python.

• 

  Daniele Varrazzo October 13th, 2011 @ 06:11 PM

  Don't know about the other adapter, how do they do adaptation and all. Probably they have always done everything in Python and so ditched the problem.

• 

  Daniele Varrazzo October 13th, 2011 @ 06:15 PM

  About the other types: I think no: Decimal is the only pure python module imported and used by psycopg in the C layer, all the other builtins are python C types.

  In [5]: [k[0] for k in psycopg2.extensions.adapters.keys()]
  Out[5]: 
  [<type 'float'>,
   <type 'str'>,
   <type 'long'>,
   <type 'int'>,
   <type 'NoneType'>,
   <type 'bytearray'>,
   <type 'datetime.date'>,
   <type 'list'>,
   <type 'bool'>,
   <type 'buffer'>,
   <type 'datetime.datetime'>,
   <type 'unicode'>,
   <class 'decimal.Decimal'>,
   <type 'datetime.timedelta'>,
   <type 'datetime.time'>,
   <type 'tuple'>]
  

• 

  Federico Di Gregorio October 14th, 2011 @ 07:19 AM

  I'd vote for moving the Decimal adapter registration to Python.
  Nobody uses the C module alone anyway.

• 

  Daniele Varrazzo October 14th, 2011 @ 09:40 PM

  • State changed from “open” to “resolved”

  Committed.