teamniteo/pyramid_deferred_sqla

pyramid_deferred_sqla does not work for GET requests

Closed this issue ยท 8 comments

zupo commented

There is code in pyramid_deferred_sqla that marks the session as readonly. But I just found out that in WooCart project (the only current user of pyramid_deferred_sqla) this code is never being called due to how we have alembic set up.

In case one does not have the same alembic setup, the code breaks with the following error on GET requests:

  File "/work/pyramid-realworld-example-app/src/conduit/tag/views.py", line 13, in tags
    return {"tags": request.db.query(Tag).all()}
  File "/work/pyramid-realworld-example-app/.venv/lib/python3.7/site-packages/pyramid/decorator.py", line 43, in __get__
    val = self.wrapped(inst)
  File "/work/pyramid-realworld-example-app/.venv/lib/python3.7/site-packages/pyramid/util.py", line 81, in <lambda>
    fn = lambda this: callable(this)
  File "/work/pyramid-realworld-example-app/.venv/src/pyramid-deferred-sqla/pyramid_deferred_sqla/__init__.py", line 162, in _create_session
    connection.connection.set_session(readonly=True)
psycopg2.ProgrammingError: set_session cannot be used inside a transaction

You can replicate this bug by cloning https://github.com/niteoweb/pyramid-realworld-example-app, commenting out this line, running make run and pointing your browser to http://localhost:8080/api/tags.

Another thing: this only happens on the first GET request. Subsequent requests work fine.

@zupo you mention the code isn't called as part of WooCart, is that just background information or is there a 2nd problem in addition to the error triggered here?

This looks like a potential sqlalchemy bug to me. DefaultDialect calls do_rollback in its setup method, which happens after the select version(), select current_schema() and two SELECT CAST(...) statements that do feature detection. The intention of this seems to be to clear the implicit transaction that is created when doing those SELECTs. However, the PGDialect does a show standard_conforming_strings after DefaultDialect has already cleared up its implicit session (so long as the underlying postgres is > 8.2).

The result of this is that the first time that a connection is created it will have a single SELECT statement for feature detection inside an implicit transaction, which means that the first (and only the first) transaction cannoy have its isolation level (or read-only state) changed through sqlalchemy.

This can be verified in the line linked above by checking connection.connection.status. This should be 1 but the first time the code is run it is 2. I believe the right fix would be to submit a patch to sqlalchemy, but we can add a workaround in the deferred package.

zupo commented

@zupo you mention the code isn't called as part of WooCart, is that just background information or is there a 2nd problem in addition to the error triggered here?

Background information.

zupo commented

Please go ahead and work on the upstream sqlalchemy patch.

Maybe @zzzeek is willing to jump in if such patch would be accepted.

I think it's a one-liner, apart from any tests. I'll put it together and we'll see.

hi there -

I like patches but they need to have clear demonstrations of the behavior, preferably via MCVE inside of a new github issue that we link to the patch. For this one, we might need to have some discussion on this because it looks like you are running psycopg2-specific commands on the low level connection so I'm not sure to the degree SQLAlchemy should be promising the state of the underlying DBAPI to be in some special state. Also there obviously would need to be tests for anything changed, ideally this would be a database-agnostic change but it's not clear yet, so getting a good github issue and discussing it would be the best way to start.

zupo commented

The underlying sqlalchemy issue is closed and I just confirmed that the initial bug reported above can no longer be reproduced in latest master.