Remove varchar length limits leftover in databases that were migrateidremove-all-varchar-limits

from MySQL in the past. This means altering any tables that have
varchar(255) or varchar(510) to just be plain varchar. This brings
these databases closer into sync with the bundled schema.rb.

This will not require rewriting any tables and will not require
scanning the entirety of any tables so it should be very
fast.

Nonetheless it does require an exclusive lock on each affected table
(one at a time, not all at once) so on a very busy database it could
cause a short period of lack of responsiveness on the database. It's
important as a result that this run at a time when no long-running
background migrations or batch queries are running on the database.

4 files changed, 75 insertions, 1 deletions

diff --git a/changelogs/unreleased/remove-all-varchar-limits.yml b/changelogs/unreleased/remove-all-varchar-limits.yml
new file mode 100644
index 00000000000..1a0aeafbd64
--- /dev/null
+++ b/changelogs/unreleased/remove-all-varchar-limits.yml
@@ -0,0 +1,8 @@
+---
+title: Remove varchar length limits leftover in databases that were migrateid from
+  MySQL in the past. This means altering any tables that have varchar(255) or varchar(510)
+  to just be plain varchar. This brings these databases closer into sync with the
+  bundled schema.rb.
+merge_request:
+author:
+type: other
diff --git a/db/migrate/20170919220540_remove_varchar_limits.rb b/db/migrate/20170919220540_remove_varchar_limits.rb
new file mode 100644
index 00000000000..df352a97be9
--- /dev/null
+++ b/db/migrate/20170919220540_remove_varchar_limits.rb
@@ -0,0 +1,57 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveVarcharLimits < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  # Because ALTER TABLE locks the table, even briefly, and we're doing
+  # this on many tables in an arbitrary order there would be a severe
+  # risk of a deadlock if we did it all in a single transaction. The
+  # only way to avoid it would be to ensure we did it in the same
+  # order that every other transaction accesses these tables which
+  # would be impossible.
+  disable_ddl_transaction!
+
+  # We're removing varchar(510) and varchar(255) limits which are a
+  # relic of the MySQL->Postgres migration in the past...
+  # c.f. https://github.com/gitlabhq/mysql-postgresql-converter/issues/8
+
+  # Note that doing ALTER COLUMN to go from varchar(xxx) to plain
+  # varchar does not need to rewrite the table or even do a full table
+  # scan to verify the constraint. It does require obtaining a lock on
+  # the table so on very busy databases it could cause a short
+  # outage. Doing this in separate transactions should minimize this.
+
+  def up
+    return unless Gitlab::Database.postgresql?
+
+    connection.tables.each do |table|
+      next unless gitlab_table?(table)
+
+      varchars = columns(table).select { |col| old_mysql_varchar?(col) }
+      next if varchars.empty?
+
+      # Do a single change_table to alter column on all the columns in
+      # a table together to minimize the number of lock acquisitions
+      say "Removing #{varchars.length} leftover varchar limits from MySQL migration from table #{table}"
+      change_table table do |t|
+        varchars.each do |c|
+          t.change c.name, :string
+        end
+      end
+    end
+  end
+
+  # No down method since these limits ought never have been present
+
+  def gitlab_table?(table)
+    return table != 'schema_migrations'
+  end
+
+  def old_mysql_varchar?(col)
+    return (col.cast_type.is_a?(ActiveRecord::Type::String) &&
+            (col.cast_type.limit == 510 || col.cast_type.limit == 255))
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 3ec430c0078..6f3f5c4a5da 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170918223303) do
+ActiveRecord::Schema.define(version: 20170919220540) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/spec/migrations/active_record/schema_spec.rb b/spec/migrations/active_record/schema_spec.rb
index e132529d8d8..3a2ecbf26eb 100644
--- a/spec/migrations/active_record/schema_spec.rb
+++ b/spec/migrations/active_record/schema_spec.rb
@@ -20,4 +20,13 @@ describe ActiveRecord::Schema do
   it '> schema version should equal the latest migration timestamp stored in schema_migrations table' do
     expect(latest_migration_timestamp).to eq(ActiveRecord::Migrator.current_version.to_i)
   end
+
+  it '> schema does not contain any varchar(255) leftovers from MySQL' do
+    return unless ActiveRecord::Base.configurations[Rails.env]['adapter'] =~ /^postgresql/
+    number_of_broken_columns = File.open(Rails.root.join('db', 'schema.rb')) do |file|
+      file.find_all { |line| line =~ /limit: (255|510)/ }
+    end.length
+
+    expect(number_of_broken_columns).to eq(0)
+  end
 end