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

gracefully handle when a migrator needs to modify a column that a view depends on #89

Open
nstrong-scw opened this issue May 5, 2022 · 8 comments

Comments

@nstrong-scw
Copy link
Contributor

Alembic version: 1.7.7
Alembic_utils version: 0.7.7
db: postgres 12

Here's a basic setup:

-- pretend I have SQLAlchemy classes set up to create these
CREATE TABLE A (
  id integer not null primary_key,
  some_field varchar(255)
);

CREATE VIEW A_VIEW(id, some_field) AS
  SELECT id, some_field FROM A;

Now, suppose you wish to change some_field to a text column. I change the class, run alembic revision -m 'update table' --autogenerate.

Expected behavior: the column should upgrade successfully.
Actual behavior: Postgres throws an error: "cannot alter type of a column used by a view or rule"

So, in other words, alembic_utils will successfully recreate views if the view definition is changed, but it doesn't catch when table columns associated with those views get modified.

I've devised a workaround, but it would be nice if it could be done automatically somehow.

Here is my (admittedly naive) workaround to solve this problem in the current code:

# - dependent_view_names is a list of view names that my migration conflicts with
# - lambda_func: a function that does the actual migration steps
def _wrap(dependent_view_names, lambda_func):
    # start by retrieving the views that currently exist at migration time
    existing_views = {v.signature: v for v in PGView.from_database(op.get_bind(), 'public')}
    for view in dependent_view_names:
        if view in existing_views:
            # drop the view and any dependent views
            op.drop_entity(existing_views[view], cascade=True)
    # do the thing
    lambda_func()
    # we replace all views, because the cascade delete may have deleted multiple views.
    for view in existing_views:
        op.replace_entity(existing_views[view])

def upgrade():
    _wrap(['A_VIEW'], op.alter_column('A', 'some_field', existing_type=sa.VARCHAR(length=255), type_=sa.Text(), existing_nullable=True))

def downgrade():
    _wrap(['A_VIEW'], op.alter_column('A', 'some_field', existing_type=sa.TEXT(), type_=sa.VARCHAR(length=255), existing_nullable=True))
@olirice
Copy link
Owner

olirice commented May 5, 2022

There is an experimental context manager for this use case:

from alembic_utils.depends import recreate_dropped

     def upgrade() -> None: 
  
         my_view = PGView(...) 
  
         with recreate_dropped(op.get_bind()) as conn: 
  
             op.drop_entity(my_view) 
             # you could also do `op.execute("drop view myview")` here
  
             # change an integer column to a bigint 
             op.alter_column( 
                 table_name="account", 
                 column_name="id", 
                 schema="public" 
                 type_=sa.BIGINT() 
                 existing_type=sa.Integer(), 
             ) 

when the context manager goes out of scope, anything you drop gets recreated

Please note that it is not currently part of the public API so it is use-at-your-own-risk

@nstrong-scw
Copy link
Contributor Author

Thanks for this!

I looked over the code behind recreate_dropped and it looks good to me. It's certainly a nicer way to handle the issue, although it'd still be cool if the need for it could be auto-detected at generate time.

The logging tends to be a little chatty (the "Resolving entities with dependencies" message gets repeated across each round), but that's a very minor complaint.

@olirice
Copy link
Owner

olirice commented May 5, 2022

it'd still be cool if the need for it could be auto-detected at generate time.

the tricky part w/ that is that the alembic migrations need to be executed for the schema diffing alembic_utils uses to work. That would be fine 80% of the time but some failure cases are:

  • if the alembic op can't be executed inside a transaction ( e.g. create index concurrently ) -> exception
  • if the operation op is slow (populating table / creating an index synchronously) then it will take a long time and might lock up the database

@olirice
Copy link
Owner

olirice commented May 5, 2022

The logging tends to be a little chatty (the "Resolving entities with dependencies" message gets repeated across each round), but that's a very minor complaint

If you'd like to open a PR I'm happy for that log line to more outside the loop

logger.info("Resolving entities with dependencies. This may take a minute")

@nstrong-scw
Copy link
Contributor Author

I took a stab at optimizing solve_resolution_order little bit:

#90

I'm getting an error KeyError: 'formatters' when I try to run tests. The error is coming from trying to parse the config file (alembic.ini), but I'm not sure why it can't find it. So, I don't know if the tests still pass or not--I'd appreciate guidance.

@olirice
Copy link
Owner

olirice commented May 6, 2022

Here are the steps to run the tests

Requirements:

  • docker-compose
  • python
git clone git@github.com:olirice/alembic_utils.git
cd alembic_utils
python -m venv venv
source venv/bin/activate
pip install -U pip
pip install -e ".[dev]"
pytest

If that fails, please add a comment with:

  • your OS
  • python version
  • a complete stacktrace
  • output of pip freeze

so i can try to reproduce

@nstrong-scw
Copy link
Contributor Author

No dice.

  • OS: MacOS 12.3.1
  • Python version: 3.10.3

pytest.log
pip.freeze.txt

As a side note, I'm not sure if it is relevant to the tests failing, but I had to re-pin pypy to the latest version because the old version pulls in a version of check-ast that doesn't build against python 3.9 and later.

@olirice
Copy link
Owner

olirice commented May 6, 2022

unfortunately i can not reproduce that error with 3.10.3 and the lib versions from pip freeze

It looks like its failing to find this key https://github.com/olirice/alembic_utils/blob/master/alembic.ini#L59
in this function

def run_alembic_command(engine: Engine, command: str, command_kwargs: Dict[str, Any]) -> str:

if you track it down I'd be happy to fix. In the meantime, please feel free to open PRs and let CI run the test suite for you as needed (mark as draft)

thanks

Repository owner deleted a comment from tobarbaro Feb 10, 2024
Repository owner deleted a comment from tobarbaro Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants