From f54197281b935f504780ece6902d23eaff67311c Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 10 Jan 2023 10:21:04 +0200 Subject: [PATCH 1/3] Accept on_update option for add_concurrent_foreign_key helper --- lib/gitlab/database/migration_helpers.rb | 16 ++++- .../gitlab/database/migration_helpers_spec.rb | 61 ++++++++++++++++++- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 4858a96c173eb5..5b0a8578fae99e 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -288,7 +288,8 @@ def remove_concurrent_index_by_name(table_name, index_name, options = {}) # order of the ALTER TABLE. This can be useful in situations where the foreign # key creation could deadlock with another process. # - def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, target_column: :id, name: nil, validate: true, reverse_lock_order: false) + # rubocop: disable Metrics/ParameterLists + def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, on_update: nil, target_column: :id, name: nil, validate: true, reverse_lock_order: false) # Transactions would result in ALTER TABLE locks being held for the # duration of the transaction, defeating the purpose of this method. if transaction_open? @@ -298,6 +299,7 @@ def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, tar options = { column: column, on_delete: on_delete, + on_update: on_update, name: name.presence || concurrent_foreign_key_name(source, column), primary_key: target_column } @@ -306,7 +308,8 @@ def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, tar warning_message = "Foreign key not created because it exists already " \ "(this may be due to an aborted migration or similar): " \ "source: #{source}, target: #{target}, column: #{options[:column]}, "\ - "name: #{options[:name]}, on_delete: #{options[:on_delete]}" + "name: #{options[:name]}, on_update: #{options[:on_update]}, "\ + "on_delete: #{options[:on_delete]}" Gitlab::AppLogger.warn warning_message else @@ -322,6 +325,7 @@ def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, tar ADD CONSTRAINT #{options[:name]} FOREIGN KEY (#{multiple_columns(options[:column])}) REFERENCES #{target} (#{multiple_columns(target_column)}) + #{on_update_statement(options[:on_update])} #{on_delete_statement(options[:on_delete])} NOT VALID; EOF @@ -343,6 +347,7 @@ def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, tar end end end + # rubocop: enable Metrics/ParameterLists def validate_foreign_key(source, column, name: nil) fk_name = name || concurrent_foreign_key_name(source, column) @@ -1278,6 +1283,13 @@ def on_delete_statement(on_delete) "ON DELETE #{on_delete.upcase}" end + def on_update_statement(on_update) + return '' if on_update.blank? + return 'ON UPDATE SET NULL' if on_update == :nullify + + "ON UPDATE #{on_update.upcase}" + end + def create_column_from(table, old, new, type: nil, batch_column_name: :id, type_cast_function: nil, limit: nil) old_col = column_for(table, old) new_type = type || old_col.type diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 8923187142f335..399c7754b42613 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -743,6 +743,59 @@ end end + context 'ON UPDATE statements' do + context 'on_update: :nullify' do + it 'appends ON UPDATE SET NULL statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).to receive(:execute).with(/ON UPDATE SET NULL/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_update: :nullify) + end + end + + context 'on_update: :cascade' do + it 'appends ON UPDATE CASCADE statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).to receive(:execute).with(/ON UPDATE CASCADE/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_update: :cascade) + end + end + + context 'on_update: nil' do + it 'appends no ON UPDATE statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).not_to receive(:execute).with(/ON UPDATE/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_update: nil) + end + end + end + context 'when no custom key name is supplied' do it 'creates a concurrent foreign key and validates it' do expect(model).to receive(:with_lock_retries).and_call_original @@ -760,6 +813,7 @@ name = model.concurrent_foreign_key_name(:projects, :user_id) expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id, + on_update: nil, on_delete: :cascade, name: name, primary_key: :id).and_return(true) @@ -792,6 +846,7 @@ expect(model).to receive(:foreign_key_exists?).with(:projects, :users, name: :foo, primary_key: :id, + on_update: nil, on_delete: :cascade, column: :user_id).and_return(true) @@ -861,6 +916,7 @@ "ADD CONSTRAINT fk_multiple_columns\n" \ "FOREIGN KEY \(partition_number, user_id\)\n" \ "REFERENCES users \(partition_number, id\)\n" \ + "ON UPDATE CASCADE\n" \ "ON DELETE CASCADE\n" \ "NOT VALID;\n" ) @@ -871,7 +927,8 @@ column: [:partition_number, :user_id], target_column: [:partition_number, :id], validate: false, - name: :fk_multiple_columns + name: :fk_multiple_columns, + on_update: :cascade ) end @@ -883,6 +940,7 @@ { column: [:partition_number, :user_id], name: :fk_multiple_columns, + on_update: :cascade, on_delete: :cascade, primary_key: [:partition_number, :id] } @@ -898,6 +956,7 @@ :users, column: [:partition_number, :user_id], target_column: [:partition_number, :id], + on_update: :cascade, validate: false, name: :fk_multiple_columns ) -- GitLab From 3f1e48f7f9cc10e168d98d836f190c0ad40dfc5a Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 10 Jan 2023 13:00:35 +0200 Subject: [PATCH 2/3] Apply review feedback --- .../gitlab/database/migration_helpers_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 399c7754b42613..c5dd589a021698 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -794,6 +794,22 @@ on_update: nil) end end + + context 'when on_update is not provided' do + it 'appends no ON UPDATE statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).not_to receive(:execute).with(/ON UPDATE/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id) + end + end end context 'when no custom key name is supplied' do -- GitLab From 28083cb0cb3a64b6eae0781b9516fdda5ae318d4 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Tue, 10 Jan 2023 13:04:42 +0200 Subject: [PATCH 3/3] Document on_update option --- lib/gitlab/database/migration_helpers.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 5b0a8578fae99e..5758984561c1fd 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -281,6 +281,9 @@ def remove_concurrent_index_by_name(table_name, index_name, options = {}) # target_column - The name of the referenced column, defaults to "id". # on_delete - The action to perform when associated data is removed, # defaults to "CASCADE". + # on_update - The action to perform when associated data is updated, + # defaults to nil. This is useful for multi column FKs if + # it's desirable to update one of the columns. # name - The name of the foreign key. # validate - Flag that controls whether the new foreign key will be validated after creation. # If the flag is not set, the constraint will only be enforced for new data. -- GitLab