Commit 9b4a1db9 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'ab-43887-concurrent-migration-helpers' into 'master'

Convenient use of concurrent migration helpers

Closes #43887

See merge request gitlab-org/gitlab-ce!17888
parents 30c480c2 c914883a
...@@ -154,7 +154,7 @@ class ProjectForeignKeysWithCascadingDeletes < ActiveRecord::Migration ...@@ -154,7 +154,7 @@ class ProjectForeignKeysWithCascadingDeletes < ActiveRecord::Migration
end end
def add_foreign_key_if_not_exists(source, target, column:) def add_foreign_key_if_not_exists(source, target, column:)
return if foreign_key_exists?(source, column) return if foreign_key_exists?(source, target, column: column)
add_concurrent_foreign_key(source, target, column: column) add_concurrent_foreign_key(source, target, column: column)
end end
...@@ -175,12 +175,6 @@ class ProjectForeignKeysWithCascadingDeletes < ActiveRecord::Migration ...@@ -175,12 +175,6 @@ class ProjectForeignKeysWithCascadingDeletes < ActiveRecord::Migration
rescue ArgumentError rescue ArgumentError
end end
def foreign_key_exists?(table, column)
foreign_keys(table).any? do |key|
key.options[:column] == column.to_s
end
end
def connection def connection
# Rails memoizes connection objects, but this causes them to be shared # Rails memoizes connection objects, but this causes them to be shared
# amongst threads; we don't want that. # amongst threads; we don't want that.
......
...@@ -10,13 +10,13 @@ class AddStageIdForeignKeyToBuilds < ActiveRecord::Migration ...@@ -10,13 +10,13 @@ class AddStageIdForeignKeyToBuilds < ActiveRecord::Migration
add_concurrent_index(:ci_builds, :stage_id) add_concurrent_index(:ci_builds, :stage_id)
end end
unless foreign_key_exists?(:ci_builds, :stage_id) unless foreign_key_exists?(:ci_builds, :ci_stages, column: :stage_id)
add_concurrent_foreign_key(:ci_builds, :ci_stages, column: :stage_id, on_delete: :cascade) add_concurrent_foreign_key(:ci_builds, :ci_stages, column: :stage_id, on_delete: :cascade)
end end
end end
def down def down
if foreign_key_exists?(:ci_builds, :stage_id) if foreign_key_exists?(:ci_builds, column: :stage_id)
remove_foreign_key(:ci_builds, column: :stage_id) remove_foreign_key(:ci_builds, column: :stage_id)
end end
...@@ -24,12 +24,4 @@ class AddStageIdForeignKeyToBuilds < ActiveRecord::Migration ...@@ -24,12 +24,4 @@ class AddStageIdForeignKeyToBuilds < ActiveRecord::Migration
remove_concurrent_index(:ci_builds, :stage_id) remove_concurrent_index(:ci_builds, :stage_id)
end end
end end
private
def foreign_key_exists?(table, column)
foreign_keys(:ci_builds).any? do |key|
key.options[:column] == column.to_s
end
end
end end
...@@ -23,23 +23,15 @@ class AddForeignKeyToMergeRequests < ActiveRecord::Migration ...@@ -23,23 +23,15 @@ class AddForeignKeyToMergeRequests < ActiveRecord::Migration
merge_requests.update_all(head_pipeline_id: nil) merge_requests.update_all(head_pipeline_id: nil)
end end
unless foreign_key_exists?(:merge_requests, :head_pipeline_id) unless foreign_key_exists?(:merge_requests, column: :head_pipeline_id)
add_concurrent_foreign_key(:merge_requests, :ci_pipelines, add_concurrent_foreign_key(:merge_requests, :ci_pipelines,
column: :head_pipeline_id, on_delete: :nullify) column: :head_pipeline_id, on_delete: :nullify)
end end
end end
def down def down
if foreign_key_exists?(:merge_requests, :head_pipeline_id) if foreign_key_exists?(:merge_requests, column: :head_pipeline_id)
remove_foreign_key(:merge_requests, column: :head_pipeline_id) remove_foreign_key(:merge_requests, column: :head_pipeline_id)
end end
end end
private
def foreign_key_exists?(table, column)
foreign_keys(table).any? do |key|
key.options[:column] == column.to_s
end
end
end end
...@@ -26,11 +26,11 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration ...@@ -26,11 +26,11 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
def down def down
execute "TRUNCATE user_interacted_projects" execute "TRUNCATE user_interacted_projects"
if foreign_key_exists?(:user_interacted_projects, :user_id) if foreign_key_exists?(:user_interacted_projects, :users)
remove_foreign_key :user_interacted_projects, :users remove_foreign_key :user_interacted_projects, :users
end end
if foreign_key_exists?(:user_interacted_projects, :project_id) if foreign_key_exists?(:user_interacted_projects, :projects)
remove_foreign_key :user_interacted_projects, :projects remove_foreign_key :user_interacted_projects, :projects
end end
...@@ -115,7 +115,7 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration ...@@ -115,7 +115,7 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
end end
def create_fk(table, target, column) def create_fk(table, target, column)
return if foreign_key_exists?(table, column) return if foreign_key_exists?(table, target, column: column)
add_foreign_key table, target, column: column, on_delete: :cascade add_foreign_key table, target, column: column, on_delete: :cascade
end end
...@@ -158,11 +158,11 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration ...@@ -158,11 +158,11 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
add_concurrent_index :user_interacted_projects, [:project_id, :user_id], unique: true, name: UNIQUE_INDEX_NAME add_concurrent_index :user_interacted_projects, [:project_id, :user_id], unique: true, name: UNIQUE_INDEX_NAME
end end
unless foreign_key_exists?(:user_interacted_projects, :user_id) unless foreign_key_exists?(:user_interacted_projects, :users, column: :user_id)
add_concurrent_foreign_key :user_interacted_projects, :users, column: :user_id, on_delete: :cascade add_concurrent_foreign_key :user_interacted_projects, :users, column: :user_id, on_delete: :cascade
end end
unless foreign_key_exists?(:user_interacted_projects, :project_id) unless foreign_key_exists?(:user_interacted_projects, :projects, column: :project_id)
add_concurrent_foreign_key :user_interacted_projects, :projects, column: :project_id, on_delete: :cascade add_concurrent_foreign_key :user_interacted_projects, :projects, column: :project_id, on_delete: :cascade
end end
end end
......
...@@ -136,11 +136,14 @@ class MyMigration < ActiveRecord::Migration ...@@ -136,11 +136,14 @@ class MyMigration < ActiveRecord::Migration
disable_ddl_transaction! disable_ddl_transaction!
def up 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
end end
``` ```
Note that it is not necessary to check if the index exists prior to
removing it.
## Adding indexes ## Adding indexes
If you need to add a unique index please keep in mind there is the possibility If you need to add a unique index please keep in mind there is the possibility
......
...@@ -59,6 +59,11 @@ module Gitlab ...@@ -59,6 +59,11 @@ module Gitlab
disable_statement_timeout disable_statement_timeout
end 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) add_index(table_name, column_name, options)
end end
...@@ -83,6 +88,11 @@ module Gitlab ...@@ -83,6 +88,11 @@ module Gitlab
disable_statement_timeout disable_statement_timeout
end 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 })) remove_index(table_name, options.merge({ column: column_name }))
end end
...@@ -107,6 +117,11 @@ module Gitlab ...@@ -107,6 +117,11 @@ module Gitlab
disable_statement_timeout disable_statement_timeout
end 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 })) remove_index(table_name, options.merge({ name: index_name }))
end end
...@@ -140,6 +155,13 @@ module Gitlab ...@@ -140,6 +155,13 @@ module Gitlab
# of PostgreSQL's "VALIDATE CONSTRAINT". As a result we'll just fall # of PostgreSQL's "VALIDATE CONSTRAINT". As a result we'll just fall
# back to the normal foreign key procedure. # back to the normal foreign key procedure.
if Database.mysql? if Database.mysql?
if foreign_key_exists?(source, target, column: column)
Rails.logger.warn "Foreign key not created because it exists already " \
"(this may be due to an aborted migration or similar): " \
"source: #{source}, target: #{target}, column: #{column}"
return
end
return add_foreign_key(source, target, return add_foreign_key(source, target,
column: column, column: column,
on_delete: on_delete) on_delete: on_delete)
...@@ -151,6 +173,11 @@ module Gitlab ...@@ -151,6 +173,11 @@ module Gitlab
key_name = concurrent_foreign_key_name(source, column) key_name = concurrent_foreign_key_name(source, column)
unless foreign_key_exists?(source, target, column: column)
Rails.logger.warn "Foreign key not created because it exists already " \
"(this may be due to an aborted migration or similar): " \
"source: #{source}, target: #{target}, column: #{column}"
# Using NOT VALID allows us to create a key without immediately # Using NOT VALID allows us to create a key without immediately
# validating it. This means we keep the ALTER TABLE lock only for a # validating it. This means we keep the ALTER TABLE lock only for a
# short period of time. The key _is_ enforced for any newly created # short period of time. The key _is_ enforced for any newly created
...@@ -163,13 +190,26 @@ module Gitlab ...@@ -163,13 +190,26 @@ module Gitlab
#{on_delete ? "ON DELETE #{on_delete.upcase}" : ''} #{on_delete ? "ON DELETE #{on_delete.upcase}" : ''}
NOT VALID; NOT VALID;
EOF EOF
end
# Validate the existing constraint. This can potentially take a very # Validate the existing constraint. This can potentially take a very
# long time to complete, but fortunately does not lock the source table # long time to complete, but fortunately does not lock the source table
# while running. # while running.
#
# Note this is a no-op in case the constraint is VALID already
execute("ALTER TABLE #{source} VALIDATE CONSTRAINT #{key_name};") execute("ALTER TABLE #{source} VALIDATE CONSTRAINT #{key_name};")
end end
def foreign_key_exists?(source, target = nil, column: nil)
foreign_keys(source).any? do |key|
if column
key.options[:column].to_s == column.to_s
else
key.to_table.to_s == target.to_s
end
end
end
# Returns the name for a concurrent foreign key. # Returns the name for a concurrent foreign key.
# #
# PostgreSQL constraint names have a limit of 63 bytes. The logic used # PostgreSQL constraint names have a limit of 63 bytes. The logic used
...@@ -860,12 +900,6 @@ into similar problems in the future (e.g. when new tables are created). ...@@ -860,12 +900,6 @@ into similar problems in the future (e.g. when new tables are created).
end end
end end
def foreign_key_exists?(table, column)
foreign_keys(table).any? do |key|
key.options[:column] == column.to_s
end
end
# Rails' index_exists? doesn't work when you only give it a table and index # Rails' index_exists? doesn't work when you only give it a table and index
# name. As such we have to use some extra code to check if an index exists for # name. As such we have to use some extra code to check if an index exists for
# a given name. # a given name.
......
...@@ -67,17 +67,35 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -67,17 +67,35 @@ describe Gitlab::Database::MigrationHelpers do
model.add_concurrent_index(:users, :foo, unique: true) model.add_concurrent_index(:users, :foo, unique: true)
end 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 end
context 'using MySQL' do context 'using MySQL' do
it 'creates a regular index' do before do
expect(Gitlab::Database).to receive(:postgresql?).and_return(false) allow(Gitlab::Database).to receive(:postgresql?).and_return(false)
end
it 'creates a regular index' do
expect(model).to receive(:add_index) expect(model).to receive(:add_index)
.with(:users, :foo, {}) .with(:users, :foo, {})
model.add_concurrent_index(:users, :foo) model.add_concurrent_index(:users, :foo)
end 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
end end
...@@ -95,6 +113,7 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -95,6 +113,7 @@ describe Gitlab::Database::MigrationHelpers do
context 'outside a transaction' do context 'outside a transaction' do
before do before do
allow(model).to receive(:transaction_open?).and_return(false) allow(model).to receive(:transaction_open?).and_return(false)
allow(model).to receive(:index_exists?).and_return(true)
end end
context 'using PostgreSQL' do context 'using PostgreSQL' do
...@@ -103,19 +122,42 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -103,19 +122,42 @@ describe Gitlab::Database::MigrationHelpers do
allow(model).to receive(:disable_statement_timeout) allow(model).to receive(:disable_statement_timeout)
end end
it 'removes the index concurrently by column name' do describe 'by column name' do
it 'removes the index concurrently' do
expect(model).to receive(:remove_index) expect(model).to receive(:remove_index)
.with(:users, { algorithm: :concurrently, column: :foo }) .with(:users, { algorithm: :concurrently, column: :foo })
model.remove_concurrent_index(:users, :foo) model.remove_concurrent_index(:users, :foo)
end 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
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 it 'removes the index concurrently by index name' do
expect(model).to receive(:remove_index) expect(model).to receive(:remove_index)
.with(:users, { algorithm: :concurrently, name: "index_x_by_y" }) .with(:users, { algorithm: :concurrently, name: "index_x_by_y" })
model.remove_concurrent_index_by_name(:users, "index_x_by_y") model.remove_concurrent_index_by_name(:users, "index_x_by_y")
end 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")
end
end
end end
context 'using MySQL' do context 'using MySQL' do
...@@ -141,6 +183,10 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -141,6 +183,10 @@ describe Gitlab::Database::MigrationHelpers do
end end
describe '#add_concurrent_foreign_key' do describe '#add_concurrent_foreign_key' do
before do
allow(model).to receive(:foreign_key_exists?).and_return(false)
end
context 'inside a transaction' do context 'inside a transaction' do
it 'raises an error' do it 'raises an error' do
expect(model).to receive(:transaction_open?).and_return(true) expect(model).to receive(:transaction_open?).and_return(true)
...@@ -157,14 +203,23 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -157,14 +203,23 @@ describe Gitlab::Database::MigrationHelpers do
end end
context 'using MySQL' do context 'using MySQL' do
it 'creates a regular foreign key' do before do
allow(Gitlab::Database).to receive(:mysql?).and_return(true) allow(Gitlab::Database).to receive(:mysql?).and_return(true)
end
it 'creates a regular foreign key' do
expect(model).to receive(:add_foreign_key) expect(model).to receive(:add_foreign_key)
.with(:projects, :users, column: :user_id, on_delete: :cascade) .with(:projects, :users, column: :user_id, on_delete: :cascade)
model.add_concurrent_foreign_key(:projects, :users, column: :user_id) model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
end end
it 'does not create a foreign key if it exists already' do
expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id).and_return(true)
expect(model).not_to receive(:add_foreign_key)
model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
end
end end
context 'using PostgreSQL' do context 'using PostgreSQL' do
...@@ -189,6 +244,14 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -189,6 +244,14 @@ describe Gitlab::Database::MigrationHelpers do
column: :user_id, column: :user_id,
on_delete: :nullify) on_delete: :nullify)
end end
it 'does not create a foreign key if it exists already' do
expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id).and_return(true)
expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/)
expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/)
model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
end
end end
end end
end end
...@@ -203,6 +266,29 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -203,6 +266,29 @@ describe Gitlab::Database::MigrationHelpers do
end end
end end
describe '#foreign_key_exists?' do
before do
key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(:projects, :users, { column: :non_standard_id })
allow(model).to receive(:foreign_keys).with(:projects).and_return([key])
end
it 'finds existing foreign keys by column' do
expect(model.foreign_key_exists?(:projects, :users, column: :non_standard_id)).to be_truthy
end
it 'finds existing foreign keys by target table only' do
expect(model.foreign_key_exists?(:projects, :users)).to be_truthy
end
it 'compares by column name if given' do
expect(model.foreign_key_exists?(:projects, :users, column: :user_id)).to be_falsey
end
it 'compares by target if no column given' do
expect(model.foreign_key_exists?(:projects, :other_table)).to be_falsey
end
end
describe '#disable_statement_timeout' do describe '#disable_statement_timeout' do
context 'using PostgreSQL' do context 'using PostgreSQL' do
it 'disables statement timeouts' do it 'disables statement timeouts' do
......
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