From 6cea905ff78e8266dd50d102718dde295667612e Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Fri, 27 Jan 2023 17:03:20 +0200 Subject: [PATCH 1/3] Allow delayed validation for partitioned FK migration helper It refactors `add_concurrent_partitioned_foreign_key` migration helper to allow multiple column references. It also alows to skip adding the FK on the parent table for async validation. Example of usage: First migration to add the FK on all partitions: ```ruby add_concurrent_partitioned_foreign_key( :p_ci_builds_metadata, :ci_builds, column: [:partition_id, :build_id], target_column: [:partition_id, :id], name: :fk_multiple_columns, validate: false ) ``` Second migration to asynchronously validate the constraints on partitions: ```ruby prepare_partitioned_async_foreign_key_validation( :p_ci_builds_metadata, name: fk_multiple_columns ) ``` Third migration to add the FK on the parent table and on any new partitions that might have been created: ```ruby add_concurrent_partitioned_foreign_key( :p_ci_builds_metadata, :ci_builds, column: [:partition_id, :build_id], target_column: [:partition_id, :id], name: :fk_multiple_columns, validate: true ) ``` --- lib/gitlab/database/migration_helpers.rb | 21 +++- .../foreign_key_helpers.rb | 44 +++++-- .../gitlab/database/migration_helpers_spec.rb | 63 +++++++++- .../foreign_key_helpers_spec.rb | 110 +++++++++++++++--- 4 files changed, 209 insertions(+), 29 deletions(-) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index c3a7b384172ce6..9c6188f1967879 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -293,7 +293,17 @@ def remove_concurrent_index_by_name(table_name, index_name, options = {}) # key creation could deadlock with another process. # # 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) + 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? @@ -321,6 +331,7 @@ def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, on_ # validating it. This means we keep the ALTER TABLE lock only for a # short period of time. The key _is_ enforced for any newly created # data. + not_valid_option = 'NOT VALID' unless table_partitioned?(source) with_lock_retries do execute("LOCK TABLE #{target}, #{source} IN SHARE ROW EXCLUSIVE MODE") if reverse_lock_order @@ -331,7 +342,7 @@ def add_concurrent_foreign_key(source, target, column:, on_delete: :cascade, on_ REFERENCES #{target} (#{multiple_columns(target_column)}) #{on_update_statement(options[:on_update])} #{on_delete_statement(options[:on_delete])} - NOT VALID; + #{not_valid_option}; EOF end end @@ -1239,6 +1250,12 @@ def partition?(table_name) end end + def table_partitioned?(table_name) + Gitlab::Database::PostgresPartitionedTable + .find_by_name_in_current_schema(table_name) + .present? + end + private def multiple_columns(columns, separator: ', ') diff --git a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb index 8849191f356416..3c12da28b4db05 100644 --- a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb +++ b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb @@ -32,46 +32,72 @@ module ForeignKeyHelpers # column - The name of the column to create the foreign key on. # 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 "CASCADE". # 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 + # in the existing partitions. The helper will need to be called again to add the + # foreign key on the parent table after validating it on all partitions. + # reverse_lock_order - Flag that controls whether we should attempt to acquire locks in the reverse + # order of the ALTER TABLE. This can be useful in situations where the foreign + # key creation could deadlock with another process. # - def add_concurrent_partitioned_foreign_key(source, target, column:, on_delete: :cascade, name: nil) + # rubocop: disable Metrics/ParameterLists + def add_concurrent_partitioned_foreign_key( + source, target, column:, target_column: :id, on_delete: :cascade, + on_update: nil, name: nil, validate: true, reverse_lock_order: false) assert_not_in_transaction_block(scope: ERROR_SCOPE) partition_options = { column: column, on_delete: on_delete, + on_update: on_update, # We'll use the same FK name for all partitions and match it to # the name used for the partitioned table to follow the convention # used by PostgreSQL when adding FKs to new partitions name: name.presence || concurrent_partitioned_foreign_key_name(source, column), - - # Force the FK validation to true for partitions (and the partitioned table) - validate: true + validate: validate } if foreign_key_exists?(source, target, **partition_options) 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: #{partition_options[:column]}, "\ - "name: #{partition_options[:name]}, on_delete: #{partition_options[:on_delete]}" + "name: #{partition_options[:name]}, on_delete: #{partition_options[:on_delete]}, "\ + "on_update: #{partition_options[:on_update]}" Gitlab::AppLogger.warn warning_message return end - partitioned_table = find_partitioned_table(source) + partition_options.merge!(target_column: target_column, reverse_lock_order: reverse_lock_order) - partitioned_table.postgres_partitions.order(:name).each do |partition| + Gitlab::Database::PostgresPartitionedTable.each_partition(source) do |partition| add_concurrent_foreign_key(partition.identifier, target, **partition_options) end - with_lock_retries do - add_foreign_key(source, target, **partition_options) + # If we are to add the FK on the parent table now, it will trigger + # the validation on all partitions. The helper must be called one + # more time with `validate: true` after the FK is valid on all partitions. + return unless validate + + add_concurrent_foreign_key(source, target, **partition_options) + end + # rubocop: enable Metrics/ParameterLists + + def validate_partitioned_foreign_key(source, column, name: nil) + assert_not_in_transaction_block(scope: ERROR_SCOPE) + + Gitlab::Database::PostgresPartitionedTable.each_partition(source) do |partition| + validate_foreign_key(partition.identifier, column, name: name) end end + private + # Returns the name for a concurrent partitioned foreign key. # # Similar to concurrent_foreign_key_name (Gitlab::Database::MigrationHelpers) diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 63a470c82a1ffe..0d30e8fc235c4d 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::MigrationHelpers do +RSpec.describe Gitlab::Database::MigrationHelpers, feature_category: :database do include Database::TableSchemaHelpers include Database::TriggerHelpers @@ -979,6 +979,53 @@ end end end + + context 'when creating foreign key on a partitioned table' do + before do + model.execute(<<~SQL) + CREATE TABLE public._test_source_partitioned_table ( + id serial NOT NULL, + partition_id serial NOT NULL, + owner_id bigint NOT NULL, + PRIMARY KEY (id, partition_id) + ) PARTITION BY LIST(partition_id); + + CREATE TABLE _test_source_partitioned_table_1 + PARTITION OF public._test_source_partitioned_table + FOR VALUES IN (1); + + CREATE TABLE public._test_dest_partitioned_table ( + id serial NOT NULL, + partition_id serial NOT NULL, + PRIMARY KEY (id, partition_id) + ); + SQL + end + + it 'creates the FK without using NOT VALID', :aggregate_failures do + allow(model).to receive(:execute).and_call_original + + expect(model).to receive(:with_lock_retries).and_yield + + expect(model).to receive(:execute).with( + "ALTER TABLE _test_source_partitioned_table\n" \ + "ADD CONSTRAINT fk_multiple_columns\n" \ + "FOREIGN KEY \(partition_id, owner_id\)\n" \ + "REFERENCES _test_dest_partitioned_table \(partition_id, id\)\n" \ + "ON UPDATE CASCADE\n" \ + "ON DELETE CASCADE\n;\n" + ) + + model.add_concurrent_foreign_key( + :_test_source_partitioned_table, + :_test_dest_partitioned_table, + column: [:partition_id, :owner_id], + target_column: [:partition_id, :id], + name: :fk_multiple_columns, + on_update: :cascade + ) + end + end end end @@ -2896,4 +2943,18 @@ def setup it { is_expected.to be_falsey } end end + + describe "#table_partitioned?" do + subject { model.table_partitioned?(table_name) } + + let(:table_name) { 'p_ci_builds_metadata' } + + it { is_expected.to be_truthy } + + context 'with a non-partitioned table' do + let(:table_name) { 'users' } + + it { is_expected.to be_falsey } + end + end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb index f0e34476cf28bd..9ef12ca7a2dd77 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers do +RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers, feature_category: :database do include Database::TableSchemaHelpers let(:migration) do @@ -21,10 +21,13 @@ column: column_name, name: foreign_key_name, on_delete: :cascade, + on_update: nil, validate: true } end + let(:create_options) { options.merge!(reverse_lock_order: false, target_column: :id) } + before do allow(migration).to receive(:puts) allow(migration).to receive(:transaction_open?).and_return(false) @@ -67,12 +70,11 @@ expect(migration).to receive(:concurrent_partitioned_foreign_key_name).and_return(foreign_key_name) - expect_add_concurrent_fk_and_call_original(partition1_name, target_table_name, **options) - expect_add_concurrent_fk_and_call_original(partition2_name, target_table_name, **options) + expect_add_concurrent_fk_and_call_original(partition1_name, target_table_name, **create_options) + expect_add_concurrent_fk_and_call_original(partition2_name, target_table_name, **create_options) - expect(migration).to receive(:with_lock_retries).ordered.and_yield - expect(migration).to receive(:add_foreign_key) - .with(source_table_name, target_table_name, **options) + expect(migration).to receive(:add_concurrent_foreign_key) + .with(source_table_name, target_table_name, **create_options) .ordered .and_call_original @@ -81,6 +83,38 @@ expect_foreign_key_to_exist(source_table_name, foreign_key_name) end + context 'with validate: false option' do + let(:options) do + { + column: column_name, + name: foreign_key_name, + on_delete: :cascade, + on_update: nil, + validate: false + } + end + + it 'creates the foreign key only on partitions' do + expect(migration).to receive(:foreign_key_exists?) + .with(source_table_name, target_table_name, **options) + .and_return(false) + + expect(migration).to receive(:concurrent_partitioned_foreign_key_name).and_return(foreign_key_name) + + expect_add_concurrent_fk_and_call_original(partition1_name, target_table_name, **create_options) + expect_add_concurrent_fk_and_call_original(partition2_name, target_table_name, **create_options) + + expect(migration).not_to receive(:add_concurrent_foreign_key) + .with(source_table_name, target_table_name, **create_options) + + migration.add_concurrent_partitioned_foreign_key( + source_table_name, target_table_name, + column: column_name, validate: false) + + expect_foreign_key_not_to_exist(source_table_name, foreign_key_name) + end + end + def expect_add_concurrent_fk_and_call_original(source_table_name, target_table_name, options) expect(migration).to receive(:add_concurrent_foreign_key) .ordered @@ -100,8 +134,6 @@ def expect_add_concurrent_fk_and_call_original(source_table_name, target_table_n .and_return(true) expect(migration).not_to receive(:add_concurrent_foreign_key) - expect(migration).not_to receive(:with_lock_retries) - expect(migration).not_to receive(:add_foreign_key) migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, column: column_name) @@ -110,30 +142,39 @@ def expect_add_concurrent_fk_and_call_original(source_table_name, target_table_n end context 'when additional foreign key options are given' do - let(:options) do + let(:exits_options) do { column: column_name, name: '_my_fk_name', on_delete: :restrict, - validate: true + validate: true, + on_update: nil } end + let(:create_options) { exits_options.merge!(reverse_lock_order: false, target_column: :id) } + it 'forwards them to the foreign key helper methods' do expect(migration).to receive(:foreign_key_exists?) - .with(source_table_name, target_table_name, **options) + .with(source_table_name, target_table_name, **exits_options) .and_return(false) expect(migration).not_to receive(:concurrent_partitioned_foreign_key_name) - expect_add_concurrent_fk(partition1_name, target_table_name, **options) - expect_add_concurrent_fk(partition2_name, target_table_name, **options) + expect_add_concurrent_fk(partition1_name, target_table_name, **create_options) + expect_add_concurrent_fk(partition2_name, target_table_name, **create_options) - expect(migration).to receive(:with_lock_retries).ordered.and_yield - expect(migration).to receive(:add_foreign_key).with(source_table_name, target_table_name, **options).ordered + expect(migration).to receive(:add_concurrent_foreign_key) + .with(source_table_name, target_table_name, **create_options) + .ordered - migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, - column: column_name, name: '_my_fk_name', on_delete: :restrict) + migration.add_concurrent_partitioned_foreign_key( + source_table_name, + target_table_name, + column: column_name, + name: '_my_fk_name', + on_delete: :restrict + ) end def expect_add_concurrent_fk(source_table_name, target_table_name, options) @@ -153,4 +194,39 @@ def expect_add_concurrent_fk(source_table_name, target_table_name, options) end end end + + describe '#validate_partitioned_foreign_key' do + context 'when run inside a transaction block' do + it 'raises an error' do + expect(migration).to receive(:transaction_open?).and_return(true) + + expect do + migration.validate_partitioned_foreign_key(source_table_name, column_name, name: '_my_fk_name') + end.to raise_error(/can not be run inside a transaction/) + end + end + + context 'when run outside a transaction block' do + before do + migration.add_concurrent_partitioned_foreign_key( + source_table_name, + target_table_name, + column: column_name, + name: foreign_key_name, + validate: false + ) + end + + it 'validates FK for each partition' do + expect(migration).to receive(:execute).with(/SET statement_timeout TO 0/).twice + expect(migration).to receive(:execute).with(/RESET statement_timeout/).twice + expect(migration).to receive(:execute) + .with(/ALTER TABLE #{partition1_name} VALIDATE CONSTRAINT #{foreign_key_name}/).ordered + expect(migration).to receive(:execute) + .with(/ALTER TABLE #{partition2_name} VALIDATE CONSTRAINT #{foreign_key_name}/).ordered + + migration.validate_partitioned_foreign_key(source_table_name, column_name, name: foreign_key_name) + end + end + end end -- GitLab From ed05574973b37fc47bf5fa657e4e3833d06436d5 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 9 Mar 2023 17:55:17 +0200 Subject: [PATCH 2/3] Prevent direct usage with partitioned tables --- lib/gitlab/database/migration_helpers.rb | 48 ++++++++--------- .../foreign_key_helpers.rb | 52 +++++++++---------- .../gitlab/database/migration_helpers_spec.rb | 14 ++++- .../foreign_key_helpers_spec.rb | 26 +++++++--- 4 files changed, 81 insertions(+), 59 deletions(-) diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 9c6188f1967879..82adb0dd369b88 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -292,33 +292,34 @@ 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. # - # 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 - ) + def add_concurrent_foreign_key(source, target, column:, **options) + options.reverse_merge!({ + on_delete: :cascade, + on_update: nil, + target_column: :id, + validate: true, + reverse_lock_order: false, + allow_partitioned: false, + column: column + }) + # Transactions would result in ALTER TABLE locks being held for the # duration of the transaction, defeating the purpose of this method. if transaction_open? raise 'add_concurrent_foreign_key can not be run inside a transaction' end - options = { - column: column, - on_delete: on_delete, - on_update: on_update, - name: name.presence || concurrent_foreign_key_name(source, column), - primary_key: target_column - } + if !options.delete(:allow_partitioned) && table_partitioned?(source) + raise ArgumentError, 'add_concurrent_foreign_key can not be used on a partitioned ' \ + 'table. Please use add_concurrent_partitioned_foreign_key on the partitioned table ' \ + 'as we need to create foreign keys on each partition and a FK on the parent table' + end + + options[:name] ||= concurrent_foreign_key_name(source, column) + options[:primary_key] = options[:target_column] + check_options = options.slice(:column, :on_delete, :on_update, :name, :primary_key) - if foreign_key_exists?(source, target, **options) + if foreign_key_exists?(source, target, **check_options) 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]}, "\ @@ -334,12 +335,12 @@ def add_concurrent_foreign_key( not_valid_option = 'NOT VALID' unless table_partitioned?(source) with_lock_retries do - execute("LOCK TABLE #{target}, #{source} IN SHARE ROW EXCLUSIVE MODE") if reverse_lock_order + execute("LOCK TABLE #{target}, #{source} IN SHARE ROW EXCLUSIVE MODE") if options[:reverse_lock_order] execute <<-EOF.strip_heredoc ALTER TABLE #{source} ADD CONSTRAINT #{options[:name]} FOREIGN KEY (#{multiple_columns(options[:column])}) - REFERENCES #{target} (#{multiple_columns(target_column)}) + REFERENCES #{target} (#{multiple_columns(options[:target_column])}) #{on_update_statement(options[:on_update])} #{on_delete_statement(options[:on_delete])} #{not_valid_option}; @@ -356,13 +357,12 @@ def add_concurrent_foreign_key( # # Note this is a no-op in case the constraint is VALID already - if validate + if options[:validate] disable_statement_timeout do execute("ALTER TABLE #{source} VALIDATE CONSTRAINT #{options[:name]};") end end end - # rubocop: enable Metrics/ParameterLists def validate_foreign_key(source, column, name: nil) fk_name = name || concurrent_foreign_key_name(source, column) diff --git a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb index 3c12da28b4db05..8fd5f184d64a96 100644 --- a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb +++ b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb @@ -43,50 +43,50 @@ module ForeignKeyHelpers # order of the ALTER TABLE. This can be useful in situations where the foreign # key creation could deadlock with another process. # - # rubocop: disable Metrics/ParameterLists - def add_concurrent_partitioned_foreign_key( - source, target, column:, target_column: :id, on_delete: :cascade, - on_update: nil, name: nil, validate: true, reverse_lock_order: false) + def add_concurrent_partitioned_foreign_key(source, target, column:, **options) assert_not_in_transaction_block(scope: ERROR_SCOPE) - partition_options = { - column: column, - on_delete: on_delete, - on_update: on_update, - - # We'll use the same FK name for all partitions and match it to - # the name used for the partitioned table to follow the convention - # used by PostgreSQL when adding FKs to new partitions - name: name.presence || concurrent_partitioned_foreign_key_name(source, column), - validate: validate - } - - if foreign_key_exists?(source, target, **partition_options) + options.reverse_merge!({ + target_column: :id, + on_delete: :cascade, + on_update: nil, + name: nil, + validate: true, + reverse_lock_order: false, + column: column + }) + + # We'll use the same FK name for all partitions and match it to + # the name used for the partitioned table to follow the convention + # used by PostgreSQL when adding FKs to new partitions + options[:name] ||= concurrent_partitioned_foreign_key_name(source, column) + check_options = options.slice(:column, :on_delete, :on_update, :name) + check_options[:primary_key] = options[:target_column] + + if foreign_key_exists?(source, target, **check_options) 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: #{partition_options[:column]}, "\ - "name: #{partition_options[:name]}, on_delete: #{partition_options[:on_delete]}, "\ - "on_update: #{partition_options[:on_update]}" + "source: #{source}, target: #{target}, column: #{options[:column]}, "\ + "name: #{options[:name]}, on_delete: #{options[:on_delete]}, "\ + "on_update: #{options[:on_update]}" Gitlab::AppLogger.warn warning_message return end - partition_options.merge!(target_column: target_column, reverse_lock_order: reverse_lock_order) - Gitlab::Database::PostgresPartitionedTable.each_partition(source) do |partition| - add_concurrent_foreign_key(partition.identifier, target, **partition_options) + add_concurrent_foreign_key(partition.identifier, target, **options) end # If we are to add the FK on the parent table now, it will trigger # the validation on all partitions. The helper must be called one # more time with `validate: true` after the FK is valid on all partitions. - return unless validate + return unless options[:validate] - add_concurrent_foreign_key(source, target, **partition_options) + options[:allow_partitioned] = true + add_concurrent_foreign_key(source, target, **options) end - # rubocop: enable Metrics/ParameterLists def validate_partitioned_foreign_key(source, column, name: nil) assert_not_in_transaction_block(scope: ERROR_SCOPE) diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 0d30e8fc235c4d..1af69f85a54eab 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -1022,9 +1022,21 @@ column: [:partition_id, :owner_id], target_column: [:partition_id, :id], name: :fk_multiple_columns, - on_update: :cascade + on_update: :cascade, + allow_partitioned: true ) end + + it 'raises an error if allow_partitioned is not set' do + expect(model).not_to receive(:with_lock_retries).and_yield + expect(model).not_to receive(:execute).with(/FOREIGN KEY/) + + args = [:_test_source_partitioned_table, :_test_dest_partitioned_table] + options = { column: [:partition_id, :owner_id], target_column: [:partition_id, :id] } + + expect { model.add_concurrent_foreign_key(*args, **options) } + .to raise_error ArgumentError, /use add_concurrent_partitioned_foreign_key/ + end end end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb index 9ef12ca7a2dd77..d5f4afd7ba46db 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb @@ -16,17 +16,22 @@ let(:partition_schema) { 'gitlab_partitions_dynamic' } let(:partition1_name) { "#{partition_schema}.#{source_table_name}_202001" } let(:partition2_name) { "#{partition_schema}.#{source_table_name}_202002" } + let(:validate) { true } let(:options) do { column: column_name, name: foreign_key_name, on_delete: :cascade, on_update: nil, - validate: true + primary_key: :id } end - let(:create_options) { options.merge!(reverse_lock_order: false, target_column: :id) } + let(:create_options) do + options + .except(:primary_key) + .merge!(reverse_lock_order: false, target_column: :id, validate: validate) + end before do allow(migration).to receive(:puts) @@ -74,7 +79,7 @@ expect_add_concurrent_fk_and_call_original(partition2_name, target_table_name, **create_options) expect(migration).to receive(:add_concurrent_foreign_key) - .with(source_table_name, target_table_name, **create_options) + .with(source_table_name, target_table_name, allow_partitioned: true, **create_options) .ordered .and_call_original @@ -84,13 +89,14 @@ end context 'with validate: false option' do + let(:validate) { false } let(:options) do { column: column_name, name: foreign_key_name, on_delete: :cascade, on_update: nil, - validate: false + primary_key: :id } end @@ -147,12 +153,16 @@ def expect_add_concurrent_fk_and_call_original(source_table_name, target_table_n column: column_name, name: '_my_fk_name', on_delete: :restrict, - validate: true, - on_update: nil + on_update: nil, + primary_key: :id } end - let(:create_options) { exits_options.merge!(reverse_lock_order: false, target_column: :id) } + let(:create_options) do + exits_options + .except(:primary_key) + .merge!(reverse_lock_order: false, target_column: :id, validate: true) + end it 'forwards them to the foreign key helper methods' do expect(migration).to receive(:foreign_key_exists?) @@ -165,7 +175,7 @@ def expect_add_concurrent_fk_and_call_original(source_table_name, target_table_n expect_add_concurrent_fk(partition2_name, target_table_name, **create_options) expect(migration).to receive(:add_concurrent_foreign_key) - .with(source_table_name, target_table_name, **create_options) + .with(source_table_name, target_table_name, allow_partitioned: true, **create_options) .ordered migration.add_concurrent_partitioned_foreign_key( -- GitLab From 206287f438f7e102677c3e2c78e91ca5e3c8dd5e Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Fri, 10 Mar 2023 13:16:58 +0200 Subject: [PATCH 3/3] Update docs for partitioned FK helper --- .../foreign_key_helpers.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb index 8fd5f184d64a96..7d9c12d776e588 100644 --- a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb +++ b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb @@ -33,12 +33,15 @@ module ForeignKeyHelpers # 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 "CASCADE". + # no default value is set. # name - The name of the foreign key. - # validate - Flag that controls whether the new foreign key will be validated after creation. + # validate - Flag that controls whether the new foreign key will be + # validated after creation and if it will be added on the parent table. # If the flag is not set, the constraint will only be enforced for new data - # in the existing partitions. The helper will need to be called again to add the - # foreign key on the parent table after validating it on all partitions. + # in the existing partitions. The helper will need to be called again + # with the flag set to `true` to add the foreign key on the parent table + # after validating it on all partitions. + # `validate: false` should be paired with `prepare_partitioned_async_foreign_key_validation` # reverse_lock_order - Flag that controls whether we should attempt to acquire locks in the reverse # order of the ALTER TABLE. This can be useful in situations where the foreign # key creation could deadlock with another process. -- GitLab