Execute NOT NULL with ADD COLUMN in PG11+ (#39)

* Execute NOT NULL with ADD COLUMN in PG11+

One of the performance improvements in Postgres 11 was executing an `ADD COLUMN ... NOT NULL` without needing a table rewrite. This is now a safe operation in PG11+, and we can avoid adding backwards-compatible safe version with a statement timeout that can fail in some cases.

https://www.postgresql.org/docs/release/11.0/

* Fix variable naming

* Ignore Metrics/PerceivedComplexity for add_column

* Remove unnecessary check

* Add more tests around add_column for older PG

* Refactor add_column to early out for PG11+

This simplifies the if checks enough that we no longer have to ignore the Rubocop complaints about complexity.

Author: katzenbar

lib/safe-pg-migrations/plugins/statement_insurer.rb

@@ -10,10 +10,10 @@ module StatementInsurer
       end
     end
 
-    def add_column(table_name, column_name, type, **options) # rubocop:disable Metrics/CyclomaticComplexity
-      need_default_value_backfill = SafePgMigrations.pg_version_num < PG_11_VERSION_NUM
+    def add_column(table_name, column_name, type, **options)
+      return super if SafePgMigrations.pg_version_num >= PG_11_VERSION_NUM
 
-      default = options.delete(:default) if need_default_value_backfill
+      default = options.delete(:default)
       null = options.delete(:null)
 
       if !default.nil? || null == false
@@ -22,7 +22,7 @@ def add_column(table_name, column_name, type, **options) # rubocop:disable Metri
 
       super
 
-      if need_default_value_backfill && !default.nil?
+      unless default.nil?
         SafePgMigrations.say_method_call(:change_column_default, table_name, column_name, default)
         change_column_default(table_name, column_name, default)
 

test/safe_pg_migrations_test.rb

@@ -120,7 +120,101 @@ def up
     ], calls[1..4].map(&:first)
   end
 
-  def test_add_column_before_pg_11
+  def test_add_column_before_pg_11_without_null_or_default_value
+    @connection.create_table(:users)
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def up
+          add_column(:users, :admin, :boolean)
+        end
+      end.new
+
+    SafePgMigrations.stub(:get_pg_version_num, 96_000) do
+      execute_calls = nil
+      write_calls =
+        record_calls(@migration, :write) do
+          execute_calls = record_calls(@connection, :execute) { run_migration }
+        end
+      assert_calls [
+        # The column is added without any default.
+        'ALTER TABLE "users" ADD "admin" boolean',
+      ], execute_calls
+
+      assert_equal [
+        '== 8128 : migrating ===========================================================',
+        '-- add_column(:users, :admin, :boolean)',
+      ], write_calls.map(&:first)[0...-3]
+    end
+  end
+
+  def test_add_column_before_pg_11_with_null_constraint
+    @connection.create_table(:users)
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def up
+          add_column(:users, :admin, :boolean, null: false)
+        end
+      end.new
+
+    SafePgMigrations.stub(:get_pg_version_num, 96_000) do
+      execute_calls = nil
+      write_calls =
+        record_calls(@migration, :write) do
+          execute_calls = record_calls(@connection, :execute) { run_migration }
+        end
+      assert_calls [
+        # The column is added without any default.
+        'ALTER TABLE "users" ADD "admin" boolean',
+
+        # The not-null constraint is added.
+        "SET statement_timeout TO '5s'",
+        'ALTER TABLE "users" ALTER COLUMN "admin" SET NOT NULL',
+        "SET statement_timeout TO '70s'",
+      ], execute_calls
+
+      assert_equal [
+        '== 8128 : migrating ===========================================================',
+        '-- add_column(:users, :admin, :boolean, {:null=>false})',
+        '   -> add_column("users", :admin, :boolean, {})',
+        '   -> change_column_null("users", :admin, false)',
+      ], write_calls.map(&:first)[0...-3]
+    end
+  end
+
+  def test_add_column_before_pg_11_with_default_value
+    @connection.create_table(:users)
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def up
+          add_column(:users, :admin, :boolean, default: false)
+        end
+      end.new
+
+    SafePgMigrations.stub(:get_pg_version_num, 96_000) do
+      execute_calls = nil
+      write_calls =
+        record_calls(@migration, :write) do
+          execute_calls = record_calls(@connection, :execute) { run_migration }
+        end
+      assert_calls [
+        # The column is added without any default.
+        'ALTER TABLE "users" ADD "admin" boolean',
+
+        # The default is added.
+        'ALTER TABLE "users" ALTER COLUMN "admin" SET DEFAULT FALSE',
+      ], execute_calls
+
+      assert_equal [
+        '== 8128 : migrating ===========================================================',
+        '-- add_column(:users, :admin, :boolean, {:default=>false})',
+        '   -> add_column("users", :admin, :boolean, {})',
+        '   -> change_column_default("users", :admin, false)',
+        '   -> backfill_column_default("users", :admin)',
+      ], write_calls.map(&:first)[0...-3]
+    end
+  end
+
+  def test_add_column_before_pg_11_with_null_and_default_value
     @connection.create_table(:users)
     @migration =
       Class.new(ActiveRecord::Migration::Current) do
@@ -175,20 +269,13 @@ def up
           execute_calls = record_calls(@connection, :execute) { run_migration }
         end
       assert_calls [
-        # The column is added with the default without any trick
-        'ALTER TABLE "users" ADD "admin" boolean DEFAULT FALSE',
-
-        # The not-null constraint is added.
-        "SET statement_timeout TO '5s'",
-        'ALTER TABLE "users" ALTER COLUMN "admin" SET NOT NULL',
-        "SET statement_timeout TO '70s'",
+        # The column is added with the default and not null constraint without any tricks
+        'ALTER TABLE "users" ADD "admin" boolean DEFAULT FALSE NOT NULL',
       ], execute_calls
 
       assert_equal [
         '== 8128 : migrating ===========================================================',
         '-- add_column(:users, :admin, :boolean, {:default=>false, :null=>false})',
-        '   -> add_column("users", :admin, :boolean, {:default=>false})',
-        '   -> change_column_null("users", :admin, false)',
       ], write_calls.map(&:first)[0...-3]
     end
   end