diff options
-rw-r--r-- | doc/development/migration_style_guide.md | 5 | ||||
-rw-r--r-- | lib/gitlab/database/migration_helpers.rb | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 62 |
3 files changed, 71 insertions, 11 deletions
diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 243ac7f0c98..1e060ffd941 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -136,11 +136,14 @@ class MyMigration < ActiveRecord::Migration disable_ddl_transaction! def up - remove_concurrent_index :table_name, :column_name if index_exists?(:table_name, :column_name) + remove_concurrent_index :table_name, :column_name end end ``` +Note that it is not necessary to check if the index exists prior to +removing it. + ## Adding indexes If you need to add a unique index please keep in mind there is the possibility diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 21287a8efd0..55160ca8708 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -59,6 +59,11 @@ module Gitlab disable_statement_timeout end + if index_exists?(table_name, column_name, options) + Rails.logger.warn "Index not created because it already exists (this may be due to an aborted migration or similar): table_name: #{table_name}, column_name: #{column_name}" + return + end + add_index(table_name, column_name, options) end @@ -83,6 +88,11 @@ module Gitlab disable_statement_timeout end + unless index_exists?(table_name, column_name, options) + Rails.logger.warn "Index not removed because it does not exist (this may be due to an aborted migration or similar): table_name: #{table_name}, column_name: #{column_name}" + return + end + remove_index(table_name, options.merge({ column: column_name })) end @@ -107,6 +117,11 @@ module Gitlab disable_statement_timeout end + unless index_exists_by_name?(table_name, index_name) + Rails.logger.warn "Index not removed because it does not exist (this may be due to an aborted migration or similar): table_name: #{table_name}, index_name: #{index_name}" + return + end + remove_index(table_name, options.merge({ name: index_name })) end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 1de3a14b809..9074e17ae80 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -67,17 +67,35 @@ describe Gitlab::Database::MigrationHelpers do model.add_concurrent_index(:users, :foo, unique: true) end + + it 'does nothing if the index exists already' do + expect(model).to receive(:index_exists?) + .with(:users, :foo, { algorithm: :concurrently, unique: true }).and_return(true) + expect(model).not_to receive(:add_index) + + model.add_concurrent_index(:users, :foo, unique: true) + end end context 'using MySQL' do - it 'creates a regular index' do - expect(Gitlab::Database).to receive(:postgresql?).and_return(false) + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + it 'creates a regular index' do expect(model).to receive(:add_index) .with(:users, :foo, {}) model.add_concurrent_index(:users, :foo) end + + it 'does nothing if the index exists already' do + expect(model).to receive(:index_exists?) + .with(:users, :foo, { unique: true }).and_return(true) + expect(model).not_to receive(:add_index) + + model.add_concurrent_index(:users, :foo, unique: true) + end end end @@ -95,6 +113,7 @@ describe Gitlab::Database::MigrationHelpers do context 'outside a transaction' do before do allow(model).to receive(:transaction_open?).and_return(false) + allow(model).to receive(:index_exists?).and_return(true) end context 'using PostgreSQL' do @@ -103,18 +122,41 @@ describe Gitlab::Database::MigrationHelpers do allow(model).to receive(:disable_statement_timeout) end - it 'removes the index concurrently by column name' do - expect(model).to receive(:remove_index) - .with(:users, { algorithm: :concurrently, column: :foo }) + describe 'by column name' do + it 'removes the index concurrently' do + expect(model).to receive(:remove_index) + .with(:users, { algorithm: :concurrently, column: :foo }) - model.remove_concurrent_index(:users, :foo) + model.remove_concurrent_index(:users, :foo) + end + + it 'does nothing if the index does not exist' do + expect(model).to receive(:index_exists?) + .with(:users, :foo, { algorithm: :concurrently, unique: true }).and_return(false) + expect(model).not_to receive(:remove_index) + + model.remove_concurrent_index(:users, :foo, unique: true) + end end - it 'removes the index concurrently by index name' do - expect(model).to receive(:remove_index) - .with(:users, { algorithm: :concurrently, name: "index_x_by_y" }) + describe 'by index name' do + before do + allow(model).to receive(:index_exists_by_name?).with(:users, "index_x_by_y").and_return(true) + end + + it 'removes the index concurrently by index name' do + expect(model).to receive(:remove_index) + .with(:users, { algorithm: :concurrently, name: "index_x_by_y" }) + + model.remove_concurrent_index_by_name(:users, "index_x_by_y") + end + + it 'does nothing if the index does not exist' do + expect(model).to receive(:index_exists_by_name?).with(:users, "index_x_by_y").and_return(false) + expect(model).not_to receive(:remove_index) - model.remove_concurrent_index_by_name(:users, "index_x_by_y") + model.remove_concurrent_index_by_name(:users, "index_x_by_y") + end end end |