Storm crashes on recent psycopg2

[antennen]

I installed Storm (0.19) and Psycopg2 (2.5) in a virtualenv. This causes the following issue:

hstorm.py:

from storm.locals import *

db = create_database("postgres://<redacted>")
store = Store(db)

When running I get this error.

(hstorm)ante@Melchior:~/dev/hstorm/hstorm$ python hstorm.py
Traceback (most recent call last):
  File "hstorm.py", line 3, in <module>
    db = create_database("postgres://<redacted>")
  File "/home/ante/dev/hstorm/local/lib/python2.7/site-packages/storm/database.py", line 463, in create_database
    None, None, [""])
  File "/home/ante/dev/hstorm/local/lib/python2.7/site-packages/storm/databases/postgres.py", line 54, in <module>
    install_exceptions(psycopg2)
  File "/home/ante/dev/hstorm/local/lib/python2.7/site-packages/storm/exceptions.py", line 145, in install_exceptions
    module_exception.__bases__ += (exception,)
TypeError: can't set attributes of built-in/extension type 'psycopg2.Error'

Downgrading to version 2.4.6 from 12th December 2012 rids me of this error. It seems 2.5 was released on 7th April 2013. Thank you for making this wonderful ORM!

[Markus Kemmerling (markus-kemmerling)]

As far as I can see there is a single error handler for storm.exceptions.Error in Connection.rollback(). As a workaround it should be fine to check for a general Exception on rollback instead (similar to commits) and to exclude storm.exceptions.Error from the sequence of errors set as base classes of the psycopg2 exceptions in install_exceptions().

[antennen (antennen)]

I've made a patch containing the mentioned workaround. I've successfully run the test-suite, with only 3 tests failing. I do believe these fail regardless of my changes. The failed tests are:

FAIL: test_result_set_count_sliced (tests.sqlobject.SQLObjectTest)
FAIL: test_result_set_count_sliced_end_none (tests.sqlobject.SQLObjectTest)
FAIL: test_find_slice_offset (tests.store.sqlite.SQLiteStoreTest)

I don't know if this is the preferred solution to this problem. If it isn't please tell me how to properly solve this.

This patch is for Storm 0.20.

[antennen (antennen)]

I forgot to add that I ran the default and Postgres test suite only, not the MySQL suite.

[Oscar Campos (oscar-campos)]

Hi guys.

I did a different workaround to fix this problem. We can't patch a built-in extension but we can patch a class that inherits from this built-in so I just catch the exception on the exceptions.py module, and create a subclass of psycopg2.Error that replaces this one so we can just patch the Error into its bases that becomes (psycopg2.Error, Error).

Is not necessary to modify the Connection.rollbak method. I ran the full PostgreSQL testsuite with no failures.

[Aurélien Bompard (abompard)]

I confirm I have this bug too, and that the patch fixes it for me.

[Gaudenz Steinlin (gaudenz-debian)]

I can confirm that this patch fixes the issue. I cleaned up the patch a bit to not contain any unneeded whitespace changes.

[Barry Warsaw (barry)]

I took a look at this. The patch seems reasonable on the face of it, but I'm not able to get a successful test run out of it. I suspect it's more a problem with my database setup than anything else though. I'll ping some folks internally to see if I can get someone else to review it.

[Gustavo Niemeyer (niemeyer)]

There's a problem of API compatibility here. The original point of the base exception is to enable people to catch storm.Error to deal with database exceptions. A few years down the road, I now see that this was a nice outcome, but an ill-conceived way to reach it which shouldn't have been used.

Regardless, we are here today. We need to think if we're breaking the API compatibility, if the workaround suggested works at all, or if there's any other workaround we can think of.

With the original behavior:

- psycopg2 code raises psycopg2.Error
- storm had injected storm.Error as a base of psycopg2.Error
- catching storm.Error catches psycopg2.Error

After this change:

- psycopg2 code raises psycopg2.Error
- storm had injected storm.Error as the base of PsycoPG25Error, which is a subclass of psycopg2.Error
- catching storm.Error does not catch psycopg2.Error

What the workaround attempts is to replace the exception name in the module namespace, but I doubt that will solve the problem, because psycopg2 is mostly in C, which means the module namespace is not really the reference used to raise the exception.

I'd be glad to be wrong here, though. Am I?

[Colin Watson (cjwatson)]

@niemeyer: I think you're right; in at least some places psycopg2 definitely creates its errors directly from C without fetching the class from the Python namespace.

I spent some time thinking about this today, because it would be worthwhile for Launchpad to start using PostgreSQL's json type (see https://code.launchpad.net/~cjwatson/launchpad/db-distroseries-publishing-options/+merge/278241), but psycopg2 only added support for adapting that in 2.5, which causes us to run into this bug. It occurred to me that we could use the isinstance/issubclass customisation added in Python 2.6 for abc.ABCMeta, and I later noticed https://bugs.launchpad.net/storm/+bug/1006711 which suggests the same thing. Could you review https://code.launchpad.net/~cjwatson/storm/psycopg-2.5/+merge/278330 which implements this?