xelixdev / django-pgviews-redux

Fork of django-postgres that focuses on maintaining and improving support for Postgres SQL Views.
The Unlicense
64 stars 15 forks source link

calling sync_pgviews causes recreation of tables #2

Closed martyzz1 closed 3 years ago

martyzz1 commented 4 years ago

I'm not sure if this is expected behaviour - but I'm looking to use this code in dockerised dev environment, and a heroku production environment.

I've noticed that calling ./manage sync_pgviews. upon deployment result in a recreation of all views.. This is going to be problematic...

I did some digging and it looks like the code for checking if a materialized view exists no longer works This query works...

cursor.execute( "SELECT COUNT(*) FROM pg_class WHERE relkind = %s and relname = %s;", [relkind, vname], ) However, the default behviour of sync_pgviews is then exposed as being "update" by default... So I didn't pursue it any further...

Not sure if this information is useful to you, but I thought I'd pass it on....

(I'm probably thinking I'll go make a manual migration to call the create view code as my solution)

martyzz1 commented 4 years ago

ahh didn't notice

./manage.py sync_pgviews --no-update

this works now

martyzz1 commented 4 years ago

https://github.com/mikicz/django-pgviews/pull/3

mikicz commented 4 years ago

Hi,

I'm not exactly sure what the problem is to be honest. From looking at the tests I think the current behaviour is the desired behaviour, therefore the original package authors ignored that the check if the materialized view exists doesn't exist. I think it comes from the fact that altering a materialized view doesn't make much sense (and by looking at the docs, the altering is very limited: https://www.postgresql.org/docs/9.3/sql-altermaterializedview.html, you can't e.g. change the SQL by looking at the docs), therefore for most changes it's faster and cleaner to drop and create the materialized view (and renaming columns etc are edge cases).

Keep in mind that there's another action you can do with materialized views, which is refreshing them, which do not drop/create them, they update the data inplace. There is not a command to do it, but you can implement one if you wish.

martyzz1 commented 4 years ago

so basically I'm trying to work out how to best do deployments..

right now I think it should be like so. This should be safe to run repeatedly, whenever new code is deployed.

./manage.py migrate
./manage.py sync_pgviews --no-update
./manage.py collectstatic --no-input --no-post-process

So this isn't about updating the data, I've got that covered. It's about ensuring that the sync_pgviews action is idempotent. If the view itself sits on a table that has a large quantity of data the repeated recreation would be troublesome...

mikicz commented 4 years ago

Ok now I'm with you. So the thing is that running migrate already recreates materialized views, so running sync_pgviews is useless in that sequence.

And I think that is a desired behaviour, recreating the materialized views on deploy, making sure they're correct with the latest DB version.

martyzz1 commented 4 years ago

almost, but not quite...

Imagine that the above sequence is played out everytime a deployment is made. The problem I was facing is when there are no changes... no new migrations. no new material views. The above code should just be a no-op. the migrations will be a no op, but the sync_pgviews --no-update. will still do a drop/create..

mikicz commented 4 years ago

I will in no way alter the current behaviour of the package, meaning all defaults will remain as they are and switches and settings will keep their meaning as current. If you wish to add new or change current functionality, you may submit a PR with changes that do not change default behaviour, but that are activated using e.g. switches or django settings (like not recreating all views on running of migrate, or sync_pgviews --no-update working on materialized views as well).

In that case and with sufficient documentation and testing I am willing to merge such a PR and push out a new version to PyPI.

amanz360 commented 3 years ago

This functionality was added in this commit: https://github.com/mikicz/django-pgviews/commit/d42ee47259772ceff33058f9699d3653a00db750

Should close this issue