-
-
Notifications
You must be signed in to change notification settings - Fork 222
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
Add support for updating materialized views #102
Conversation
"'drop_view' followed by 'create_view', being sure to also recreate "\ | ||
"any previously-existing indexes." | ||
Scenic.database.reapplying_indexes(on: name) do | ||
drop_view(name, revert_to_version: revert_to_version, materialized: materialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [89/80]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also fine with this. I feel like readability is better with the longer line.
34b7671
to
7570446
Compare
echo "[success]" | ||
|
||
echo "rails destroy scenic:view search --materialized" | ||
echo "rails destroy scenic:model search --materialized" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this line should be moved below 117, and the scenic:view echo should stay here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we're actually testing here is the model
destroy since we never tested that previously. We have to first do a view
destroy so that the second version of the view is deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Updating materialized views used to be an error because providing a new definition for a materialized view requires dropping and recreating it. This will cause any indexes on the old materialized view to be dropped. For this reason, we suggested that users manually `drop_view`, `create_view` and `add_index`. This is a frustrating user experience. This changes view updates to record any indexes that were in place before the `drop_view` and then re-create those indexes once the new materialized view is in place. Any indexes that are now invalid (most-likely because the column no longer exists) will be dropped. This behavior is similar to what happens when you alter the schema of a table, so I do not believe it to be surprising. We log the results of each index creation for the users information in the migration output. For example: ``` == 20151228201117 UpdateSearchesToVersion2: migrating ========================= -- update_view(:searches, {:version=>2, :revert_to_version=>1, :materialized=>true}) -> index 'searches_test_1' on 'searches' has been recreated -> index 'searches_test_2' on 'searches' is no longer valid and has been dropped. -> 0.0148s == 20151228201117 UpdateSearchesToVersion2: migrated (0.0149s) ================ ``` Closes #98
7570446
to
d804934
Compare
"index '#{index.index_name}' on '#{index.object_name}' has been recreated" | ||
rescue ActiveRecord::StatementInvalid | ||
connection.execute("ROLLBACK TO SAVEPOINT #{index.index_name}") | ||
"index '#{index.index_name}' on '#{index.object_name}' is no longer valid and has been dropped." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [104/80]
@calebthompson I moved the PG specific code into the adapter. I think there's some further refactoring (as pointed out by hound's ABC warning here) to slim down and break up the adapter, but I'd like to attack that in a separate refactoring. |
lgtm |
Updating materialized views used to be an error because providing a new
definition for a materialized view requires dropping and recreating it.
This will cause any indexes on the old materialized view to be dropped.
For this reason, we suggested that users manually
drop_view
,create_view
andadd_index
. This is a frustrating user experience.This changes view updates to record any indexes that were in place
before the
drop_view
and then re-create those indexes once the newmaterialized view is in place. Any indexes that are now invalid
(most-likely because the column no longer exists) will be dropped. This
behavior is similar to what happens when you alter the schema of a
table, so I do not believe it to be surprising.
We log the results of each index creation for the users information in
the migration output. For example:
Closes #98