From e867099091e19e8cae47d928761b869ec20db2c3 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 9 May 2016 14:14:32 +0200 Subject: [PATCH 1/9] Added helper methods for database migrations These helpers can be used to perform migrations without taking down the entire application. For example, the method "add_column_with_default" can be used to add a new column with a default value without locking the entire table. --- lib/gitlab/database/migration_helpers.rb | 132 ++++++++++++++++++ .../gitlab/database/migration_helpers_spec.rb | 124 ++++++++++++++++ 2 files changed, 256 insertions(+) create mode 100644 lib/gitlab/database/migration_helpers.rb create mode 100644 spec/lib/gitlab/database/migration_helpers_spec.rb diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb new file mode 100644 index 0000000000..7aa8c90e6d --- /dev/null +++ b/lib/gitlab/database/migration_helpers.rb @@ -0,0 +1,132 @@ +module Gitlab + module Database + module MigrationHelpers + # Creates a new index, concurrently when supported + # + # On PostgreSQL this method creates an index concurrently, on MySQL this + # creates a regular index. + # + # Example: + # + # add_concurrent_index :users, :some_column + # + # See Rails' `add_index` for more info on the available arguments. + def add_concurrent_index(*args) + if transaction_open? + raise 'add_concurrent_index can not be run inside a transaction, ' \ + 'you can disable transactions by calling disable_ddl_transaction! ' \ + 'in the body of your migration class' + end + + if Database.postgresql? + args << { algorithm: :concurrently } + end + + add_index(*args) + end + + # Updates the value of a column in batches. + # + # This method updates the table in batches of 5% of the total row count. + # Any data inserted while running this method (or after it has finished + # running) is _not_ updated automatically. + # + # This method _only_ updates rows where the column's value is set to NULL. + # + # table - The name of the table. + # column - The name of the column to update. + # value - The value for the column. + def update_column_in_batches(table, column, value) + quoted_table = quote_table_name(table) + quoted_column = quote_column_name(column) + quoted_value = quote(value) + processed = 0 + + total = exec_query("SELECT COUNT(*) AS count FROM #{quoted_table}"). + to_hash. + first['count']. + to_i + + # Update in batches of 5% with an upper limit of 5000 rows. + batch_size = ((total / 100.0) * 5.0).ceil + + while processed < total + start_row = exec_query(%Q{ + SELECT id + FROM #{quoted_table} + ORDER BY id ASC + LIMIT 1 OFFSET #{processed} + }).to_hash.first + + stop_row = exec_query(%Q{ + SELECT id + FROM #{quoted_table} + ORDER BY id ASC + LIMIT 1 OFFSET #{processed + batch_size} + }.strip_heredoc).to_hash.first + + query = %Q{ + UPDATE #{quoted_table} + SET #{quoted_column} = #{quoted_value} + WHERE id >= #{start_row['id']} + } + + if stop_row + query += " AND id < #{stop_row['id']}" + end + + execute(query.strip_heredoc) + + processed += batch_size + end + end + + # Adds a column with a default value without locking an entire table. + # + # This method runs the following steps: + # + # 1. Add the column with a default value of NULL. + # 2. Update all existing rows in batches. + # 3. Change the default value of the column to the specified value. + # 4. Update any remaining rows. + # + # These steps ensure a column can be added to a large and commonly used + # table without locking the entire table for the duration of the table + # modification. + # + # table - The name of the table to update. + # column - The name of the column to add. + # type - The column type (e.g. `:integer`). + # default - The default value for the column. + # allow_null - When set to `true` the column will allow NULL values, the + # default is to not allow NULL values. + def add_column_with_default(table, column, type, default:, allow_null: false) + if transaction_open? + raise 'add_column_with_default can not be run inside a transaction, ' \ + 'you can disable transactions by calling disable_ddl_transaction! ' \ + 'in the body of your migration class' + end + + transaction do + add_column(table, column, type, default: nil) + + # Changing the default before the update ensures any newly inserted + # rows already use the proper default value. + change_column_default(table, column, default) + end + + begin + transaction do + update_column_in_batches(table, column, default) + end + rescue Exception => error + remove_column(table, column) + + raise error + end + + change_column_null(table, column, false) unless allow_null + end + end + end +end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb new file mode 100644 index 0000000000..ad2f3cb7e4 --- /dev/null +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -0,0 +1,124 @@ +require 'spec_helper' + +describe Gitlab::Database::MigrationHelpers, lib: true do + let(:model) do + Class.new do + include Gitlab::Database::MigrationHelpers + + def method_missing(name, *args, &block) + ActiveRecord::Base.connection.send(name, *args, &block) + end + end.new + end + + describe '#add_concurrent_index' do + context 'outside a transaction' do + before do + expect(model).to receive(:transaction_open?).and_return(false) + end + + context 'using PostgreSQL' do + it 'creates the index concurrently' do + expect(Gitlab::Database).to receive(:postgresql?).and_return(true) + + expect(model).to receive(:add_index). + with(:users, :foo, algorithm: :concurrently) + + model.add_concurrent_index(:users, :foo) + end + end + + context 'using MySQL' do + it 'creates a regular index' do + expect(Gitlab::Database).to receive(:postgresql?).and_return(false) + + expect(model).to receive(:add_index). + with(:users, :foo) + + model.add_concurrent_index(:users, :foo) + end + end + end + + context 'inside a transaction' do + it 'raises RuntimeError' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect { model.add_concurrent_index(:users, :foo) }. + to raise_error(RuntimeError) + end + end + end + + describe '#update_column_in_batches' do + before do + create_list(:empty_project, 5) + end + + it 'updates all the rows in a table' do + model.update_column_in_batches(:projects, :import_error, 'foo') + + expect(Project.where(import_error: 'foo').count).to eq(5) + end + end + + describe '#add_column_with_default' do + context 'outside of a transaction' do + before do + expect(model).to receive(:transaction_open?).and_return(false) + + expect(model).to receive(:transaction).twice.and_yield + + expect(model).to receive(:add_column). + with(:projects, :foo, :integer, default: nil) + + expect(model).to receive(:change_column_default). + with(:projects, :foo, 10) + end + + it 'adds the column while allowing NULL values' do + expect(model).to receive(:update_column_in_batches). + with(:projects, :foo, 10) + + expect(model).not_to receive(:change_column_null) + + model.add_column_with_default(:projects, :foo, :integer, + default: 10, + allow_null: true) + end + + it 'adds the column while not allowing NULL values' do + expect(model).to receive(:update_column_in_batches). + with(:projects, :foo, 10) + + expect(model).to receive(:change_column_null). + with(:projects, :foo, false) + + model.add_column_with_default(:projects, :foo, :integer, default: 10) + end + + it 'removes the added column whenever updating the rows fails' do + expect(model).to receive(:update_column_in_batches). + with(:projects, :foo, 10). + and_raise(RuntimeError) + + expect(model).to receive(:remove_column). + with(:projects, :foo) + + expect { + model.add_column_with_default(:projects, :foo, :integer, default: 10) + }.to raise_error(RuntimeError) + end + end + + context 'inside a transaction' do + it 'raises RuntimeError' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect { + model.add_column_with_default(:projects, :foo, :integer, default: 10) + }.to raise_error(RuntimeError) + end + end + end +end From d7cfe1a5bf671774a2fb7ffdb483fdbadf9f3228 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 9 May 2016 14:31:39 +0200 Subject: [PATCH 2/9] Added custom migration generator templates This allows us to automatically include Gitlab::Database::MigrationHelpers into newly generated migrations. --- .../migration/create_table_migration.rb | 32 ++++++++++++ .../active_record/migration/migration.rb | 52 +++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 lib/templates/active_record/migration/create_table_migration.rb create mode 100644 lib/templates/active_record/migration/migration.rb diff --git a/lib/templates/active_record/migration/create_table_migration.rb b/lib/templates/active_record/migration/create_table_migration.rb new file mode 100644 index 0000000000..1235ecd8e6 --- /dev/null +++ b/lib/templates/active_record/migration/create_table_migration.rb @@ -0,0 +1,32 @@ +class <%= migration_class_name %> < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation failes and can be re-tried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + create_table :<%= table_name %> do |t| +<% attributes.each do |attribute| -%> +<% if attribute.password_digest? -%> + t.string :password_digest<%= attribute.inject_options %> +<% else -%> + t.<%= attribute.type %> :<%= attribute.name %><%= attribute.inject_options %> +<% end -%> +<% end -%> +<% if options[:timestamps] %> + t.timestamps null: false +<% end -%> + end +<% attributes_with_index.each do |attribute| -%> + add_index :<%= table_name %>, :<%= attribute.index_name %><%= attribute.inject_index_options %> +<% end -%> + end +end diff --git a/lib/templates/active_record/migration/migration.rb b/lib/templates/active_record/migration/migration.rb new file mode 100644 index 0000000000..3cde953d15 --- /dev/null +++ b/lib/templates/active_record/migration/migration.rb @@ -0,0 +1,52 @@ +class <%= migration_class_name %> < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation failes and can be re-tried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + +<%- if migration_action == 'add' -%> + def change +<% attributes.each do |attribute| -%> + <%- if attribute.reference? -%> + add_reference :<%= table_name %>, :<%= attribute.name %><%= attribute.inject_options %> + <%- else -%> + add_column :<%= table_name %>, :<%= attribute.name %>, :<%= attribute.type %><%= attribute.inject_options %> + <%- if attribute.has_index? -%> + add_index :<%= table_name %>, :<%= attribute.index_name %><%= attribute.inject_index_options %> + <%- end -%> + <%- end -%> +<%- end -%> + end +<%- elsif migration_action == 'join' -%> + def change + create_join_table :<%= join_tables.first %>, :<%= join_tables.second %> do |t| + <%- attributes.each do |attribute| -%> + <%= '# ' unless attribute.has_index? -%>t.index <%= attribute.index_name %><%= attribute.inject_index_options %> + <%- end -%> + end + end +<%- else -%> + def change +<% attributes.each do |attribute| -%> +<%- if migration_action -%> + <%- if attribute.reference? -%> + remove_reference :<%= table_name %>, :<%= attribute.name %><%= attribute.inject_options %> + <%- else -%> + <%- if attribute.has_index? -%> + remove_index :<%= table_name %>, :<%= attribute.index_name %><%= attribute.inject_index_options %> + <%- end -%> + remove_column :<%= table_name %>, :<%= attribute.name %>, :<%= attribute.type %><%= attribute.inject_options %> + <%- end -%> +<%- end -%> +<%- end -%> + end +<%- end -%> +end From e8efaca23ce0b5aa347dfa053091d0b48e746fb8 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 9 May 2016 15:05:19 +0200 Subject: [PATCH 3/9] Updated migration styleguide for new helpers --- doc/development/migration_style_guide.md | 46 ++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 28dedf3978..1c13b09458 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -8,7 +8,10 @@ In addition, having to take a server offline for a an upgrade small or big is a big burden for most organizations. For this reason it is important that your migrations are written carefully, can be applied online and adhere to the style guide below. -It's advised to have offline migrations only in major GitLab releases. +Migrations should not require GitLab installations to be taken offline unless +_absolutely_ necessary. If a migration requires downtime this should be +clearly mentioned during the review process as well as being documented in the +monthly release post. When writing your migrations, also consider that databases might have stale data or inconsistencies and guard for that. Try to make as little assumptions as possible @@ -58,6 +61,45 @@ remove_index :namespaces, column: :name if index_exists?(:namespaces, :name) If you need to add an unique index please keep in mind there is possibility of existing duplicates. If it is possible write a separate migration for handling this situation. It can be just removing or removing with overwriting all references to these duplicates depend on situation. +When adding an index make sure to use the method `add_concurrent_index` instead +of the regular `add_index` method. The `add_concurrent_index` method +automatically creates concurrent indexes when using PostgreSQL, removing the +need for downtime. To use this method you must disable transactions by calling +the method `disable_ddl_transaction!` in the body of your migration class like +so: + +``` +class MyMigration < ActiveRecord::Migration + disable_ddl_transaction! + + def change + + end +end +``` + +## Adding Columns With Default Values + +When adding columns with default values you should use the method +`add_column_with_default`. This method ensures the table is updated without +requiring downtime. This method is not reversible so you must manually define +the `up` and `down` methods in your migration class. + +For example, to add the column `foo` to the `projects` table with a default +value of `10` you'd write the following: + +``` +class MyMigration < ActiveRecord::Migration + def up + add_column_with_default(:projects, :foo, :integer, 10) + end + + def down + remove_column(:projects, :foo) + end +end +``` + ## Testing Make sure that your migration works with MySQL and PostgreSQL with data. An empty database does not guarantee that your migration is correct. @@ -89,4 +131,4 @@ select_all("SELECT name, COUNT(id) as cnt FROM tags GROUP BY name HAVING COUNT(i execute("UPDATE taggings SET tag_id = #{origin_tag_id} WHERE tag_id IN(#{duplicate_ids.join(",")})") execute("DELETE FROM tags WHERE id IN(#{duplicate_ids.join(",")})") end -``` \ No newline at end of file +``` From d890807939a7b5ae9306cc347f87a1f53d46728e Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 9 May 2016 15:06:22 +0200 Subject: [PATCH 4/9] Link to migration guide in migration templates --- .../active_record/migration/create_table_migration.rb | 3 +++ lib/templates/active_record/migration/migration.rb | 3 +++ 2 files changed, 6 insertions(+) diff --git a/lib/templates/active_record/migration/create_table_migration.rb b/lib/templates/active_record/migration/create_table_migration.rb index 1235ecd8e6..4b7e39e057 100644 --- a/lib/templates/active_record/migration/create_table_migration.rb +++ b/lib/templates/active_record/migration/create_table_migration.rb @@ -1,3 +1,6 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + class <%= migration_class_name %> < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/lib/templates/active_record/migration/migration.rb b/lib/templates/active_record/migration/migration.rb index 3cde953d15..ae88593cd1 100644 --- a/lib/templates/active_record/migration/migration.rb +++ b/lib/templates/active_record/migration/migration.rb @@ -1,3 +1,6 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + class <%= migration_class_name %> < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers From 443c3aa5972fda2f9899fe3618ebfb9a31f59e1f Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 11 May 2016 12:43:54 +0200 Subject: [PATCH 5/9] Tell Rubocop to ignore lib/templates --- .rubocop.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index 9f179efa3c..5d56be65f1 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -21,6 +21,7 @@ AllCops: - 'lib/email_validator.rb' - 'lib/gitlab/upgrader.rb' - 'lib/gitlab/seeder.rb' + - 'lib/templates/**/*' ##################### Style ################################## @@ -276,7 +277,7 @@ Style/IdenticalConditionalBranches: Enabled: false # Checks the indentation of the first line of the right-hand-side of a -# multi-line assignment. +# multi-line assignment. Style/IndentAssignment: Enabled: false From 36083b4d0ee6787da6be13b45cc7144de704c308 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 11 May 2016 14:11:04 +0200 Subject: [PATCH 6/9] Fixed styling per Rubocop pedantics --- lib/gitlab/database/migration_helpers.rb | 4 +++- spec/lib/gitlab/database/migration_helpers_spec.rb | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 7aa8c90e6d..4773a88d0c 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -119,7 +119,9 @@ module Gitlab transaction do update_column_in_batches(table, column, default) end - rescue Exception => error + # We want to rescue _all_ exceptions here, even those that don't inherit + # from StandardError. + rescue Exception => error # rubocop: disable all remove_column(table, column) raise error diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index ad2f3cb7e4..ec43165bb5 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -105,9 +105,9 @@ describe Gitlab::Database::MigrationHelpers, lib: true do expect(model).to receive(:remove_column). with(:projects, :foo) - expect { + expect do model.add_column_with_default(:projects, :foo, :integer, default: 10) - }.to raise_error(RuntimeError) + end.to raise_error(RuntimeError) end end @@ -115,9 +115,9 @@ describe Gitlab::Database::MigrationHelpers, lib: true do it 'raises RuntimeError' do expect(model).to receive(:transaction_open?).and_return(true) - expect { + expect do model.add_column_with_default(:projects, :foo, :integer, default: 10) - }.to raise_error(RuntimeError) + end.to raise_error(RuntimeError) end end end From 6a29117793934850986a9e423c86746b4a0763e9 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 13 May 2016 00:11:01 +0200 Subject: [PATCH 7/9] Removed stray strip_heredoc from migration helpers --- lib/gitlab/database/migration_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 4773a88d0c..74c87fa290 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -63,7 +63,7 @@ module Gitlab FROM #{quoted_table} ORDER BY id ASC LIMIT 1 OFFSET #{processed + batch_size} - }.strip_heredoc).to_hash.first + }).to_hash.first query = %Q{ UPDATE #{quoted_table} From 59640866cee3d80a2144cb1992de36ba6678d4f7 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 19 May 2016 10:04:46 -0500 Subject: [PATCH 8/9] Remove left-over use of strip_heredoc --- lib/gitlab/database/migration_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 74c87fa290..31a87b73fb 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -75,7 +75,7 @@ module Gitlab query += " AND id < #{stop_row['id']}" end - execute(query.strip_heredoc) + execute(query) processed += batch_size end From 7e7764139d171ceeee5200b2677bdab0f8ab7c2d Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 19 May 2016 10:05:39 -0500 Subject: [PATCH 9/9] Fixed typos in migration templates --- lib/templates/active_record/migration/create_table_migration.rb | 2 +- lib/templates/active_record/migration/migration.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/templates/active_record/migration/create_table_migration.rb b/lib/templates/active_record/migration/create_table_migration.rb index 4b7e39e057..27acc75dcc 100644 --- a/lib/templates/active_record/migration/create_table_migration.rb +++ b/lib/templates/active_record/migration/create_table_migration.rb @@ -9,7 +9,7 @@ class <%= migration_class_name %> < ActiveRecord::Migration # existing transaction. When using "add_concurrent_index" make sure that this # method is the _only_ method called in the migration, any other changes # should go in a separate migration. This ensures that upon failure _only_ the - # index creation failes and can be re-tried or reverted easily. + # index creation fails and can be retried or reverted easily. # # To disable transactions uncomment the following line and remove these # comments: diff --git a/lib/templates/active_record/migration/migration.rb b/lib/templates/active_record/migration/migration.rb index ae88593cd1..06bdea1136 100644 --- a/lib/templates/active_record/migration/migration.rb +++ b/lib/templates/active_record/migration/migration.rb @@ -9,7 +9,7 @@ class <%= migration_class_name %> < ActiveRecord::Migration # existing transaction. When using "add_concurrent_index" make sure that this # method is the _only_ method called in the migration, any other changes # should go in a separate migration. This ensures that upon failure _only_ the - # index creation failes and can be re-tried or reverted easily. + # index creation fails and can be retried or reverted easily. # # To disable transactions uncomment the following line and remove these # comments: