olirice / alembic_utils

An alembic/sqlalchemy extension for migrating sql views, functions, triggers, and policies
MIT License
193 stars 42 forks source link

ReplaceableEntity declarative depends_on #87

Closed nick4u closed 2 years ago

nick4u commented 2 years ago

Hi. This is PR with proposed extension for ReplaceableEntity adding manual ability to declare object dependency. Solution resolves issue when altering views/materialized views that depends on others. If view B depends on updated (existing) view A being changed drops view B -> updates A -> recreates B (see src/test/test_depends.py)

Dependency graph is resolved using graphlib and manually declared deps via depends_on attribute. graphlib is in std lib since python 3.8 - back port exists for older version of python.

This code works also when views and materialized views have dependiences. There is small caveat - dropping materialized view also drops any existing indexes created on it. Those changes do not solve the issue but as a workaround we have test that can detect this and outputs info that allows to easily correct such a cases - test

Main diff with current implementation and reason we for this PR: If in tests module with test_create_revision_with_explicit_depends_on you would comment all depends_on explicit hints this test fails as sqlalchemy.exc.ProgrammingError: (psycopg2.errors.UndefinedTable) relation "public.B_view" does not exist This tests differs from original test_depends.test_create_revision as it does 2nd alembic migration that is changing existing view with deps - now it would fail

I know it is possible to extract information from Postgres system views about what columns (and theirs origin) are used by view - this would be better way to extract depends_on info - I simply had not time allowed to do it this way. If you extract this depends_on info from postgres using system views this PR resolves ordering and calculation of what and in what order has to be done

olirice commented 2 years ago

note: same as #86

thanks Nick, that is very interesting.

While I do see the value and can imagine that some users may prefer a python explicit approach to dependencies, I'm going to keep with alembic_utils' opinionated stance of leaning on postgres to do the dependency resolution.

All the information we need to handle the cases your PR adds support for is available in postgres. If we do decide to enhance the dependency resolver, I'd strongly prefer to upgrade the existing one.

For that reason, and the workarounds, I'm going to close this PR

thanks again

olirice commented 2 years ago

and sorry for the slow response, its been a busy week!

If you're interested in making any other contributions, please open an issue to discuss what you had in mind. I'd love to hear any ideas you have for improvements