Add side_by_side mode to update_materialized_view #387

Roguelazer · 2023-04-04T23:42:08Z

This adds a side_by_side kwarg to the update_materialized_view method, which builds the new view alongside the old one and then atomically swaps them to reduce downtime at the cost of increasing disk usage. It is plumbed through to migrations as a hash value for the materialized kwarg of update_view.

Fixes #386.

This adds a `side_by_side` kwarg to the `update_materialized_view` method, which builds the new view alongside the old one and then atomically swaps them to reduce downtime at the cost of increasing disk usage. It is plumbed through to migrations as a hash value for the `materialized` kwarg of `update_view`.

Roguelazer · 2023-04-04T23:50:37Z

it's somewhat maddening that this repository doesn't have a working rubocop.yml (what is in there is a many-years-old version of rubocop and seems to disagree with how hound is configured).

ferblape · 2023-04-05T05:21:16Z

We also implemented something similar in a project using Scenic, but we had some issues with the indexes of the view you are replacing: you should drop them in the current view before adding the swap one, otherwise when you rename the view you'll have conflicts in the naming.

Roguelazer · 2023-04-05T05:28:33Z

Ah, good point. I could probably modify this to rename all the indices, too…

Roguelazer · 2023-04-05T15:30:12Z

Actually, this won't cause conflicts because the indexes on the new view get created after the swap, and the drop at the end of the swap removes the conflicting indexes.

That being said, building the indexes after the drop means that the indexes are built while holding the access exclusive lock, which probably is not a good idea.

renchap · 2023-06-09T15:55:16Z

@calebhearth @derekprior Gentle ping on this, do you need anything else for this PR to be merged?

This would really help some Mastodon materialized view changes :)

Roguelazer · 2023-08-04T23:57:59Z

Any changes you'd like me to make to this?

gsinkin · 2023-08-23T21:17:49Z

Just adding another nudge here

Roguelazer · 2023-10-18T18:59:01Z

Anything you'd like me to do to help move this forward?

derekprior

I think there are some issues that we're likely to encounter with this in the wild that require some more thought.

I also would like to encapsulate the functionality a bit differently but I don't have and easily explainable idea at the moment. Best bet may be to tackle the outstanding questions to get to a mergeable idea and then we can refactor after that.

derekprior · 2024-01-17T22:27:08Z

lib/scenic/adapters/postgres.rb

@@ -155,17 +155,36 @@ def create_materialized_view(name, sql_definition, no_data: false)
      # @param no_data [Boolean] Default: false. Set to true to create
      #   materialized view without running the associated query. You will need
      #   to perform a non-concurrent refresh to populate with data.
+      # @param side_by_side [Boolean] Default: false. Set to true to create the
+      #   new version under a different name and atomically swap them, limiting
+      #   downtime at the cost of doubling disk usage


I think we should be more precise than downtime. We limit the amount of time the view is inaccessible, perhaps?

derekprior · 2024-01-17T22:28:55Z

lib/scenic/adapters/postgres.rb

+      def update_materialized_view(
+        name, sql_definition, no_data: false, side_by_side: false
+      )


I know hound is not going to like this due to line length, but I don't care....

Suggested change

def update_materialized_view(

name, sql_definition, no_data: false, side_by_side: false

)

def update_materialized_view(name, sql_definition, no_data: false, side_by_side: false)

I'd need to also add some comments to make rubocop be quiet. I don't mind doing so (and actually set Metrics/LineLength to 120 on all my projects anyway).

I guess rubocop isn't automatically run (and the config file is remarkably old; at least <1.7 and broken with current rubocop versions) so I won't worry about it

derekprior · 2024-01-17T22:33:01Z

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, no_data: false)
+      def update_materialized_view(
+        name, sql_definition, no_data: false, side_by_side: false


I'm not sure side_by_side is the name I'd opt for here but I'm lacking a better suggetion at the moment other than background which I don't love either.

In your experience, is side_by_side an established name for this process?

I think "online" might be a more established name but I was trying to think of something that was very specific about what it did

derekprior · 2024-01-17T22:34:33Z

lib/scenic/adapters/postgres.rb

+          ) do
+            create_materialized_view(new_name, sql_definition, no_data: no_data)
+          end
+          rename_materialized_view(name, old_name)


Is there a reason not to drop here?

I think I did it this way out of the assumption that we might commit after the rename (but before the drop) and pause to give other threads a chance to finish using that view before we drop it, to reduce the lock impact. I didn't actually build that, of course.

derekprior · 2024-01-17T22:41:34Z

lib/scenic/adapters/postgres.rb

-          create_materialized_view(name, sql_definition, no_data: no_data)
+        if side_by_side
+          session_id = Time.now.to_i
+          new_name = "#{name}_new_#{session_id}"


Postgres identifiers are limited to 63 characters. We're removing 15 characters from that allotment already. This will likely be problematic for some view names. What does Rails do when, for example, a generated index name would be too long? We may need to access that functionality (or copy it) here.

IIRC they use a truncated hash of the generated components.

The name only matters for debugging, so I'd be fine with doing that. Something like

new_name = "#{name}_new_#{session_id}" if new_name.size > 63 # session_id is 10c, _new_ is 5c; leave lots of space free new_name = "#{Digest::SHA256.hexdigest(name)[...40]}_new_#{session_id}" end

derekprior · 2024-01-17T22:42:40Z

lib/scenic/adapters/postgres/index_reapplication.rb

+        def on_side_by_side(name, new_table_name, temporary_id)
+          indexes = Indexes.new(connection: connection).on(name)
+          indexes.each_with_index do |index, i|
+            old_name = "predrop_index_#{temporary_id}_#{i}"


This looks like it cuts 24 characters off the alloted 63 for postgres identifiers (see earlier comment). We may need to think more about how to lessen the chance this is an issue for folks

This one doesn't actually have the index name in it (it just looks like predrop_index_1705535993_1). I'm pretty sure I did it that way because I hit the index size limit. :-D

hound · 2024-01-18T00:14:49Z

lib/scenic/adapters/postgres.rb

@@ -271,9 +304,19 @@ def refresh_dependencies_for(name, concurrently: false)
          name,
          self,
          connection,
-          concurrently: concurrently,
+          concurrently: concurrently


Style/TrailingCommaInArguments: Put a comma after the last parameter of a multiline method call.

hound · 2024-01-18T00:14:49Z

lib/scenic/adapters/postgres.rb

+        if side_by_side
+          session_id = Time.now.to_i
+          new_name = generate_name name, "new_#{session_id}"
+          drop_name = generate_name name, "drop_#{session_id}"


Lint/UselessAssignment: Useless assignment to variable - drop_name.

hound · 2024-01-18T00:14:49Z

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, no_data: false)
+      def update_materialized_view(name, sql_definition, no_data: false, side_by_side: false)


Metrics/LineLength: Line is too long. [93/80]

hound · 2024-01-18T00:14:49Z

lib/scenic/adapters/postgres.rb

@@ -155,17 +157,34 @@ def create_materialized_view(name, sql_definition, no_data: false)
      # @param no_data [Boolean] Default: false. Set to true to create
      #   materialized view without running the associated query. You will need
      #   to perform a non-concurrent refresh to populate with data.
+      # @param side_by_side [Boolean] Default: false. Set to true to create the
+      #   new version under a different name and atomically swap them, limiting
+      #   the time that a view is inaccessible at the cost of doubling disk usage


Metrics/LineLength: Line is too long. [81/80]

hound · 2024-01-18T00:14:49Z

lib/scenic/adapters/postgres.rb

-  #{sql_definition.rstrip.chomp(';')}
-  #{'WITH NO DATA' if no_data};
+  #{sql_definition.rstrip.chomp(";")}
+  #{"WITH NO DATA" if no_data};


Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.

hound · 2024-01-18T00:14:49Z

lib/scenic/adapters/postgres.rb

@@ -137,8 +139,8 @@ def create_materialized_view(name, sql_definition, no_data: false)

        execute <<-SQL
  CREATE MATERIALIZED VIEW #{quote_table_name(name)} AS
-  #{sql_definition.rstrip.chomp(';')}
-  #{'WITH NO DATA' if no_data};
+  #{sql_definition.rstrip.chomp(";")}


Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations.

hound · 2024-01-18T00:14:49Z

Gemfile

-if rails_version == "master"
-  rails_constraint = { github: "rails/rails" }
+rails_constraint = if rails_version == "master"
+  {github: "rails/rails"}


Layout/IndentationWidth: Use 2 (not -17) spaces for indentation.
Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

Roguelazer added 2 commits April 4, 2023 16:37

fix various lint issues

85d866a

see if the linter likes this one

afd3177

Roguelazer added 4 commits April 5, 2023 09:27

recreate indexes safely too

d33d150

unbounded ranges are a ruby 2.6 feature

aefb6f3

fix some lints

27a6e28

one more rubocop lint

28e1be3

renchap mentioned this pull request Jun 13, 2023

Improve follow recommendations SQL query mastodon/mastodon#25334

Closed

derekprior requested changes Jan 17, 2024

View reviewed changes

Roguelazer added 2 commits January 17, 2024 16:04

Merge remote-tracking branch 'origin/main' into less-locks

504c43a

address review feedback

623338e

hound bot reviewed Jan 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add side_by_side mode to update_materialized_view #387

Add side_by_side mode to update_materialized_view #387

Roguelazer commented Apr 4, 2023

Roguelazer commented Apr 4, 2023

ferblape commented Apr 5, 2023

Roguelazer commented Apr 5, 2023

Roguelazer commented Apr 5, 2023

renchap commented Jun 9, 2023

Roguelazer commented Aug 4, 2023

gsinkin commented Aug 23, 2023

Roguelazer commented Oct 18, 2023

derekprior left a comment

derekprior Jan 17, 2024

derekprior Jan 17, 2024

Roguelazer Jan 18, 2024

Roguelazer Jan 18, 2024

derekprior Jan 17, 2024

Roguelazer Jan 17, 2024

derekprior Jan 17, 2024

Roguelazer Jan 18, 2024

derekprior Jan 17, 2024

Roguelazer Jan 17, 2024

derekprior Jan 17, 2024

Roguelazer Jan 18, 2024

hound bot Jan 18, 2024

hound bot Jan 18, 2024

hound bot Jan 18, 2024

hound bot Jan 18, 2024

hound bot Jan 18, 2024

hound bot Jan 18, 2024

hound bot Jan 18, 2024

Add side_by_side mode to update_materialized_view #387

Are you sure you want to change the base?

Add side_by_side mode to update_materialized_view #387

Conversation

Roguelazer commented Apr 4, 2023

Roguelazer commented Apr 4, 2023

ferblape commented Apr 5, 2023

Roguelazer commented Apr 5, 2023

Roguelazer commented Apr 5, 2023

renchap commented Jun 9, 2023

Roguelazer commented Aug 4, 2023

gsinkin commented Aug 23, 2023

Roguelazer commented Oct 18, 2023

derekprior left a 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

hound bot Jan 18, 2024

Choose a reason for hiding this comment

hound bot Jan 18, 2024

Choose a reason for hiding this comment

hound bot Jan 18, 2024

Choose a reason for hiding this comment

hound bot Jan 18, 2024

Choose a reason for hiding this comment

hound bot Jan 18, 2024

Choose a reason for hiding this comment

hound bot Jan 18, 2024

Choose a reason for hiding this comment

hound bot Jan 18, 2024

Choose a reason for hiding this comment