Add support for CASCADE in drop_view and update_view #218

jalada · 2017-08-31T11:06:50Z

We use lots of materialized views with dependencies and it can be quite painful to update them in a migration, especially remembering to recreate indexes.

This PR:

Adds an optional cascade: true to drop_view and update_view.
Makes update_view recreate any views that are lost during the view being updated, including any indexes on those views.
Stops the user from rolling back drop_view if they use cascade: true, because you can't tell what might've been lost. I've made the necessary change in CommandRecorder.

Things of note / bits I'm unsure about:

I don't know how this impacts the sqlite adapter, if at all.
I made try_index_create public because I'm using it to recreate indexes, but I'm not using it via the on() method. This is a new use of IndexReapplication and I'm conscious of that; perhaps some bigger API changes could be done here, but I didn't want to mess around too much!

If there's anything that needs changing, or any questions, let me know!

December 2020 update: Phew, the years just fly by don't they. There's a summary of outstanding known issues at the bottom of this thread.

houndci-bot · 2017-08-31T11:07:04Z

spec/support/migration_helpers.rb

+  def connection
+    ActiveRecord::Base.connection
+  end
+


Extra empty line detected at module body end.

Suggested change

houndci-bot · 2017-08-31T11:07:05Z

spec/support/migration_helpers.rb

@@ -0,0 +1,27 @@
+module MigrationHelpers
+


Extra empty line detected at module body beginning.

Suggested change

houndci-bot · 2017-08-31T11:07:05Z

spec/scenic/statements_spec.rb

@@ -87,7 +87,7 @@ module Scenic
        connection.update_view(:name, version: 3)

        expect(Scenic.database).to have_received(:update_view)
-          .with(:name, definition.to_sql)
+          .with(:name, definition.to_sql, false)


Place the . on the previous line, together with the method call receiver.

houndci-bot · 2017-08-31T11:07:05Z

spec/scenic/command_recorder_spec.rb

+      args = [:users, { cascade: true, revert_to_version: 3 }]
+
+      expect { recorder.revert { recorder.drop_view(*args) } }
+        .to raise_error(ActiveRecord::IrreversibleMigration)


Place the . on the previous line, together with the method call receiver.

houndci-bot · 2017-08-31T11:07:05Z

spec/integration/cascade_spec.rb

+    end
+  end
+
+


Extra blank line detected.
Extra empty line detected at block body end.

Suggested change

houndci-bot · 2017-08-31T11:07:05Z

spec/integration/cascade_spec.rb

+      end
+    end
+
+    it 'recreates the dependent view' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-08-31T11:07:06Z

spec/integration/cascade_spec.rb

+
+  describe 'as part of updating a view' do
+    around do |example|
+      with_view_definition :greetings, 2, "SELECT text 'good day' AS greeting" do


Line is too long. [81/80]

houndci-bot · 2017-08-31T11:07:06Z

spec/integration/cascade_spec.rb

+    }.to_not raise_error
+  end
+
+  describe 'as part of updating a view' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-08-31T11:07:06Z

spec/integration/cascade_spec.rb

+
+  it 'works' do
+    run_migration(migration_for_create, :up)
+    expect {


Avoid using {...} for multi-line blocks.

houndci-bot · 2017-08-31T11:07:06Z

spec/integration/cascade_spec.rb

+    end
+  end
+
+  it 'works' do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

jalada · 2017-08-31T11:12:38Z

Oh, also, this is related to #96

houndci-bot · 2017-09-19T11:05:05Z

lib/scenic/statements.rb

    # @return The database response from executing the create statement.
    #
    # @example
    #   update_view :engagement_reports, version: 3, revert_to_version: 2
    #
-    def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, materialized: false)
+    def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, materialized: false, cascade: false)


Unused method argument - revert_to_version.
Line is too long. [121/80]

houndci-bot · 2017-09-19T11:05:05Z

lib/scenic/statements.rb

    # @return The database response from executing the drop statement.
    #
    # @example Drop a view, rolling back to version 3 on rollback
    #   drop_view(:users_who_recently_logged_in, revert_to_version: 3)
    #
-    def drop_view(name, revert_to_version: nil, materialized: false)
+    def drop_view(name, revert_to_version: nil, materialized: false, cascade: false)


Unused method argument - revert_to_version.
Line is too long. [84/80]

houndci-bot · 2017-09-19T11:05:05Z

lib/scenic/statements.rb

    # @return The database response from executing the create statement.
    #
    # @example
    #   update_view :engagement_reports, version: 3, revert_to_version: 2
    #
-    def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, materialized: false)
+    def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, materialized: false, cascade: false)


Unused method argument - revert_to_version.
Line is too long. [121/80]

houndci-bot · 2017-09-19T11:05:06Z

lib/scenic/statements.rb

    # @return The database response from executing the drop statement.
    #
    # @example Drop a view, rolling back to version 3 on rollback
    #   drop_view(:users_who_recently_logged_in, revert_to_version: 3)
    #
-    def drop_view(name, revert_to_version: nil, materialized: false)
+    def drop_view(name, revert_to_version: nil, materialized: false, cascade: false)


Unused method argument - revert_to_version.
Line is too long. [84/80]

jalada · 2017-09-19T11:31:53Z

I've found a bug where this isn't working properly if you're updating a materialized view. Will push a fix!

houndci-bot · 2017-09-20T09:25:23Z

lib/scenic/command_recorder.rb

@@ -45,6 +45,11 @@ def perform_scenic_inversion(method, args)
        raise ActiveRecord::IrreversibleMigration, message
      end

+      if method == :create_view && scenic_args.cascade
+        message = "#{method} is not reversible if dependent objects were also dropped with CASCADE"


Line is too long. [99/80]

houndci-bot · 2017-09-20T09:25:23Z

lib/scenic/adapters/postgres.rb

+            create_materialized_view view.name, view.definition
+            # Also recreate any indexes that were lost
+            lost_indexes = indexes.select{|index| index.object_name == view.name}
+            lost_indexes.each{|index| index_reapplier.try_index_create  index}


Space missing to the left of {.
Space between { and | missing.
Space missing inside }.

houndci-bot · 2017-09-20T09:25:23Z

lib/scenic/adapters/postgres.rb

+          if view.materialized
+            create_materialized_view view.name, view.definition
+            # Also recreate any indexes that were lost
+            lost_indexes = indexes.select{|index| index.object_name == view.name}


Space missing to the left of {.
Space between { and | missing.
Space missing inside }.
Line is too long. [81/80]

Suggested change

lost_indexes = indexes.select{|index| index.object_name == view.name}

lost_indexes = indexes.select { |index| index.object_name == view.name }

houndci-bot · 2017-09-20T09:25:23Z

lib/scenic/adapters/postgres.rb

+        index_reapplier = IndexReapplication.new(connection: connection)
+
+        # Find any views that were lost
+        dropped_views = old_views.reject{|ov| current_views.any?{|cv| ov.name == cv.name}}


Space missing to the left of {.
Space between { and | missing.
Line is too long. [90/80]
Space missing inside }.

Suggested change

dropped_views = old_views.reject{|ov| current_views.any?{|cv| ov.name == cv.name}}

dropped_views = old_views.reject do |ov|

current_views.any? { |cv| ov.name == cv.name }

end

houndci-bot · 2017-09-20T09:25:23Z

lib/scenic/adapters/postgres.rb

@@ -238,6 +268,24 @@ def refresh_dependencies_for(name)
          connection,
        )
      end
+
+      def recreate_dropped_views(old_views, current_views, indexes=[])


Surrounding space missing in default value assignment.

Suggested change

def recreate_dropped_views(old_views, current_views, indexes=[])

def recreate_dropped_views(old_views, current_views, indexes = [])

houndci-bot · 2017-09-20T09:25:24Z

lib/scenic/adapters/postgres.rb

+
+          # Get indexes of existing materialized views 
+          indexes = Indexes.new(connection: connection)
+          view_indexes = existing_views.select(&:materialized).flat_map do |view|


Line is too long. [81/80]

Suggested change

view_indexes = existing_views.select(&:materialized).flat_map do |view|

potential_dependent_view_indexes = potential_dependent_views.

select(&:materialized).

flat_map { |view| indexes.on(view.name) }

houndci-bot · 2017-09-20T09:25:24Z

lib/scenic/adapters/postgres.rb

+          # Get existing views that could be dependent on this one.
+          existing_views = views.reject{|v| v.name == name}
+
+          # Get indexes of existing materialized views 


Trailing whitespace detected.

Suggested change

# Get indexes of existing materialized views

# Get indexes of existing materialized views

houndci-bot · 2017-09-20T09:25:24Z

lib/scenic/adapters/postgres.rb

        raise_unless_materialized_views_supported

+        if cascade
+          # Get existing views that could be dependent on this one.
+          existing_views = views.reject{|v| v.name == name}


Space missing to the left of {.
Space between { and | missing.
Space missing inside }.

Suggested change

existing_views = views.reject{|v| v.name == name}

existing_views = views.reject { |v| v.name == name }

Suggested change

existing_views = views.reject{|v| v.name == name}

potential_dependent_views = views.reject{|v| v.name == name}

houndci-bot · 2017-09-20T09:25:24Z

lib/scenic/adapters/postgres.rb

      #
      # @raise [MaterializedViewsNotSupportedError] if the version of Postgres
      #   in use does not support materialized views.
      #
      # @return [void]
-      def update_materialized_view(name, sql_definition)
+      def update_materialized_view(name, sql_definition, cascade=false)


Surrounding space missing in default value assignment.

Suggested change

def update_materialized_view(name, sql_definition, cascade=false)

def update_materialized_view(name, sql_definition, cascade = false)

houndci-bot · 2017-09-20T09:25:24Z

lib/scenic/adapters/postgres.rb

-      def drop_view(name)
-        execute "DROP VIEW #{quote_table_name(name)};"
+      def drop_view(name, cascade=false)
+        execute "DROP VIEW #{quote_table_name(name)}#{" CASCADE" if cascade};"


Prefer single-quoted strings inside interpolations.

Suggested change

execute "DROP VIEW #{quote_table_name(name)}#{" CASCADE" if cascade};"

execute "DROP VIEW #{quote_table_name(name)}#{' CASCADE' if cascade};"

jalada · 2017-09-20T09:28:36Z

Hound violations aside, I'd appreciate some suggestions on how to refactor the duplicated code, and also my use of eval in the tests to create similar migrations (one set materialized, one set not).

jalada · 2017-11-02T09:26:15Z

We've found an issue with the order in which views are captured and recreated; they are not in dependency order. Does anyone know of a way of asking for PSQL to return you views in order of dependency, or get the dependency tree for a bunch of views?

annarankin · 2017-11-08T16:45:54Z

Hello folks! This feature looks like a great addition 👍 Based on this article, I took a stab at a query to find PSQL views with their dependency order:

WITH RECURSIVE view_dependency_tree(parent_schema, parent_obj, child_schema, child_obj, ind, ord) AS 
(
  SELECT 
    vtu_parent.view_schema, 
    vtu_parent.view_name, 
    vtu_parent.table_schema, 
    vtu_parent.table_name, 
    '', 
    array[row_number() over (ORDER BY view_schema, view_name)]
  FROM information_schema.view_table_usage vtu_parent
  WHERE vtu_parent.view_schema = 'public'
  
  UNION ALL

  SELECT 
    vtu_child.view_schema, 
    vtu_child.view_name, 
    vtu_child.table_schema, 
    vtu_child.table_name, 
    vtu_parent.ind || '  ', 
    vtu_parent.ord || (row_number() over (ORDER BY view_schema, view_name))
  FROM view_dependency_tree vtu_parent, information_schema.view_table_usage vtu_child
  WHERE vtu_child.view_schema = vtu_parent.child_schema 
    AND vtu_child.view_name = vtu_parent.child_obj
) 
SELECT
  tree.parent_obj AS parent,
  tree.child_obj AS child,
  tree.ord
FROM view_dependency_tree tree
INNER JOIN information_schema.views view
  ON view.table_name = tree.child_obj
ORDER BY ord;

It seems to work for my views locally, hope it's helpful!

jalada · 2017-11-08T16:59:05Z

@annarankin this looks super useful! I’ll see if I can get it wired in

jalada · 2017-11-09T11:44:57Z

I had a problem with the information_schema-based query above. It doesn't include materialized views. It only showed me dependencies between my views and the underlying tables.

This gives me a set of views (materialized or otherwise) and the 'level' on the tree they are on. This could be combined with our 'what views disappeared?' code to recreate them in the correct order:

WITH RECURSIVE t AS
  -- Get every view & materialized view, assign a level 0
  ( SELECT c.oid,
           c.relname,
           0 AS LEVEL
   FROM pg_class c
   JOIN pg_namespace ON c.relnamespace = pg_namespace.oid
   WHERE c.relkind IN ('v', 'm') AND pg_namespace.nspname = 'public'
   -- Union back on ourselves, increasing the level to indicate that the view is dependent
   UNION ALL SELECT c.oid,
                    c.relname,
                    a.level+1
   FROM t a
   JOIN pg_depend d ON d.refobjid=a.oid
   JOIN pg_rewrite w ON w.oid= d.objid AND w.ev_class!=a.oid
   JOIN pg_class c ON c.oid=w.ev_class)
-- Take the max level for each view.
SELECT relname, MAX(level) AS level
FROM t
GROUP BY relname;

It's slow, about 3s on my DB. It's also PSQL specific, so we'd still need some other solution for other versions of SQL that have dependent non-materialized views.

annarankin · 2017-11-09T15:44:54Z

Nice! Just for some qualitative feedback, I tried it on my local; it's pretty quick (~6ms) for 47 views and a max level of 5.

jalada · 2017-11-15T14:31:34Z

That's a lot of dependent views! That must've been frustrating without cascade support...

Another issue we've encountered: If we have dependent views with a SELECT * they don't pick up newly added fields in their parent views because PSQL expands out the * when it returns the view definition. Not sure what to do about that.

ericfirth · 2018-10-12T15:36:24Z

whats the status of this? we have the same problem at my company. If this is going to go in, I'd be happy to work on fixing the hound/code climate violations, but I wouldn't want to waste that time if this won't be going in scenic

calebhearth · 2018-12-05T16:34:17Z

lib/scenic/adapters/postgres.rb

-        drop_view(name)
+      def update_view(name, sql_definition, cascade=false)
+        if cascade
+          # Get existing views that could be dependent on this one.


Suggested change

# Get existing views that could be dependent on this one.

calebhearth · 2018-12-05T16:34:26Z

lib/scenic/adapters/postgres.rb

+          # Get existing views that could be dependent on this one.
+          existing_views = views.reject{|v| v.name == name}
+
+          # Get indexes of existing materialized views 


Suggested change

# Get indexes of existing materialized views

calebhearth · 2018-12-05T16:35:54Z

lib/scenic/adapters/postgres.rb

+      def update_view(name, sql_definition, cascade=false)
+        if cascade
+          # Get existing views that could be dependent on this one.
+          existing_views = views.reject{|v| v.name == name}


Suggested change

existing_views = views.reject{|v| v.name == name}

potential_dependent_views = views.reject{|v| v.name == name}

calebhearth · 2018-12-05T16:36:20Z

lib/scenic/adapters/postgres.rb

+
+          # Get indexes of existing materialized views 
+          indexes = Indexes.new(connection: connection)
+          view_indexes = existing_views.select(&:materialized).flat_map do |view|


Suggested change

view_indexes = existing_views.select(&:materialized).flat_map do |view|

potential_dependent_view_indexes = potential_dependent_views.select(&:materialized).flat_map do |view|

calebhearth · 2018-12-05T16:36:42Z

lib/scenic/adapters/postgres.rb

        create_view(name, sql_definition)
+
+        recreate_dropped_views(existing_views, views, view_indexes) if cascade


Suggested change

recreate_dropped_views(existing_views, views, view_indexes) if cascade

if cascade

recreate_dropped_views(potential_dependent_views, views, potential_dependent_view_indexes)

end

calebhearth · 2018-12-05T16:47:37Z

spec/integration/cascade_spec.rb

+    end
+  end
+
+


Suggested change

calebhearth · 2018-12-05T16:47:46Z

spec/integration/cascade_spec.rb

+      end
+    end
+  end
+


Suggested change

calebhearth · 2018-12-05T16:47:58Z

spec/integration/revert_spec.rb

@@ -52,23 +52,4 @@ def change
    end
  end



Suggested change

calebhearth · 2018-12-05T16:48:24Z

spec/support/migration_helpers.rb

@@ -0,0 +1,27 @@
+module MigrationHelpers
+


Suggested change

calebhearth · 2018-12-05T16:48:35Z

spec/support/migration_helpers.rb

+  def connection
+    ActiveRecord::Base.connection
+  end
+


Suggested change

dansteele · 2019-10-29T11:22:38Z

Would be good to see this merged in 👍

brnosouza · 2019-11-29T13:18:00Z

Are there any plans to merge this?

jtemplet · 2020-11-30T22:50:59Z

Hey all, just wanted to follow up to see if there's any help needed to get this merged in?

cc @calebthompson @jalada

jalada · 2020-12-01T17:46:39Z

@jtemplet there are two problems with this PR that we identified, which makes it a bit more 'work-in-progress' than we originally thought:

When capturing views for recreation, they're not in dependency order. There's a couple of proposed solutions further down the thread. But one doesn't work, and one was slow & PSQL specific.
Views with SELECT * in the definition. This is pretty frustrating and I didn't know what to do about it.

So yes, it definitely needs some help, quite considerable help.

derekprior · 2021-12-15T23:11:26Z

We're having some issues with getting things in a stable, dependency order in a couple of different places in the codebase. I'm inclined to close this for now, but if we're able to confidently fix that issue then we could potentially revisit. See: dependencies

jalada · 2024-02-01T10:20:13Z

I wonder if #398 would help when revisiting this problem.

Context: We're doing a Ruby 2.7 -> 3.0 upgrade and our old fork isn't compatible, so we're thinking again about how to solve this problem.

Add support for CASCADE in drop_view and update_view

172e3b4

houndci-bot reviewed Aug 31, 2017

View reviewed changes

Views are _not_ returned in dependency order

f532c19

houndci-bot reviewed Sep 19, 2017

View reviewed changes

Perform cascade capture + recreate for materialized view update too

b8c27c1

houndci-bot reviewed Sep 20, 2017

View reviewed changes

calebhearth requested changes Dec 5, 2018

View reviewed changes

derekprior changed the base branch from master to main June 18, 2020 15:04

noellabo mentioned this pull request Dec 13, 2021

Fix follow recommendation biased towards older accounts mastodon/mastodon#17126

Merged

derekprior added the dependencies label Dec 15, 2021

derekprior closed this Dec 15, 2021

	lost_indexes = indexes.select{\|index\| index.object_name == view.name}
	lost_indexes = indexes.select { \|index\| index.object_name == view.name }

-        dropped_views = old_views.reject{|ov| current_views.any?{|cv| ov.name == cv.name}}
+        dropped_views = old_views.reject do |ov|
+          current_views.any? { |cv| ov.name == cv.name }
+       end

	def recreate_dropped_views(old_views, current_views, indexes=[])
	def recreate_dropped_views(old_views, current_views, indexes = [])

	# Get indexes of existing materialized views
	# Get indexes of existing materialized views

	existing_views = views.reject{\|v\| v.name == name}
	existing_views = views.reject { \|v\| v.name == name }

	existing_views = views.reject{\|v\| v.name == name}
	potential_dependent_views = views.reject{\|v\| v.name == name}

	def update_materialized_view(name, sql_definition, cascade=false)
	def update_materialized_view(name, sql_definition, cascade = false)

	execute "DROP VIEW #{quote_table_name(name)}#{" CASCADE" if cascade};"
	execute "DROP VIEW #{quote_table_name(name)}#{' CASCADE' if cascade};"

	view_indexes = existing_views.select(&:materialized).flat_map do \|view\|
	potential_dependent_view_indexes = potential_dependent_views.select(&:materialized).flat_map do \|view\|

		create_view(name, sql_definition)

		recreate_dropped_views(existing_views, views, view_indexes) if cascade

Add support for CASCADE in drop_view and update_view #218

Add support for CASCADE in drop_view and update_view #218

Conversation

jalada commented Aug 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalada commented Aug 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalada commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalada commented Sep 20, 2017

jalada commented Nov 2, 2017

annarankin commented Nov 8, 2017

jalada commented Nov 8, 2017

jalada commented Nov 9, 2017 • edited

annarankin commented Nov 9, 2017

jalada commented Nov 15, 2017

ericfirth commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dansteele commented Oct 29, 2019

brnosouza commented Nov 29, 2019

jtemplet commented Nov 30, 2020 • edited

jalada commented Dec 1, 2020

derekprior commented Dec 15, 2021

jalada commented Feb 1, 2024

jalada commented Aug 31, 2017 •

edited

jalada commented Aug 31, 2017 •

edited

jalada commented Nov 9, 2017 •

edited

jtemplet commented Nov 30, 2020 •

edited