search

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 #86

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

olirice commented 2 years ago

Hi Nick,

Thanks for opening this, I can see you put a lot of thought into it! I didn't know graphilib was in std

To this point, alembic_utils has relied solely on postgres for dependency resolution. That decision was made so that dependencies can be resolved at the source of truth to make sure they can't get out-of-sync with the database.

Looking through the PR, I'm not clear what new usecases would be supported with explicit dependencies that aren't supported by the existing dependency resolver. Are there any, or is this intended as a way to make dependencies explicit in code?

Could you please rebase or merge from olirice/alembic_utils:master to reduce some of the diff noise to make it easier to see what was added?

To be as upfront as possible, I'm not likely to accept this PR. I'd prefer to keep dependencies as a non-concern for the user, even if that means some migrations have to get broken up into multiple steps but this was very interesting to see!

nick4u commented 2 years ago

I will clean up and push again - I've initially implemented those changes using release 0.7.6 - and merged 0.7.7 to minimize changes and instead pushed to much noise indeed

Looking through the PR, I'm not clear what new usecases would be supported with explicit dependencies that aren't supported by the existing dependency resolver.

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