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

If create or replace fails for a view, drop and recreate? #5

Closed
olirice opened this issue Jun 11, 2020 · 13 comments
Closed

If create or replace fails for a view, drop and recreate? #5

olirice opened this issue Jun 11, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@olirice
Copy link
Owner

olirice commented Jun 11, 2020

quoted from @mfsjr in #4

I made an unfortunate discovery preparing to retest with v2.6.

Dropping a column from a view cannot be handled using "create or replace", which returns "ERROR: cannot drop columns from view", Postgres docs confirm this, and "alter view" doesn't do it either.

In other words, what "create or replace" actually does is laughably disconnected from the English language.

It appears the concern is breaking other views that could depend on the soon-to-be-dropped column, even if there are none (bad).

Dropping the view and then dispatching "create or replace" works. If there is a dependent view, the drop will fail (good).

So it seems that in order for view migrations to work in both directions, "create or replace view" must first drop the view in question. That will fail with a sensible message if there are dependent views and succeed otherwise.

Would that be possible?

@olirice olirice added the enhancement New feature or request label Jun 11, 2020
@olirice
Copy link
Owner Author

olirice commented Jun 11, 2020

I think what you're asking for is

  • If create or replace view fails, try drop view and then create view.

That may still fail if the view has dependencies, but it will increase the chances of the migration succeeding.

That strategy could work for both upgrades and downgrades.

@olirice
Copy link
Owner Author

olirice commented Jun 11, 2020

Example of what would need to be emitted to have that effect

-- Starting with a 2 column view
create or replace view myview as
select 1 val1, 2 val2;

-- Begin migration to remove "val2" column
do $$

	begin
		/* First try to replace the view,
		which may succeed, even if there
		are dependent views */
		create or replace view myview as
		select 1 val1;
		
	-- If replacing the view raises an exception
	exception when others then
		
		/* Drop the existing view
		which will raise another exception
		and fail if there is a dependent view */
		drop view myview;
		
		-- Recreate the view however you want
		create view myview as
		select 1 val1;
				
	end;
	
$$ language 'plpgsql'

@olirice
Copy link
Owner Author

olirice commented Jun 11, 2020

My concern is that people may rely migrations to protect them from deleting columns in a view.

If you want that behavior now, you can get it by tweaking the autogenerated migration like this

From this

def downgrade():
    myview = PGView(
        schema='public',
        signature='myview',
        definition='select 1 as val1'
    )
    
    op.replace_entity(myview)

To this

def downgrade():
    myview = PGView(
        schema='public',
        signature='myview',
        definition='select 1 as val1'
    )
    
    op.drop_entity(myview)
    op.create_entity(myview)

I'd like to think more about if this is a problem that should be solved at the framework level before committing to anything.

hope that workaround helps in the meantime

@mfsjr
Copy link

mfsjr commented Jun 11, 2020

It always make sense to think about what will have to be lived with.

Trying create or replace before dropping makes sense.

A configuration switch might be helpful. Putting this situation in the documentation might also be helpful.

I'm ok regardless, thanks again.

@mfsjr
Copy link

mfsjr commented Jun 11, 2020

Retested with 0.2.6 and it worked.

@mfsjr
Copy link

mfsjr commented Jun 18, 2020

Any further thoughts on this? If the default behavior is to drop the view and give a readable error when the view has dependents, that seems hard to beat.

A configuration option that can modulate that behavior also seems like a maintainable solution.

@olirice
Copy link
Owner Author

olirice commented Jun 18, 2020

While I agree that the change would make a migration work by default more frequently, I’m not comfortable with that change b/c a new user would be surprised by it. They also may rely on certain failure conditions.

To this point I haven't introduced and configuration options & and I'd like to avoid that for as long as possible.

But, here's what you can do:
If you subclass PGView and override the to_sql_statement_create_or_replace to return the sql template I posted above, you'll have your own version with the behavior your want.

from alembic_utils.pg_view import PGView

class SuperPGView(PGView):
    
    def to_sql_statement_create_or_replace(self) -> str:
        """Generates a SQL "create or replace view" statement"""
        return sql_text(f"""
do $$
	begin
		CREATE OR REPLACE VIEW {self.literal_schema}."{self.signature}" AS {self.definition};
		
	exception when others then
		DROP VIEW {self.literal_schema}."{self.signature}";
		
		CREATE VIEW {self.literal_schema}."{self.signature}" AS {self.definition}
	end;
$$ language 'plpgsql'
"""
        )

I haven't tested that snippet. If you do go that route please post and changes you have to make for others who might end up here.

Here's a link to that method's current implementation

def to_sql_statement_create_or_replace(self) -> str:

@olirice
Copy link
Owner Author

olirice commented Jun 26, 2020

Were you able to get that working?

@mfsjr
Copy link

mfsjr commented Jun 27, 2020

I'm very sorry for the delay in getting back to you, thanks very much for the suggestion.

Yes, that worked on the first try!

@olirice
Copy link
Owner Author

olirice commented Jun 27, 2020

Great! closing

@Woodz
Copy link

Woodz commented Jan 14, 2021

It looks like the two approaches both have pros and cons:

  1. Upsert (create or update) can be used for non-breaking changes (where changing column sequence is considering breaking) without requiring dependant views to be dropped and re-created
  2. Force recreate (drop + create) will fail with dependant views regardless of whether the change is breaking or not

The proposal is to create a 3rd approach that combines them:
3. Try to Upsert, falling back to Force create. This will only fail if it is breaking change and the view is a dependency of something else, but this doesn't prevent accidentally making breaking changes

The other approach I could suggest is to pass a flag (e.g. --breaking) to alembic revision --autogenerate which controls whether breaking changes are supported or not. Therefore developers will explicitly need to define the revision as breaking in order to generate a migration that drops and recreates the view. Since this also impacts functions (#7) and presumably other DB objects, a more general approach could be taken.

@olirice
Copy link
Owner Author

olirice commented Jan 14, 2021

release 0.2.12 is now live on pypi and fails over from create or replace view to drop view ...; create view ....; by default

@Woodz
Copy link

Woodz commented Jan 14, 2021

release 0.2.12 is now live on pypi and fails over from create or replace view to drop view ...; create view ....; by default

Great! Thank you:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants