Merge branch...

Merge branch '38358-update-migration-helpers-to-use-check-constraints-instead-of-change-column-null' into 'master'

Update migration helpers to use check constraints instead of change_column_null

See merge request gitlab-org/gitlab!30943

doc/development/what_requires_downtime.md

@@ -171,8 +171,39 @@ Adding or removing a NOT NULL clause (or another constraint) can typically be
 done without requiring downtime. However, this does require that any application
 changes are deployed _first_. Thus, changing the constraints of a column should
 happen in a post-deployment migration.
-NOTE: Avoid using `change_column` as it produces inefficient query because it re-defines
-the whole column type. For example, to add a NOT NULL constraint, prefer `change_column_null`
+
+NOTE: Avoid using `change_column` as it produces an inefficient query because it re-defines
+the whole column type.
+
+To add a NOT NULL constraint, use the `add_not_null_constraint` migration helper:
+
+```ruby
+# A post-deployment migration in db/post_migrate
+class AddNotNull < ActiveRecord::Migration[4.2]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint :users, :username
+  end
+
+  def down
+    remove_not_null_constraint :users, :username
+  end
+end
+```
+
+If the column to be updated requires cleaning first (e.g. there are `NULL` values), you should:
+
+1. Add the `NOT NULL` constraint with `validate: false`
+
+   `add_not_null_constraint :users, :username, validate: false`
+
+1. Clean up the data with a data migration
+1. Validate the `NOT NULL` constraint with a followup migration
+
+   `validate_not_null_constraint :users, :username`
 
 ## Changing Column Types
 

lib/gitlab/database/migration_helpers.rb

@@ -265,6 +265,12 @@ module Gitlab
       # or `RESET ALL` is executed
       def disable_statement_timeout
         if block_given?
+          if statement_timeout_disabled?
+            # Don't do anything if the statement_timeout is already disabled
+            # Allows for nested calls of disable_statement_timeout without
+            # resetting the timeout too early (before the outer call ends)
+            yield
+          else
             begin
               execute('SET statement_timeout TO 0')
 
@@ -272,6 +278,7 @@ module Gitlab
             ensure
               execute('RESET ALL')
             end
+          end
         else
           unless transaction_open?
             raise <<~ERROR
@@ -495,7 +502,7 @@ module Gitlab
               update_column_in_batches(table, column, default_after_type_cast, &block)
             end
 
-            change_column_null(table, column, false) unless allow_null
+            add_not_null_constraint(table, column) unless allow_null
           # We want to rescue _all_ exceptions here, even those that don't inherit
           # from StandardError.
           rescue Exception => error # rubocop: disable all
@@ -1334,12 +1341,73 @@ into similar problems in the future (e.g. when new tables are created).
         check_constraint_exists?(table, text_limit_name(table, column, name: constraint_name))
       end
 
+      # Migration Helpers for managing not null constraints
+      def add_not_null_constraint(table, column, constraint_name: nil, validate: true)
+        if column_is_nullable?(table, column)
+          add_check_constraint(
+            table,
+            "#{column} IS NOT NULL",
+            not_null_constraint_name(table, column, name: constraint_name),
+            validate: validate
+          )
+        else
+          warning_message = <<~MESSAGE
+            NOT NULL check constraint was not created:
+            column #{table}.#{column} is already defined as `NOT NULL`
+          MESSAGE
+
+          Rails.logger.warn warning_message
+        end
+      end
+
+      def validate_not_null_constraint(table, column, constraint_name: nil)
+        validate_check_constraint(
+          table,
+          not_null_constraint_name(table, column, name: constraint_name)
+        )
+      end
+
+      def remove_not_null_constraint(table, column, constraint_name: nil)
+        remove_check_constraint(
+          table,
+          not_null_constraint_name(table, column, name: constraint_name)
+        )
+      end
+
+      def check_not_null_constraint_exists?(table, column, constraint_name: nil)
+        check_constraint_exists?(
+          table,
+          not_null_constraint_name(table, column, name: constraint_name)
+        )
+      end
+
       private
 
+      def statement_timeout_disabled?
+        # This is a string of the form "100ms" or "0" when disabled
+        connection.select_value('SHOW statement_timeout') == "0"
+      end
+
+      def column_is_nullable?(table, column)
+        # Check if table.column has not been defined with NOT NULL
+        check_sql = <<~SQL
+          SELECT c.is_nullable
+          FROM information_schema.columns c
+          WHERE c.table_name = '#{table}'
+          AND c.column_name = '#{column}'
+        SQL
+
+        connection.select_value(check_sql) == 'YES'
+      end
+
       def text_limit_name(table, column, name: nil)
         name.presence || check_constraint_name(table, column, 'max_length')
       end
 
+      def not_null_constraint_name(table, column, name: nil)
+        name.presence || check_constraint_name(table, column, 'not_null')
+      end
+
       def missing_schema_object_message(table, type, name)
         <<~MESSAGE
           Could not find #{type} "#{name}" on table "#{table}" which was referenced during the migration.
@@ -1383,7 +1451,7 @@ into similar problems in the future (e.g. when new tables are created).
 
         update_column_in_batches(table, new, Arel::Table.new(table)[old], batch_column_name: batch_column_name)
 
-        change_column_null(table, new, false) unless old_col.null
+        add_not_null_constraint(table, new) unless old_col.null
 
         copy_indexes(table, old, new)
         copy_foreign_keys(table, old, new)

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -217,9 +217,10 @@ describe Gitlab::Database::MigrationHelpers do
           it 'appends ON DELETE SET NULL statement' do
             expect(model).to receive(:with_lock_retries).and_call_original
             expect(model).to receive(:disable_statement_timeout).and_call_original
+            expect(model).to receive(:statement_timeout_disabled?).and_return(false)
             expect(model).to receive(:execute).with(/statement_timeout/)
             expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-            expect(model).to receive(:execute).with(/RESET ALL/)
+            expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
             expect(model).to receive(:execute).with(/ON DELETE SET NULL/)
 
@@ -233,9 +234,10 @@ describe Gitlab::Database::MigrationHelpers do
           it 'appends ON DELETE CASCADE statement' do
             expect(model).to receive(:with_lock_retries).and_call_original
             expect(model).to receive(:disable_statement_timeout).and_call_original
+            expect(model).to receive(:statement_timeout_disabled?).and_return(false)
             expect(model).to receive(:execute).with(/statement_timeout/)
             expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-            expect(model).to receive(:execute).with(/RESET ALL/)
+            expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
             expect(model).to receive(:execute).with(/ON DELETE CASCADE/)
 
@@ -249,9 +251,10 @@ describe Gitlab::Database::MigrationHelpers do
           it 'appends no ON DELETE statement' do
             expect(model).to receive(:with_lock_retries).and_call_original
             expect(model).to receive(:disable_statement_timeout).and_call_original
+            expect(model).to receive(:statement_timeout_disabled?).and_return(false)
             expect(model).to receive(:execute).with(/statement_timeout/)
             expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-            expect(model).to receive(:execute).with(/RESET ALL/)
+            expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
             expect(model).not_to receive(:execute).with(/ON DELETE/)
 
@@ -266,10 +269,11 @@ describe Gitlab::Database::MigrationHelpers do
         it 'creates a concurrent foreign key and validates it' do
           expect(model).to receive(:with_lock_retries).and_call_original
           expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:statement_timeout_disabled?).and_return(false)
           expect(model).to receive(:execute).with(/statement_timeout/)
           expect(model).to receive(:execute).ordered.with(/NOT VALID/)
           expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-          expect(model).to receive(:execute).with(/RESET ALL/)
+          expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
           model.add_concurrent_foreign_key(:projects, :users, column: :user_id)
         end
@@ -293,10 +297,11 @@ describe Gitlab::Database::MigrationHelpers do
           it 'creates a new foreign key' do
             expect(model).to receive(:with_lock_retries).and_call_original
             expect(model).to receive(:disable_statement_timeout).and_call_original
+            expect(model).to receive(:statement_timeout_disabled?).and_return(false)
             expect(model).to receive(:execute).with(/statement_timeout/)
             expect(model).to receive(:execute).ordered.with(/NOT VALID/)
             expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+foo/)
-            expect(model).to receive(:execute).with(/RESET ALL/)
+            expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
             model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :foo)
           end
@@ -321,10 +326,11 @@ describe Gitlab::Database::MigrationHelpers do
             it 'creates a new foreign key' do
               expect(model).to receive(:with_lock_retries).and_call_original
               expect(model).to receive(:disable_statement_timeout).and_call_original
+              expect(model).to receive(:statement_timeout_disabled?).and_return(false)
               expect(model).to receive(:execute).with(/statement_timeout/)
               expect(model).to receive(:execute).ordered.with(/NOT VALID/)
               expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT.+bar/)
-              expect(model).to receive(:execute).with(/RESET ALL/)
+              expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
               model.add_concurrent_foreign_key(:projects, :users, column: :user_id, name: :bar)
             end
@@ -361,6 +367,7 @@ describe Gitlab::Database::MigrationHelpers do
         aggregate_failures do
           expect(model).not_to receive(:concurrent_foreign_key_name)
           expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:statement_timeout_disabled?).and_return(false)
           expect(model).to receive(:execute).with(/statement_timeout/)
           expect(model).to receive(:execute).ordered.with(/ALTER TABLE projects VALIDATE CONSTRAINT/)
           expect(model).to receive(:execute).ordered.with(/RESET ALL/)
@@ -377,6 +384,7 @@ describe Gitlab::Database::MigrationHelpers do
         aggregate_failures do
           expect(model).to receive(:concurrent_foreign_key_name)
           expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:statement_timeout_disabled?).and_return(false)
           expect(model).to receive(:execute).with(/statement_timeout/)
           expect(model).to receive(:execute).ordered.with(/ALTER TABLE projects VALIDATE CONSTRAINT/)
           expect(model).to receive(:execute).ordered.with(/RESET ALL/)
@@ -527,6 +535,26 @@ describe Gitlab::Database::MigrationHelpers do
         end
       end
     end
+
+    # This spec runs without an enclosing transaction (:delete truncation method for db_cleaner)
+    context 'when the statement_timeout is already disabled', :delete do
+      before do
+        ActiveRecord::Base.connection.execute('SET statement_timeout TO 0')
+      end
+
+      after do
+        # Use ActiveRecord::Base.connection instead of model.execute
+        # so that this call is not counted below
+        ActiveRecord::Base.connection.execute('RESET ALL')
+      end
+
+      it 'yields control without disabling the timeout or resetting' do
+        expect(model).not_to receive(:execute).with('SET statement_timeout TO 0')
+        expect(model).not_to receive(:execute).with('RESET ALL')
+
+        expect { |block| model.disable_statement_timeout(&block) }.to yield_control
+      end
+    end
   end
 
   describe '#true_value' do
@@ -619,7 +647,7 @@ describe Gitlab::Database::MigrationHelpers do
           expect(model).to receive(:update_column_in_batches)
             .with(:projects, :foo, 10)
 
-          expect(model).not_to receive(:change_column_null)
+          expect(model).not_to receive(:add_not_null_constraint)
 
           model.add_column_with_default(:projects, :foo, :integer,
                                         default: 10,
@@ -630,8 +658,8 @@ describe Gitlab::Database::MigrationHelpers do
           expect(model).to receive(:update_column_in_batches)
             .with(:projects, :foo, 10)
 
-          expect(model).to receive(:change_column_null)
-            .with(:projects, :foo, false)
+          expect(model).to receive(:add_not_null_constraint)
+            .with(:projects, :foo)
 
           model.add_column_with_default(:projects, :foo, :integer, default: 10)
         end
@@ -650,16 +678,16 @@ describe Gitlab::Database::MigrationHelpers do
         end
 
         it 'removes the added column whenever changing a column NULL constraint fails' do
-          expect(model).to receive(:change_column_null)
-            .with(:projects, :foo, false)
-            .and_raise(RuntimeError)
+          expect(model).to receive(:add_not_null_constraint)
+            .with(:projects, :foo)
+            .and_raise(ActiveRecord::ActiveRecordError)
 
           expect(model).to receive(:remove_column)
             .with(:projects, :foo)
 
           expect do
             model.add_column_with_default(:projects, :foo, :integer, default: 10)
-          end.to raise_error(RuntimeError)
+          end.to raise_error(ActiveRecord::ActiveRecordError)
         end
       end
 
@@ -671,7 +699,7 @@ describe Gitlab::Database::MigrationHelpers do
           allow(model).to receive(:transaction).and_yield
           allow(model).to receive(:column_for).with(:user_details, :foo).and_return(column)
           allow(model).to receive(:update_column_in_batches).with(:user_details, :foo, 10, batch_column_name: :user_id)
-          allow(model).to receive(:change_column_null).with(:user_details, :foo, false)
+          allow(model).to receive(:add_not_null_constraint).with(:user_details, :foo)
           allow(model).to receive(:change_column_default).with(:user_details, :foo, 10)
 
           expect(model).to receive(:add_column)
@@ -693,7 +721,7 @@ describe Gitlab::Database::MigrationHelpers do
           allow(model).to receive(:transaction).and_yield
           allow(model).to receive(:column_for).with(:projects, :foo).and_return(column)
           allow(model).to receive(:update_column_in_batches).with(:projects, :foo, 10)
-          allow(model).to receive(:change_column_null).with(:projects, :foo, false)
+          allow(model).to receive(:add_not_null_constraint).with(:projects, :foo)
           allow(model).to receive(:change_column_default).with(:projects, :foo, 10)
 
           expect(model).to receive(:add_column)
@@ -782,7 +810,7 @@ describe Gitlab::Database::MigrationHelpers do
 
           expect(model).to receive(:update_column_in_batches)
 
-          expect(model).to receive(:change_column_null).with(:users, :new, false)
+          expect(model).to receive(:add_not_null_constraint).with(:users, :new)
 
           expect(model).to receive(:copy_indexes).with(:users, :old, :new)
           expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new)
@@ -915,7 +943,7 @@ describe Gitlab::Database::MigrationHelpers do
 
         expect(model).to receive(:update_column_in_batches)
 
-        expect(model).to receive(:change_column_null).with(:users, :old, false)
+        expect(model).to receive(:add_not_null_constraint).with(:users, :old)
 
         expect(model).to receive(:copy_indexes).with(:users, :new, :old)
         expect(model).to receive(:copy_foreign_keys).with(:users, :new, :old)
@@ -2225,6 +2253,7 @@ describe Gitlab::Database::MigrationHelpers do
                        .and_return(false).exactly(1)
 
           expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:statement_timeout_disabled?).and_return(false)
           expect(model).to receive(:execute).with(/statement_timeout/)
           expect(model).to receive(:with_lock_retries).and_call_original
           expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/)
@@ -2268,6 +2297,7 @@ describe Gitlab::Database::MigrationHelpers do
                        .and_return(false).exactly(1)
 
           expect(model).to receive(:disable_statement_timeout).and_call_original
+          expect(model).to receive(:statement_timeout_disabled?).and_return(false)
           expect(model).to receive(:execute).with(/statement_timeout/)
           expect(model).to receive(:with_lock_retries).and_call_original
           expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/)
@@ -2309,6 +2339,7 @@ describe Gitlab::Database::MigrationHelpers do
 
         expect(model).to receive(:check_constraint_exists?).and_return(true)
         expect(model).to receive(:disable_statement_timeout).and_call_original
+        expect(model).to receive(:statement_timeout_disabled?).and_return(false)
         expect(model).to receive(:execute).with(/statement_timeout/)
         expect(model).to receive(:execute).ordered.with(validate_sql)
         expect(model).to receive(:execute).ordered.with(/RESET ALL/)
@@ -2448,4 +2479,135 @@ describe Gitlab::Database::MigrationHelpers do
       end
     end
   end
+
+  describe '#add_not_null_constraint' do
+    context 'when it is called with the default options' do
+      it 'calls add_check_constraint with an infered constraint name and validate: true' do
+        constraint_name = model.check_constraint_name(:test_table,
+                                                      :name,
+                                                      'not_null')
+        check = "name IS NOT NULL"
+
+        expect(model).to receive(:column_is_nullable?).and_return(true)
+        expect(model).to receive(:check_constraint_name).and_call_original
+        expect(model).to receive(:add_check_constraint)
+                     .with(:test_table, check, constraint_name, validate: true)
+
+        model.add_not_null_constraint(:test_table, :name)
+      end
+    end
+
+    context 'when all parameters are provided' do
+      it 'calls add_check_constraint with the correct parameters' do
+        constraint_name = 'check_name_not_null'
+        check = "name IS NOT NULL"
+
+        expect(model).to receive(:column_is_nullable?).and_return(true)
+        expect(model).not_to receive(:check_constraint_name)
+        expect(model).to receive(:add_check_constraint)
+                     .with(:test_table, check, constraint_name, validate: false)
+
+        model.add_not_null_constraint(
+          :test_table,
+          :name,
+          constraint_name: constraint_name,
+          validate: false
+        )
+      end
+    end
+
+    context 'when the column is defined as NOT NULL' do
+      it 'does not add a check constraint' do
+        expect(model).to receive(:column_is_nullable?).and_return(false)
+        expect(model).not_to receive(:check_constraint_name)
+        expect(model).not_to receive(:add_check_constraint)
+
+        model.add_not_null_constraint(:test_table, :name)
+      end
+    end
+  end
+
+  describe '#validate_not_null_constraint' do
+    context 'when constraint_name is not provided' do
+      it 'calls validate_check_constraint with an infered constraint name' do
+        constraint_name = model.check_constraint_name(:test_table,
+                                                      :name,
+                                                      'not_null')
+
+        expect(model).to receive(:check_constraint_name).and_call_original
+        expect(model).to receive(:validate_check_constraint)
+                     .with(:test_table, constraint_name)
+
+        model.validate_not_null_constraint(:test_table, :name)
+      end
+    end
+
+    context 'when constraint_name is provided' do
+      it 'calls validate_check_constraint with the correct parameters' do
+        constraint_name = 'check_name_not_null'
+
+        expect(model).not_to receive(:check_constraint_name)
+        expect(model).to receive(:validate_check_constraint)
+                     .with(:test_table, constraint_name)
+
+        model.validate_not_null_constraint(:test_table, :name, constraint_name: constraint_name)
+      end
+    end
+  end
+
+  describe '#remove_not_null_constraint' do
+    context 'when constraint_name is not provided' do
+      it 'calls remove_check_constraint with an infered constraint name' do
+        constraint_name = model.check_constraint_name(:test_table,
+                                                      :name,
+                                                      'not_null')
+
+        expect(model).to receive(:check_constraint_name).and_call_original
+        expect(model).to receive(:remove_check_constraint)
+                     .with(:test_table, constraint_name)
+
+        model.remove_not_null_constraint(:test_table, :name)
+      end
+    end
+
+    context 'when constraint_name is provided' do
+      it 'calls remove_check_constraint with the correct parameters' do
+        constraint_name = 'check_name_not_null'
+
+        expect(model).not_to receive(:check_constraint_name)
+        expect(model).to receive(:remove_check_constraint)
+                     .with(:test_table, constraint_name)
+
+        model.remove_not_null_constraint(:test_table, :name, constraint_name: constraint_name)
+      end
+    end
+  end
+
+  describe '#check_not_null_constraint_exists?' do
+    context 'when constraint_name is not provided' do
+      it 'calls check_constraint_exists? with an infered constraint name' do
+        constraint_name = model.check_constraint_name(:test_table,
+                                                      :name,
+                                                      'not_null')
+
+        expect(model).to receive(:check_constraint_name).and_call_original
+        expect(model).to receive(:check_constraint_exists?)
+                     .with(:test_table, constraint_name)
+
+        model.check_not_null_constraint_exists?(:test_table, :name)
+      end
+    end
+
+    context 'when constraint_name is provided' do
+      it 'calls check_constraint_exists? with the correct parameters' do
+        constraint_name = 'check_name_not_null'
+
+        expect(model).not_to receive(:check_constraint_name)
+        expect(model).to receive(:check_constraint_exists?)
+                     .with(:test_table, constraint_name)
+
+        model.check_not_null_constraint_exists?(:test_table, :name, constraint_name: constraint_name)
+      end
+    end
+  end
 end

spec/support/shared_examples/lib/gitlab/migration_helpers_shared_examples.rb

@@ -13,10 +13,11 @@ end
 RSpec.shared_examples 'performs validation' do |validation_option|
   it 'performs validation' do
     expect(model).to receive(:disable_statement_timeout).and_call_original
+    expect(model).to receive(:statement_timeout_disabled?).and_return(false)
     expect(model).to receive(:execute).with(/statement_timeout/)
     expect(model).to receive(:execute).ordered.with(/NOT VALID/)
     expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/)
-    expect(model).to receive(:execute).with(/RESET ALL/)
+    expect(model).to receive(:execute).ordered.with(/RESET ALL/)
 
     model.add_concurrent_foreign_key(*args, **options.merge(validation_option))
   end