diff options
author | Rémy Coutable <remy@rymai.me> | 2016-06-13 12:50:39 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-06-13 12:50:39 +0000 |
commit | c0e415e488125428631f15c5e990a97435e9e056 (patch) | |
tree | 380ced5bbbc3410335c6a16d05a8665499619c39 | |
parent | dc38551b93abe5f1adb655aff68fc451dc3d8e73 (diff) | |
parent | ea7ff1341032c04ff9abad0e286888a3ab8a9a15 (diff) | |
download | gitlab-ce-c0e415e488125428631f15c5e990a97435e9e056.tar.gz |
Merge branch 'fix-migration-helper-race-conditions' into 'master'
Fix migration helper race conditions
## What does this MR do?
This MR fixes two problems with the migration helpers:
1. An error in `change_column_null` would not drop the previously created column
2. `update_column_in_batches` would rely on the number of rows in a table to determine how many to update. This meant that newly inserted rows (after the `COUNT`) would not be taken into account.
This MR also removes an outdated comment for `update_column_in_batches`.
## Are there points in the code the reviewer needs to double check?
No.
## Why was this MR needed?
See above.
## What are the relevant issue numbers?
Fixes #18483
## Does this MR meet the acceptance criteria?
- [ ] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] ~~API support added~~
- [ ] Tests
- [x] Added for this feature/bug
- [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4618
-rw-r--r-- | lib/gitlab/database/migration_helpers.rb | 13 | ||||
-rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 13 |
2 files changed, 20 insertions, 6 deletions
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 978c3f7896d..dd3ff0ab18b 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -31,8 +31,6 @@ module Gitlab # 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. @@ -55,10 +53,10 @@ module Gitlab first['count']. to_i - # Update in batches of 5% + # Update in batches of 5% until we run out of any rows to update. batch_size = ((total / 100.0) * 5.0).ceil - while processed < total + loop do start_row = exec_query(%Q{ SELECT id FROM #{quoted_table} @@ -66,6 +64,9 @@ module Gitlab LIMIT 1 OFFSET #{processed} }).to_hash.first + # There are no more rows to process + break unless start_row + stop_row = exec_query(%Q{ SELECT id FROM #{quoted_table} @@ -126,6 +127,8 @@ module Gitlab begin transaction do update_column_in_batches(table, column, default) + + change_column_null(table, column, false) unless allow_null end # We want to rescue _all_ exceptions here, even those that don't inherit # from StandardError. @@ -134,8 +137,6 @@ module Gitlab raise error end - - change_column_null(table, column, false) unless allow_null end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 83ddabe6b0b..1ec539066a7 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -120,6 +120,19 @@ describe Gitlab::Database::MigrationHelpers, lib: true do model.add_column_with_default(:projects, :foo, :integer, default: 10) end.to raise_error(RuntimeError) 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(:remove_column). + with(:projects, :foo) + + expect do + model.add_column_with_default(:projects, :foo, :integer, default: 10) + end.to raise_error(RuntimeError) + end end context 'inside a transaction' do |