feat: set lock_timeout to 0 for index create/drop (#30)

Author: rchoquet

README.md

@@ -206,9 +206,7 @@ So you can actually check that the `CREATE INDEX` statement will be performed co
 ```ruby
 SafePgMigrations.config.safe_timeout = 5.seconds # Lock and statement timeout used for all DDL operations except from CREATE / DROP INDEX
 
-SafePgMigrations.config.index_lock_timeout = 30.seconds # Lock timeout used for CREATE / DROP INDEX
-
-SafePgMigrations.config.blocking_activity_logger_margin = 1.second # Delay to output blocking queries before timeout. Must be smaller than safe_timeout and index_lock_timeout
+SafePgMigrations.config.blocking_activity_logger_margin = 1.second # Delay to output blocking queries before timeout. Must be shorter than safe_timeout
 
 SafePgMigrations.config.batch_size = 1000 # Size of the batches used for backfilling when adding a column with a default value pre-PG11
 

lib/safe-pg-migrations/configuration.rb

@@ -5,15 +5,13 @@
 module SafePgMigrations
   class Configuration
     attr_accessor :safe_timeout
-    attr_accessor :index_lock_timeout
     attr_accessor :blocking_activity_logger_margin
     attr_accessor :batch_size
     attr_accessor :retry_delay
     attr_accessor :max_tries
 
     def initialize
       self.safe_timeout = 5.seconds
-      self.index_lock_timeout = 30.seconds
       self.blocking_activity_logger_margin = 1.second
       self.batch_size = 1000
       self.retry_delay = 1.minute
@@ -24,10 +22,6 @@ def pg_safe_timeout
       pg_duration(safe_timeout)
     end
 
-    def pg_index_lock_timeout
-      pg_duration(index_lock_timeout)
-    end
-
     def pg_duration(duration)
       value, unit = duration.integer? ? [duration, 's'] : [(duration * 1000).to_i, 'ms']
       "#{value}#{unit}"

lib/safe-pg-migrations/plugins/blocking_activity_logger.rb

@@ -25,31 +25,22 @@ module BlockingActivityLogger
     SQL
 
     %i[
-      add_column remove_column add_foreign_key remove_foreign_key change_column_default
-      change_column_null create_table add_index remove_index
+      add_column remove_column add_foreign_key remove_foreign_key change_column_default change_column_null create_table
     ].each do |method|
       define_method method do |*args, &block|
-        log_blocking_queries(method) { super(*args, &block) }
+        log_blocking_queries { super(*args, &block) }
       end
     end
 
     private
 
-    def delay_before_logging(method)
-      timeout_delay =
-        if %i[add_index remove_index].include?(method)
-          SafePgMigrations.config.index_lock_timeout
-        else
-          SafePgMigrations.config.safe_timeout
-        end
-
-      timeout_delay - SafePgMigrations.config.blocking_activity_logger_margin
-    end
+    def log_blocking_queries
+      delay_before_logging =
+        SafePgMigrations.config.safe_timeout - SafePgMigrations.config.blocking_activity_logger_margin
 
-    def log_blocking_queries(method)
       blocking_queries_retriever_thread =
         Thread.new do
-          sleep delay_before_logging(method)
+          sleep delay_before_logging
           SafePgMigrations.alternate_connection.query(SELECT_BLOCKING_QUERIES_SQL % raw_connection.backend_pid)
         end
 

lib/safe-pg-migrations/plugins/statement_insurer.rb

@@ -50,15 +50,15 @@ def add_index(table_name, column_name, **options)
       options[:algorithm] = :concurrently
       SafePgMigrations.say_method_call(:add_index, table_name, column_name, options)
 
-      with_index_timeouts { super }
+      without_timeout { super }
     end
 
     def remove_index(table_name, options = {})
       options = { column: options } unless options.is_a?(Hash)
       options[:algorithm] = :concurrently
       SafePgMigrations.say_method_call(:remove_index, table_name, options)
 
-      with_index_timeouts { super }
+      without_timeout { super }
     end
 
     def backfill_column_default(table_name, column_name)
@@ -99,12 +99,12 @@ def without_statement_timeout
       with_setting(:statement_timeout, 0) { yield }
     end
 
-    def with_index_timeouts
-      without_statement_timeout do
-        with_setting(:lock_timeout, SafePgMigrations.config.pg_index_lock_timeout) do
-          yield
-        end
-      end
+    def without_lock_timeout
+      with_setting(:lock_timeout, 0) { yield }
+    end
+
+    def without_timeout
+      without_statement_timeout { without_lock_timeout { yield } }
     end
   end
 end

lib/safe-pg-migrations/plugins/statement_retrier.rb

@@ -3,8 +3,7 @@
 module SafePgMigrations
   module StatementRetrier
     RETRIABLE_SCHEMA_STATEMENTS = %i[
-      add_column add_foreign_key remove_foreign_key change_column_default
-      change_column_null add_index remove_index remove_column
+      add_column add_foreign_key remove_foreign_key change_column_default change_column_null remove_column
     ].freeze
 
     RETRIABLE_SCHEMA_STATEMENTS.each do |method|

test/create_table_test.rb

@@ -43,7 +43,7 @@ def change
 
       # Create the index.
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'CREATE INDEX CONCURRENTLY "index_users_on_user_id" ON "users" ("user_id")',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '5s'",
@@ -82,11 +82,11 @@ def change
     assert_calls [
       "SET statement_timeout TO '5s'",
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '5s'",
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'CREATE INDEX CONCURRENTLY "index_users_on_email" ON "users" ("email")',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '5s'",

test/safe_pg_migrations_test.rb

@@ -309,7 +309,7 @@ def change
 
       # An index is created because of the column reference.
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'CREATE INDEX CONCURRENTLY "index_users_on_user_id" ON "users" ("user_id")',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '70s'",
@@ -332,7 +332,7 @@ def change
     calls = record_calls(@connection, :execute) { run_migration }
     assert_calls [
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'CREATE INDEX CONCURRENTLY "index_users_on_email" ON "users" ("email")',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '70s'",
@@ -355,12 +355,12 @@ def change
 
     assert_calls [
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'CREATE INDEX CONCURRENTLY "my_custom_index_name" ON "users" ("email") WHERE email IS NOT NULL',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '70s'",
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '70s'",
     ], calls
@@ -398,12 +398,12 @@ def change
     calls = record_calls(@connection, :execute) { run_migration }
     assert_calls [
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
 
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'DROP INDEX CONCURRENTLY "index_users_on_email"',
-      "SET lock_timeout TO '30s'",
+      "SET lock_timeout TO '0'",
       "SET statement_timeout TO '0'",
 
       'CREATE INDEX CONCURRENTLY "index_users_on_email" ON "users" ("email")',
@@ -428,7 +428,7 @@ def change
 
       # The index is created concurrently.
       'SET statement_timeout TO 0',
-      "SET lock_timeout TO '30s'",
+      'SET lock_timeout TO 0',
       'CREATE INDEX CONCURRENTLY "index_users_on_user_id" ON "users" ("user_id")',
       "SET lock_timeout TO '5s'",
       "SET statement_timeout TO '70s'",