New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New transaction checks error when stamping #417

Closed
sqlalchemy-bot opened this Issue Feb 28, 2017 · 7 comments

Comments

Projects
None yet
1 participant
@sqlalchemy-bot

sqlalchemy-bot commented Feb 28, 2017

Migrated issue, originally created by Julien Danjou

Hi Mike,

So a recent commit included in 0.9.0:

https://bitbucket.org/zzzeek/alembic/commits/3f070ea17fa9f5618a299637421d73be6e98d2e5

broke our migration script this way:

alembic.util.exc.CommandError: Migration "stamp_revision -> 1e1a63d3d186" has left an uncommitted transaction opened; transactional_ddl is False so Alembic is not committing transactions

Full log:
http://logs.openstack.org/10/438810/12/check/gate-gnocchi-tox-py27-mysql-ubuntu-xenial/4f5b49d/console.html#_2017-02-28_18_30_27_537440

The code is stamping with "head" (that's 1e1a63d3d186). To do so it uses this migration code in env.py:

https://github.com/openstack/gnocchi/blob/master/gnocchi/indexer/alembic/env.py#L59-L80

and the problem is that since the migrations are run in a global transaction, that does not work anymore. Removing the transaction here makes the migration unsafe for an online migration AFAICT.

So… I understand why the check is there but I'm not sure whetever we need to fix our code or if the check is too aggressive or in the wrong place, or if it's a corner case. Hint appreciated. :)

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Michael Bayer (@zzzeek) wrote:

Ugh, it is probably a regression. That method is tested so not sure why it wasn't raising. I can't look at it until tomorrow :(

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Michael Bayer (@zzzeek) wrote:

oh ok, you have another transaction that's not normally part of env.py. I guess the check I added should test beforehand that there's not a transaction :).

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Michael Bayer (@zzzeek) wrote:

https://gerrit.sqlalchemy.org/324 . It would be super helpful if you could confirm this works on your end. I'll try to release tomorrow.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Julien Danjou wrote:

I've tested the patch Mike:

Ran: 1225 tests in 163.0000 sec.
 - Passed: 1184
 - Skipped: 41
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0

The patch is a go. I repeat, the patch is a go.

jd over. :)

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Dave McDaniel wrote:

The patch seems to fix this error for me as well.

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Michael Bayer (@zzzeek) wrote:

Don't raise on open transaction if we already started in one

An adjustment to the bug fix for 🎫369 to accommodate for
env.py scripts that use an enclosing transaction distinct from the
one that the context provides, so that the check for "didn't commit
the transaction" doesn't trigger in this scenario.

Change-Id: I3c1fa614495b61532999a84b2af3773e4d33c30b
Fixes: #417

cfb9d34

@sqlalchemy-bot

This comment has been minimized.

sqlalchemy-bot commented Mar 1, 2017

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment