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

Update pg_materialized_view.py #46

Closed
wants to merge 2 commits into from
Closed

Conversation

onkel-uwe
Copy link

I had problems using this to create materialized views with indizes cause of alembic migration tried to replace the views without indizes every time a new autogenerate has to be used. We deactivated dropping of type: 'materialized views' in alembic.ini and build it with replaceableObject of alembic migration, but this changes at pg_materialized_view.py resolve the whole problem

Materalized view would be created like this then, multicolumn-index is possible also, same as statement without indizes-parameter:

test_persons_matview = PGMaterializedView(
schema='public',
signature='test_persons',
definition="""select firstname, lastname, city from public.persons
""",
with_data=False,
indizes=[{'name': 'test_persons_lastname_idx', 'columns': 'lastname'},
{'name': 'test_persons_city_idx', 'columns': 'city'}]
)

I had problems using this to create materialized views with indizes cause of alembic migration tried to replace the views without indizes every time a new autogenerate has to be used. We deactivated dropping of type: 'materialized views' in alembic.ini and build it with replaceableObject of alembic migration, but this changes at pg_materialized_view.py resolve the whole problem 

Materalized view would be created like this then, multicolumn-index is possible also, same as statement without indizes-parameter:

test_persons_matview = PGMaterializedView(
    schema='public',
    signature='test_persons',
    definition="""select firstname, lastname, city from public.persons
    """,
    with_data=False,
    indizes=[{'name': 'test_persons_lastname_idx', 'columns': 'lastname'},
             {'name': 'test_persons_city_idx', 'columns': 'city'}]
)
@olirice
Copy link
Owner

olirice commented May 12, 2021

Thank you for opening a PR

Supporting indexes on materialized views is a feature I'm interested in but this PR would need changes before it could be accepted

  • The type of indexes would need to be List[Index] from sqlalchemy.Index and support unique, non-unique, single and multi-column
  • cls.from_database does not currently pull the indexes down from the database. That would cause any materialized view with an index to produce an op.replace_entity any time --autogenerate was used
  • cls.get_required_migration_op would also have to change because it would not detect difference in indexes
  • test cases

There are some other complications related to drop cascades and dependency resolution but if you have the interest to tackle the list above, I'm happy to handle the resolver/weird stuff since its pretty hairy

Thanks for the fast reply.
Ive implemented the usage of List(Index) instead of my own-generated List. Problem there is, I dont know how to get the import Index in the autogenerated migrationfile. I have to speak with my colleagues about that. Im learning/using python since start of this year, so it maybe isnt the best pythonic way Ive implemented it.
cls.from_database seems ok with it. As long as I dont change something in the view definition the autogenerate dont create a replace statement. Problem is, that if you would need a replace (ie change of defined columns for the MatView) the downgrade dont know the indexes.
Look into cls.get_required_migration_op would need a bit of time.
@aidos
Copy link
Contributor

aidos commented Jan 14, 2022

@olirice Turns out that this is a feature I'm going to need myself now. In terms of the API, you're happy with extending PGMaterializedView to take a list of sqla indexes? If so I'll start another PR taking a run at this myself.

@aidos
Copy link
Contributor

aidos commented Jan 16, 2022

@olirice this is a bit of a curious one. Pulling down the indexes is simple enough (sqla even supports doing it directly using their reflection api), but all the sqla tooling is around having a proper table to attach the index to.

I was looking into the possibility of making a fake table to make sql happy and then we could maybe piggy-back on the existing alembic index migration code. Again, I think you could probably make that work, but on closer inspection it's not going to work for me anyway because sqla won't reflect the more complex indexes I need to support.

Certainly in my case, it would be simpler to compare pg_indexes.indexdef against my own create statement string and I'd be able to tell what needing creating / dropping.

Looking at the sqla / alembic code it almost feels like it's much harder there because you have to support generalised reflection of arbitrary index definitions (eg CREATE INDEX complex_trgm_idx ON blah USING gin (lower(text_field) gin_trgm_ops)). There's a lot more parsing and work to do to create these (which isn't supported currently).

@olirice
Copy link
Owner

olirice commented Jan 17, 2022

@aidos thank you for the summary of what you looked into

I agree that this is a big undertaking. I've been maintaining materialized view indexes manually in migrations rather than developing objects for them for that reason

@aidos
Copy link
Contributor

aidos commented Jan 18, 2022

I've been deeper into the alembic / sqlalchemy side now and I'm going to start from that direction since I actually need to support more complex indexes than they're currently handling. I'll get it so that expression based indexes (eg lower(name) etc) are working first (in sqla reflection and alembic migrations). Once that's done I'll revisit the materialised views aspect to see if there's a reasonable workaround.

@olirice
Copy link
Owner

olirice commented Feb 15, 2023

closing for age

@olirice olirice closed this Feb 15, 2023
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

Successfully merging this pull request may close these issues.

None yet

3 participants