Merge branch 'concurrent-foreign-keys' into 'master'

Add method for creating foreign keys concurrently

See merge request !9069

8 files changed, 183 insertions, 10 deletions

diff --git a/db/migrate/20160919145149_add_group_id_to_labels.rb b/db/migrate/20160919145149_add_group_id_to_labels.rb
index 05e21af0584..d10f3a6d104 100644
--- a/db/migrate/20160919145149_add_group_id_to_labels.rb
+++ b/db/migrate/20160919145149_add_group_id_to_labels.rb
@@ -7,7 +7,7 @@ class AddGroupIdToLabels < ActiveRecord::Migration
 
   def change
     add_column :labels, :group_id, :integer
-    add_foreign_key :labels, :namespaces, column: :group_id, on_delete: :cascade
+    add_foreign_key :labels, :namespaces, column: :group_id, on_delete: :cascade # rubocop: disable Migration/AddConcurrentForeignKey
     add_concurrent_index :labels, :group_id
   end
 end
diff --git a/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb b/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb
index f49df6802a7..2abfe47b776 100644
--- a/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb
+++ b/db/migrate/20161020083353_add_pipeline_id_to_merge_request_metrics.rb
@@ -28,6 +28,6 @@ class AddPipelineIdToMergeRequestMetrics < ActiveRecord::Migration
   def change
     add_column :merge_request_metrics, :pipeline_id, :integer
     add_concurrent_index :merge_request_metrics, :pipeline_id
-    add_foreign_key :merge_request_metrics, :ci_commits, column: :pipeline_id, on_delete: :cascade
+    add_foreign_key :merge_request_metrics, :ci_commits, column: :pipeline_id, on_delete: :cascade # rubocop: disable Migration/AddConcurrentForeignKey
   end
 end
diff --git a/db/migrate/20161031171301_add_project_id_to_subscriptions.rb b/db/migrate/20161031171301_add_project_id_to_subscriptions.rb
index 97534679b59..d5c343dc527 100644
--- a/db/migrate/20161031171301_add_project_id_to_subscriptions.rb
+++ b/db/migrate/20161031171301_add_project_id_to_subscriptions.rb
@@ -5,7 +5,7 @@ class AddProjectIdToSubscriptions < ActiveRecord::Migration
 
   def up
     add_column :subscriptions, :project_id, :integer
-    add_foreign_key :subscriptions, :projects, column: :project_id, on_delete: :cascade
+    add_foreign_key :subscriptions, :projects, column: :project_id, on_delete: :cascade # rubocop: disable Migration/AddConcurrentForeignKey
   end
 
   def down
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 0bd6e148ba8..4800a509b37 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -26,11 +26,59 @@ module Gitlab
         add_index(table_name, column_name, options)
       end
 
+      # Adds a foreign key with only minimal locking on the tables involved.
+      #
+      # This method only requires minimal locking when using PostgreSQL. When
+      # using MySQL this method will use Rails' default `add_foreign_key`.
+      #
+      # source - The source table containing the foreign key.
+      # target - The target table the key points to.
+      # column - The name of the column to create the foreign key on.
+      # on_delete - The action to perform when associated data is removed,
+      #             defaults to "CASCADE".
+      def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade)
+        # Transactions would result in ALTER TABLE locks being held for the
+        # duration of the transaction, defeating the purpose of this method.
+        if transaction_open?
+          raise 'add_concurrent_foreign_key can not be run inside a transaction'
+        end
+
+        # While MySQL does allow disabling of foreign keys it has no equivalent
+        # of PostgreSQL's "VALIDATE CONSTRAINT". As a result we'll just fall
+        # back to the normal foreign key procedure.
+        if Database.mysql?
+          return add_foreign_key(source, target,
+                                 column: column,
+                                 on_delete: on_delete)
+        end
+
+        disable_statement_timeout
+
+        key_name = "fk_#{source}_#{target}_#{column}"
+
+        # Using NOT VALID allows us to create a key without immediately
+        # 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
+        # data.
+        execute <<-EOF.strip_heredoc
+        ALTER TABLE #{source}
+        ADD CONSTRAINT #{key_name}
+        FOREIGN KEY (#{column})
+        REFERENCES #{target} (id)
+        ON DELETE #{on_delete} NOT VALID;
+        EOF
+
+        # Validate the existing constraint. This can potentially take a very
+        # long time to complete, but fortunately does not lock the source table
+        # while running.
+        execute("ALTER TABLE #{source} VALIDATE CONSTRAINT #{key_name};")
+      end
+
       # Long-running migrations may take more than the timeout allowed by
       # the database. Disable the session's statement timeout to ensure
       # migrations don't get killed prematurely. (PostgreSQL only)
       def disable_statement_timeout
-        ActiveRecord::Base.connection.execute('SET statement_timeout TO 0') if Database.postgresql?
+        execute('SET statement_timeout TO 0') if Database.postgresql?
       end
 
       # Updates the value of a column in batches.
diff --git a/rubocop/cop/migration/add_concurrent_foreign_key.rb b/rubocop/cop/migration/add_concurrent_foreign_key.rb
new file mode 100644
index 00000000000..e40a7087a47
--- /dev/null
+++ b/rubocop/cop/migration/add_concurrent_foreign_key.rb
@@ -0,0 +1,27 @@
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if `add_concurrent_foreign_key` is used instead of
+      # `add_foreign_key`.
+      class AddConcurrentForeignKey < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = '`add_foreign_key` requires downtime, use `add_concurrent_foreign_key` instead'
+
+        def on_send(node)
+          return unless in_migration?(node)
+
+          name = node.children[1]
+
+          add_offense(node, :selector) if name == :add_foreign_key
+        end
+
+        def method_name(node)
+          node.children.first
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index aa35fb1701c..d4266d0deae 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -1,4 +1,5 @@
 require_relative 'cop/gem_fetcher'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_column_with_default'
+require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_index'
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 7fd25b9e5bf..e94ca4fcfd2 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -12,15 +12,14 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
   describe '#add_concurrent_index' do
     context 'outside a transaction' do
       before do
-        expect(model).to receive(:transaction_open?).and_return(false)
-
-        unless Gitlab::Database.postgresql?
-          allow_any_instance_of(Gitlab::Database::MigrationHelpers).to receive(:disable_statement_timeout)
-        end
+        allow(model).to receive(:transaction_open?).and_return(false)
       end
 
       context 'using PostgreSQL' do
-        before { expect(Gitlab::Database).to receive(:postgresql?).and_return(true) }
+        before do
+          allow(Gitlab::Database).to receive(:postgresql?).and_return(true)
+          allow(model).to receive(:disable_statement_timeout)
+        end
 
         it 'creates the index concurrently' do
           expect(model).to receive(:add_index).
@@ -59,6 +58,71 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
     end
   end
 
+  describe '#add_concurrent_foreign_key' do
+    context 'inside a transaction' do
+      it 'raises an error' do
+        expect(model).to receive(:transaction_open?).and_return(true)
+
+        expect do
+          model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
+        end.to raise_error(RuntimeError)
+      end
+    end
+
+    context 'outside a transaction' do
+      before do
+        allow(model).to receive(:transaction_open?).and_return(false)
+      end
+
+      context 'using MySQL' do
+        it 'creates a regular foreign key' do
+          allow(Gitlab::Database).to receive(:mysql?).and_return(true)
+
+          expect(model).to receive(:add_foreign_key).
+            with(:projects, :users, column: :user_id, on_delete: :cascade)
+
+          model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
+        end
+      end
+
+      context 'using PostgreSQL' do
+        before do
+          allow(Gitlab::Database).to receive(:mysql?).and_return(false)
+        end
+
+        it 'creates a concurrent foreign key' do
+          expect(model).to receive(:disable_statement_timeout)
+          expect(model).to receive(:execute).ordered.with(/NOT VALID/)
+          expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
+
+          model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
+        end
+      end
+    end
+  end
+
+  describe '#disable_statement_timeout' do
+    context 'using PostgreSQL' do
+      it 'disables statement timeouts' do
+        expect(Gitlab::Database).to receive(:postgresql?).and_return(true)
+
+        expect(model).to receive(:execute).with('SET statement_timeout TO 0')
+
+        model.disable_statement_timeout
+      end
+    end
+
+    context 'using MySQL' do
+      it 'does nothing' do
+        expect(Gitlab::Database).to receive(:postgresql?).and_return(false)
+
+        expect(model).not_to receive(:execute)
+
+        model.disable_statement_timeout
+      end
+    end
+  end
+
   describe '#update_column_in_batches' do
     before do
       create_list(:empty_project, 5)
diff --git a/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb b/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb
new file mode 100644
index 00000000000..7cb24dc5646
--- /dev/null
+++ b/spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/migration/add_concurrent_foreign_key'
+
+describe RuboCop::Cop::Migration::AddConcurrentForeignKey do
+  include CopHelper
+
+  let(:cop) { described_class.new }
+
+  context 'outside of a migration' do
+    it 'does not register any offenses' do
+      inspect_source(cop, 'def up; add_foreign_key(:projects, :users, column: :user_id); end')
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'in a migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when using add_foreign_key' do
+      inspect_source(cop, 'def up; add_foreign_key(:projects, :users, column: :user_id); end')
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+  end
+end