zopefoundation / zope.sqlalchemy

Integration of SQLAlchemy with transaction management
Other
32 stars 34 forks source link

Enable savepints for all engines. Add disable_savepoints param #45

Closed lrowe closed 9 months ago

lrowe commented 4 years ago

Fixes #44. Looking for feedback but requires documentation before merging.

From my Travis tests it appears Python 3.6 was the first to ship with a SQLite version that supports savepoints. (It's possible to hack in support using pysqlite for earlier versions using: https://github.com/lrowe/pysqlite/wiki/Building-pysqlite-with-static-env-and-savepoints.)

Should this be considered a breaking change? Potentially those running older versions of SQLite may now need to add disable_savepoints=True to their register call.

icemac commented 4 years ago

I like this PR although I am not using this library against SQLite on Python < 3.6. I'd consider this a breaking change as we should not yet drop support for Python < 3.6.

lrowe commented 4 years ago

@mgedmin transaction.savepoint() without optimistic=True raises a TypeError when one of its data managers is missing a savepoint method. To get that same behaviour you would need to use zope.sqlalchemy.register(disable_savepoints=True).

If not and your database/driver does not support subtransactions you get a sqlalchemy.exc.OperationalError when the database errors on the transaction.commit() or .rollback() having called transaction.savepoint() in the transaction. Example test failures before I added the version based unittest.skipIf: https://travis-ci.org/github/zopefoundation/zope.sqlalchemy/jobs/668966939

I'm reluctant to try and make a decision automagically as the logic required will likely be incomplete and error prone. "Explicit is better than implicit" and all that.

Python 2.7 and 3.5 sqlite who are using savepoints (likely optimistic ones) should either:

According to https://devguide.python.org/#status-of-python-branches Python 3.5 end-of-life is September this year. Perhaps it's enough to just clearly document this and call it out in the changelog?

icemac commented 4 years ago

I think documenting is enough.

dwt commented 4 years ago

What exactly is missing to get this pull request to continue? I just had to add an ugly workaround in my app because of this - and I would like to get rid of it soon. :-)

sallner commented 3 years ago

@dwt We need the following:

@lrowe Do you intend to work on that soon, or should someone else step in?

icemac commented 9 months ago

I am closing this old PR. Feel free to re-open it if you want to work on it.