Commit c0e415e4 authored by Rémy Coutable's avatar Rémy Coutable

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
parents dc38551b ea7ff134
...@@ -31,8 +31,6 @@ module Gitlab ...@@ -31,8 +31,6 @@ module Gitlab
# Any data inserted while running this method (or after it has finished # Any data inserted while running this method (or after it has finished
# running) is _not_ updated automatically. # 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. # table - The name of the table.
# column - The name of the column to update. # column - The name of the column to update.
# value - The value for the column. # value - The value for the column.
...@@ -55,10 +53,10 @@ module Gitlab ...@@ -55,10 +53,10 @@ module Gitlab
first['count']. first['count'].
to_i 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 batch_size = ((total / 100.0) * 5.0).ceil
while processed < total loop do
start_row = exec_query(%Q{ start_row = exec_query(%Q{
SELECT id SELECT id
FROM #{quoted_table} FROM #{quoted_table}
...@@ -66,6 +64,9 @@ module Gitlab ...@@ -66,6 +64,9 @@ module Gitlab
LIMIT 1 OFFSET #{processed} LIMIT 1 OFFSET #{processed}
}).to_hash.first }).to_hash.first
# There are no more rows to process
break unless start_row
stop_row = exec_query(%Q{ stop_row = exec_query(%Q{
SELECT id SELECT id
FROM #{quoted_table} FROM #{quoted_table}
...@@ -126,6 +127,8 @@ module Gitlab ...@@ -126,6 +127,8 @@ module Gitlab
begin begin
transaction do transaction do
update_column_in_batches(table, column, default) update_column_in_batches(table, column, default)
change_column_null(table, column, false) unless allow_null
end end
# We want to rescue _all_ exceptions here, even those that don't inherit # We want to rescue _all_ exceptions here, even those that don't inherit
# from StandardError. # from StandardError.
...@@ -134,8 +137,6 @@ module Gitlab ...@@ -134,8 +137,6 @@ module Gitlab
raise error raise error
end end
change_column_null(table, column, false) unless allow_null
end end
end end
end end
......
...@@ -120,6 +120,19 @@ describe Gitlab::Database::MigrationHelpers, lib: true do ...@@ -120,6 +120,19 @@ describe Gitlab::Database::MigrationHelpers, lib: true do
model.add_column_with_default(:projects, :foo, :integer, default: 10) model.add_column_with_default(:projects, :foo, :integer, default: 10)
end.to raise_error(RuntimeError) end.to raise_error(RuntimeError)
end 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 end
context 'inside a transaction' do context 'inside a transaction' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment