From 7097920ff91b489aaa08f8474f4fc0fe42bf9194 Mon Sep 17 00:00:00 2001 From: Pere Joan Martorell Date: Thu, 23 Oct 2025 12:19:58 +0200 Subject: [PATCH] Use session-level lock timeout for migrations with disable_ddl_transaction! BREAKING CHANGE: Migrations using disable_ddl_transaction! now have lock timeout protection. Previously these migrations had no timeout applied and would wait indefinitely for locks. Now they use session-level timeouts (SET lock_timeout) which are automatically reset after the migration. This addresses issue #16 where concurrent index creation and other non-transactional operations were left unprotected from lock contention. Changes: - Modified LockManager to use SET lock_timeout (session-level) for non-transactional migrations instead of SET LOCAL (transaction-level) - Added automatic timeout reset after non-transactional migrations complete - Updated tests to verify session-level timeout behavior - Updated README with detailed explanation and migration examples - Added breaking change notice to CHANGELOG with migration guide Users can opt-out by using disable_lock_timeout! or set custom timeouts using set_lock_timeout for specific migrations that need different behavior. --- CHANGELOG.md | 11 +++++ README.md | 43 ++++++++++++++++++- lib/migration_lock_timeout/lock_manager.rb | 19 +++++++- spec/migration_lock_timeout/migration_spec.rb | 13 ++++-- 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e68291f..0f8bddc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [Unreleased] + +## [2.0.0] - TBD +### Changed +- **BREAKING CHANGE**: Migrations using `disable_ddl_transaction!` now use session-level lock timeout (`SET lock_timeout`) instead of being skipped entirely. Previously, these migrations had no lock timeout applied. Now they will fail if they cannot acquire locks within the configured timeout period. This provides lock timeout protection for concurrent index creation and other non-transactional operations. The timeout is automatically reset after the migration completes. + + **Migration Guide**: If you have migrations using `disable_ddl_transaction!` that previously worked but may take longer than your configured timeout to acquire locks, you should either: + - Use `disable_lock_timeout!` to explicitly disable the timeout for that migration + - Use `set_lock_timeout ` to set a longer timeout for that specific migration + - Adjust your default timeout configuration to accommodate these operations + ## [1.5.0] ### Removed - Dropped support for Rails 6.0 and earlier diff --git a/README.md b/README.md index 8e9e340..c830053 100644 --- a/README.md +++ b/README.md @@ -70,7 +70,8 @@ set a timeout for a particular migration. ## disable_ddl_transaction! -If you use `disable_ddl_transaction!`, no lock timeout will occur +When you use `disable_ddl_transaction!`, the gem automatically switches to using a session-level lock timeout instead of a transaction-level timeout. This is necessary because non-transactional migrations don't run inside a database transaction, so the `SET LOCAL` command (which only applies within a transaction) wouldn't work. + ```ruby class AddMonkey < ActiveRecord::Migration @@ -84,6 +85,46 @@ If you use `disable_ddl_transaction!`, no lock timeout will occur end ``` +For this migration, the gem will execute: +```psql +SET lock_timeout = '5s'; -- Session-level timeout +-- Your migration code runs here +RESET lock_timeout; -- Reset to default after migration +``` + +This is particularly useful for operations that require `disable_ddl_transaction!`, such as: +- Creating indexes concurrently (`add_index :table, :column, algorithm: :concurrently`) +- Adding columns with a default value in older PostgreSQL versions +- Other operations that cannot run inside a transaction + +**Note:** The lock timeout is automatically reset after the migration completes to avoid affecting subsequent database operations. + +**Important:** If you need to disable the lock timeout for a specific non-transactional migration (for example, if the operation legitimately needs to wait longer for locks), you can combine `disable_ddl_transaction!` with `disable_lock_timeout!`: + +```ruby + class AddIndexConcurrently < ActiveRecord::Migration + disable_ddl_transaction! + disable_lock_timeout! # Explicitly disable timeout for this migration + + def change + add_index :large_table, :column, algorithm: :concurrently + end + end +``` + +Alternatively, you can set a custom timeout for the migration: + +```ruby + class AddIndexConcurrently < ActiveRecord::Migration + disable_ddl_transaction! + set_lock_timeout 30 # Wait up to 30 seconds for locks + + def change + add_index :large_table, :column, algorithm: :concurrently + end + end +``` + ## Running the specs To run the specs you must have [PostgreSQL](https://www.postgresql.org/) diff --git a/lib/migration_lock_timeout/lock_manager.rb b/lib/migration_lock_timeout/lock_manager.rb index f3e78b8..6c71f9f 100644 --- a/lib/migration_lock_timeout/lock_manager.rb +++ b/lib/migration_lock_timeout/lock_manager.rb @@ -5,12 +5,27 @@ def migrate(direction) timeout_disabled = self.class.disable_lock_timeout time = self.class.lock_timeout_override || MigrationLockTimeout.try(:config).try(:default_timeout) - if !timeout_disabled && direction == :up && time && !disable_ddl_transaction + + if !timeout_disabled && direction == :up && time safety_assured? do - execute "SET LOCAL lock_timeout = '#{time}s'" + if disable_ddl_transaction + # Use session-level timeout for non-transactional migrations + execute "SET lock_timeout = '#{time}s'" + else + # Use transaction-level timeout for transactional migrations + execute "SET LOCAL lock_timeout = '#{time}s'" + end end end + super + ensure + # Reset session-level timeout after non-transactional migrations + if !timeout_disabled && direction == :up && time && disable_ddl_transaction + safety_assured? do + execute "RESET lock_timeout" + end + end end def safety_assured? diff --git a/spec/migration_lock_timeout/migration_spec.rb b/spec/migration_lock_timeout/migration_spec.rb index 9ddd720..1c0ab75 100644 --- a/spec/migration_lock_timeout/migration_spec.rb +++ b/spec/migration_lock_timeout/migration_spec.rb @@ -176,11 +176,14 @@ def down end end - it 'runs migrate up without timeout' do + it 'runs migrate up with session-level timeout' do migration = AddMonkey.new expect_create_table - expect(ActiveRecord::Base.connection).not_to receive(:execute). - with("SET LOCAL lock_timeout = '5s'"). + expect(ActiveRecord::Base.connection).to receive(:execute). + with("SET lock_timeout = '5s'"). + and_call_original + expect(ActiveRecord::Base.connection).to receive(:execute). + with("RESET lock_timeout"). and_call_original migration.migrate(:up) end @@ -188,7 +191,9 @@ def down it 'runs migrate down without timeout' do migration = AddMonkey.new expect(ActiveRecord::Base.connection).not_to receive(:execute). - with("SET LOCAL lock_timeout = '5s'") + with("SET lock_timeout = '5s'") + expect(ActiveRecord::Base.connection).not_to receive(:execute). + with("RESET lock_timeout") migration.migrate(:down) end end