Skip to content
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

_ProxyTransaction.__exit__ fails after call to _ProxyTransaction.rollback #829

Closed
estandiaa-marain opened this issue Apr 17, 2021 · 9 comments
Labels

Comments

@estandiaa-marain
Copy link

estandiaa-marain commented Apr 17, 2021

Describe the bug
We are using this approach (suggested in this Stack Overflow answer) to do a dry-run for the alembic upgrade

with connectable.connect() as connection:
    context.configure(
        connection=connection, target_metadata=target_metadata,
        transactional_ddl=True
    )
    with context.begin_transaction() as transaction:
        context.run_migrations()
        if 'dry-run' in context.get_x_argument():
            print('Dry-run succeeded; now rolling back transaction...')
            transaction.rollback()

This used to work fine for our PostgreSQL database until recently.

Expected behavior
The DB upgrade is rolled back.

To Reproduce
See the code above. The key line is the call to transaction.rollback(). If this line is called when the transaction context manager is active, then an error is thrown when the __exit__ method is called.

Error

File "<python_path>/lib/python3.7/site-packages/alembic/runtime/migration.py", line 44, in __exit__
    self._proxied_transaction.__exit__(type_, value, traceback)
AttributeError: 'NoneType' object has no attribute '__exit__'

The problem seems to be that _ProxyTransaction.rollback sets self.migration_context._transaction = None which then throws an error when _ProxyTransaction.__exit__ runs self._proxied_transaction.__exit__(type_, value, traceback)

Versions.

  • OS: Ubuntu 20.04.2 LTS
  • Python: 3.7.7
  • Alembic: 1.5.8
  • SQLAlchemy: 1.3.23
  • Database: PostgreSQL 13.2
  • DBAPI: I am not sure what this is

Additional comments
This is my first issue in this repo. Please let me know if I missed anything or if you need any additional information. I am happy to provide more details.

Thank you in advance.

@estandiaa-marain estandiaa-marain added the requires triage New issue that requires categorization label Apr 17, 2021
@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the master branch:

Ensure proxy transaction still present on exit before closing https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2756

@zzzeek zzzeek added bug Something isn't working migration environment regression and removed requires triage New issue that requires categorization labels Apr 17, 2021
@zzzeek
Copy link
Member

zzzeek commented Apr 17, 2021

thanks for reporting. This won't be released until 1.6.0 for which I have another large change to merge still, so for now I would advise either staying on 1.4.x or you might try a temporary workaround like:

    with context.begin_transaction() as transaction:
        context.run_migrations()
        transaction._proxied_transaction.rollback()

@estandiaa-marain
Copy link
Author

Thanks for the quick response. The workaround that works great!

@jiyolla
Copy link

jiyolla commented Aug 26, 2021

For me, the 'transaction' is set to None from the beginning of context.

with connectable.connect() as connection:
    context.configure(
        connection=connection, target_metadata=target_metadata
    )
    with context.begin_transaction() as transaction:
        context.run_migrations()
        if 'dry-run' in context.get_x_argument():
            print('Dry-run succeeded; now rolling back transaction...')
            transaction.rollback()

Any reason this is happening?

I'm on alembic=1.6.5 with sqlite(and the default dbapi), sqlalchemy=1.4.22, python=3.8.5, Ubuntu=20.04,

@zzzeek
Copy link
Member

zzzeek commented Aug 26, 2021

you need to be on a backend that uses transactional DDL or otherwise set the transactional_ddl flag to True, this confused me as well just now. i've updated the SO answer as well as the above description. code looks like:

with connectable.connect() as connection:
    context.configure(
        connection=connection, target_metadata=target_metadata,
        transactional_ddl=True
    )
    with context.begin_transaction() as transaction:
        context.run_migrations()
        if 'dry-run' in context.get_x_argument():
            print('Dry-run succeeded; now rolling back transaction...')
            transaction.rollback()

@jiyolla
Copy link

jiyolla commented Aug 29, 2021

you need to be on a backend that uses transactional DDL or otherwise set the transactional_ddl flag to True, this confused me as well just now. i've updated the SO answer as well as the above description. code looks like:

with connectable.connect() as connection:
    context.configure(
        connection=connection, target_metadata=target_metadata,
        transactional_ddl=True
    )
    with context.begin_transaction() as transaction:
        context.run_migrations()
        if 'dry-run' in context.get_x_argument():
            print('Dry-run succeeded; now rolling back transaction...')
            transaction.rollback()

Sry for the late reply.

That worked! Thank you very much.
Glad to know that you are the author of the SO answer.
It's that SO post lead me here.

@jiyolla
Copy link

jiyolla commented Aug 29, 2021

Since you are also the maintainer of the alembic,
may I ask when is it supposed to do such 'dry run'?

I initially searched for 'dry run', because I wasn't sure that the handwritten part of migration scripts will work as expected and I do not want the untested code to possibly damage my database.

But it seems that if anything goes wrong the rollback procedure is automatically started.
Is this true?
If it's true, then when should I use the dry-run?
Maybe when the migration is not 'illegal'(so no error raised and thus no rollback) but not as 'intended'?
Then possibly one will run some extra code(like printing some sample rows, or metadatas) before rollback to make sure the migration worked as intended?
But that would mean quite frequent modification of 'env.py', if the double-checking process is not general enough and rather as per migration.
So I'm not sure if that's good idea.

Your answer as both the author of the alembic and the dryrun code would definitively help.

@zzzeek
Copy link
Member

zzzeek commented Aug 29, 2021

you perhaps could use "dry run" to test your migration as you are adding new things to it. usually this would be in an environment where "downgrades" aren't being implemented for whatever reason. the rationale given in the stackoverflow answer is when running a migration against a production database that otherwise succeeded in development scenarios. id strongly recommend having an accurate staging copy of the production environment instead of that, but every situation is different.

@jiyolla
Copy link

jiyolla commented Aug 29, 2021

you perhaps could use "dry run" to test your migration as you are adding new things to it. usually this would be in an environment where "downgrades" aren't being implemented for whatever reason. the rationale given in the stackoverflow answer is when running a migration against a production database that otherwise succeeded in development scenarios. id strongly recommend having an accurate staging copy of the production environment instead of that, but every situation is different.

Thanks for the quick reply.
I should have read the question description in SO more carefully.

jiyolla added a commit to Remian103/lolskin-price-tracker that referenced this issue Sep 2, 2021
See
sqlalchemy/alembic#829

Dry run for migration scripts on production database for double check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants