Update migration query to make it work with existing view #2308
Conversation
Signed-off-by: Minkyu Park <minkyu.park.200@gmail.com>
| SELECT EXISTS ( | ||
| SELECT * FROM information_schema.views | ||
| WHERE table_schema='public' AND table_name='datasets_view' | ||
| ) INTO datasets_view_exists; | ||
|
|
||
| IF datasets_view_exists THEN | ||
| -- Altering is not allowed when the column is being used from views. So here, | ||
| -- we temporarily drop the view before altering and recreate it. | ||
| SELECT view_definition FROM information_schema.views | ||
| WHERE table_schema='public' AND table_name='datasets_view' | ||
| INTO datasets_view_definition; | ||
|
|
||
| DROP VIEW datasets_view; | ||
| ALTER TABLE dataset_symlinks ALTER COLUMN name TYPE VARCHAR; | ||
| EXECUTE format('CREATE VIEW datasets_view AS %s', datasets_view_definition); | ||
| ELSE | ||
| ALTER TABLE dataset_symlinks ALTER COLUMN name TYPE VARCHAR; | ||
| END IF; |
There was a problem hiding this comment.
I think it's an overkill, as
DROP VIEW IF EXISTS datasets_view;
at the beginning of the script should be enough as in PR:
https://github.com/MarquezProject/marquez/pull/2313/files
| SELECT EXISTS ( | ||
| SELECT * FROM information_schema.views | ||
| WHERE table_schema='public' AND table_name='datasets_view' | ||
| ) INTO datasets_view_exists; | ||
|
|
||
| IF datasets_view_exists THEN | ||
| -- Altering is not allowed when the column is being used from views. So here, | ||
| -- we temporarily drop the view before altering and recreate it. | ||
| SELECT view_definition FROM information_schema.views | ||
| WHERE table_schema='public' AND table_name='datasets_view' | ||
| INTO datasets_view_definition; | ||
|
|
||
| DROP VIEW datasets_view; | ||
| ALTER TABLE dataset_symlinks ALTER COLUMN name TYPE VARCHAR; | ||
| EXECUTE format('CREATE VIEW datasets_view AS %s', datasets_view_definition); | ||
| ELSE | ||
| ALTER TABLE dataset_symlinks ALTER COLUMN name TYPE VARCHAR; | ||
| END IF; |
There was a problem hiding this comment.
I think it's an overkill, as
DROP VIEW IF EXISTS datasets_view;
at the beginning of the script should be enough as in PR:
https://github.com/MarquezProject/marquez/pull/2313/files
No, it won't. As the doc describes here, https://flywaydb.org/documentation/concepts/migrations.html#repeatable-migrations , the repeatable migration only runs when their checksum changes, which means that it runs only when the repeatable migration itself changes. Repeatable migration is repeatable, but it doesn't run every time. |
There was a problem hiding this comment.
@fm100 You're right that dropping the view wouldn't be sufficient to go. Thanks for explanation.
Why is this better than modifying R__3_Datasets_view.sql file to get another checksum or making it run each time as described here -> https://flywaydb.org/blog/flyway-timestampsandrepeatables ?
Interesting. I don't think that this is better than making repeatable migration run. I did try adding a new line but it didn't change the checksum and this is what I ended up doing. It seems like that white spaces don't change the checksum but comments do. Anyway, unblocking this is pretty important and let's merge this. |
Signed-off-by: Minkyu Park minkyu.park.200@gmail.com
Problem
V52 migration fails if the
datasets_viewis already created by the previous migration, because postgresql does not allow altering the type of the column when it is being used by views.Closes: #2298
Solution
This PR changes the V52 migration query to drop the view before
ALTER. Because repeatable migration runs only when its checksum changes, we have to get the view definition first, and then drop and recreate it as before.Checklist
CHANGELOG.mdwith details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary).sqldatabase schema migration according to Flyway's naming convention (if relevant)