From cf0e4b50797d008cd1c1b8a0549ae5b7cb8b1e84 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Tue, 23 Jan 2024 11:30:20 +0200 Subject: [PATCH] Fix calculation of batch ranges for background migration created with explicit ranges --- CHANGELOG.md | 2 ++ .../background_migrations/migration.rb | 5 +--- lib/online_migrations/batch_iterator.rb | 2 +- .../migration_runner_test.rb | 26 +++++++++++++++++++ test/background_migrations/migration_test.rb | 10 +++++++ 5 files changed, 40 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fe2c7c..a041f76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## master (unreleased) +- Fix calculation of batch ranges for background migration created with explicit ranges + ## 0.13.0 (2024-01-22) - Add ability to configure the path where generated background migrations will be placed diff --git a/lib/online_migrations/background_migrations/migration.rb b/lib/online_migrations/background_migrations/migration.rb index 0e9d41e..a04b7dd 100644 --- a/lib/online_migrations/background_migrations/migration.rb +++ b/lib/online_migrations/background_migrations/migration.rb @@ -197,10 +197,7 @@ def next_batch_range on_shard do # rubocop:disable Lint/UnreachableLoop - iterator.each_batch(of: batch_size, column: batch_column_name, start: next_min_value) do |relation, min_value, max_value| - if max_value.nil? - max_value = relation.pick(relation.arel_table[batch_column_name].maximum) - end + iterator.each_batch(of: batch_size, column: batch_column_name, start: next_min_value, finish: max_value) do |_relation, min_value, max_value| batch_range = [min_value, max_value] break diff --git a/lib/online_migrations/batch_iterator.rb b/lib/online_migrations/batch_iterator.rb index 52212ae..555876c 100644 --- a/lib/online_migrations/batch_iterator.rb +++ b/lib/online_migrations/batch_iterator.rb @@ -69,7 +69,7 @@ def each_batch(of: 1000, column: relation.primary_key, start: nil, finish: nil, # efficient UPDATE queries, hence we get rid of it. batch_relation = batch_relation.except(:order) - last_id = (last_row && last_row[column]) || start_id + last_id = (last_row && last_row[column]) || finish # Retaining the results in the query cache would undermine the point of batching. batch_relation.uncached { yield batch_relation, start_id, last_id } diff --git a/test/background_migrations/migration_runner_test.rb b/test/background_migrations/migration_runner_test.rb index 9fa8c01..ab22119 100644 --- a/test/background_migrations/migration_runner_test.rb +++ b/test/background_migrations/migration_runner_test.rb @@ -220,6 +220,32 @@ def test_run_all_migration_jobs assert_no_admins end + def test_run_all_migration_jobs_on_empty_table + m = create_migration + run_all_migration_jobs(m) + assert_no_admins + end + + def test_run_all_migration_jobs_on_empty_table_with_explicit_ranges + m = create_migration(min_value: 1, max_value: 100_000_000) + run_all_migration_jobs(m) + assert_no_admins + end + + def test_run_all_migration_jobs_on_empty_relation + _user = User.create!(admin: false) + m = create_migration + run_all_migration_jobs(m) + assert_no_admins + end + + def test_run_all_migration_jobs_on_empty_relation_with_explicit_ranges + _user = User.create!(admin: false) + m = create_migration(min_value: 1, max_value: 100_000_000) + run_all_migration_jobs(m) + assert_no_admins + end + def test_run_all_migration_jobs_on_composite_migration on_each_shard { 2.times { Dog.create! } } diff --git a/test/background_migrations/migration_test.rb b/test/background_migrations/migration_test.rb index 227c2fe..c6e4e56 100644 --- a/test/background_migrations/migration_test.rb +++ b/test/background_migrations/migration_test.rb @@ -267,6 +267,16 @@ def test_next_batch_range_on_edges assert_equal [user3.id, user3.id], m.next_batch_range end + def test_next_batch_range_on_empty_table + m = create_migration(batch_size: 2, sub_batch_size: 1) + assert_equal [1, 1], m.next_batch_range + end + + def test_next_batch_range_on_empty_table_and_explicit_ranges + m = create_migration(batch_size: 2, sub_batch_size: 1, min_value: 1000, max_value: 1_000_000) + assert_equal [1000, 1_000_000], m.next_batch_range + end + def test_next_batch_range_on_composite_migration on_shard(:shard_one) { Dog.create!(id: 100) } on_shard(:shard_two) { Dog.create!(id: 1000) }