Commit 1362d9fe authored by Andreas Brandl's avatar Andreas Brandl

Shortcut concurrent index creation/removal if no effect.

Index creation does not have an effect if the index is present already.
Index removal does not have an affect if the index is not present.

This helps to avoid patterns like this in migrations:
```
if index_exists?(...)
  remove_concurrent_index(...)
end
```
parent 35097c19
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment