Skip to content

Add option to disable mview sync #2

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Qu4tro
Copy link

@Qu4tro Qu4tro commented Jun 17, 2019

No description provided.

@Qu4tro Qu4tro requested a review from a team June 17, 2019 19:42
@Qu4tro Qu4tro changed the title Disable mview sync by default Add option to disable mview sync Jun 18, 2019
Copy link
Member

@rhngit rhngit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm; we have one normal view right now, right?

@Qu4tro
Copy link
Author

Qu4tro commented Jun 18, 2019

Yes. Unless you count #412, then we have 3.

@amackie-zyper
Copy link

amackie-zyper commented Jun 18, 2019

Our previous change to this fork was a modification to completely disable post-migration sync of everything. Why not bring this back with some kind of Django settings based option to decide what kind of views get synced on startup?

Or possibly more generally what ViewSyncer.run() does? That way we don't need to worry about changing deployment steps either

@Qu4tro
Copy link
Author

Qu4tro commented Jun 18, 2019

I guess that makes sense, but I'm not sure where sync_pgviews from apps is called.

We would then have a test setting to migrate everything and a production setting to avoid materialized views. Was that what were you thinking?

@amackie-zyper
Copy link

So it's been commented out here in this commit: 70ca721#diff-3298f2c0a31d05260003b7d38be4dbdcR45

My thought was that something would check django.settings for something like PGVIEWS_STARTUP_SYNC which would have a list like ['views', 'materialized_views'], which we would set to ['views'] in zyper-django-api, and its the same for test and production. Mviews carry on having to have a specific migration for them. (I'm really not keen on any difference between local, test & prod in this way)

Also, I hope we have a better chance of upstreaming if we can make this optional. To that end, if we take this route it might be best to get this fork up to date and discard the old commits.

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.

3 participants