diff --git a/db/post_migrate/20221004074914_create_routing_table_for_builds_metadata.rb b/db/post_migrate/20221004074914_create_routing_table_for_builds_metadata.rb index 71a8625dce24c8a1a06e2a3e37cd9c0e510a3eba..a792fc91d3d38395f33c937fe82c106ff384c107 100644 --- a/db/post_migrate/20221004074914_create_routing_table_for_builds_metadata.rb +++ b/db/post_migrate/20221004074914_create_routing_table_for_builds_metadata.rb @@ -1,30 +1,7 @@ # frozen_string_literal: true class CreateRoutingTableForBuildsMetadata < Gitlab::Database::Migration[2.0] - include Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers + def up; end - disable_ddl_transaction! - - TABLE_NAME = :ci_builds_metadata - PARENT_TABLE_NAME = :p_ci_builds_metadata - FIRST_PARTITION = 100 - PARTITION_COLUMN = :partition_id - - def up - convert_table_to_first_list_partition( - table_name: TABLE_NAME, - partitioning_column: PARTITION_COLUMN, - parent_table_name: PARENT_TABLE_NAME, - initial_partitioning_value: FIRST_PARTITION - ) - end - - def down - revert_converting_table_to_first_list_partition( - table_name: TABLE_NAME, - partitioning_column: PARTITION_COLUMN, - parent_table_name: PARENT_TABLE_NAME, - initial_partitioning_value: FIRST_PARTITION - ) - end + def down; end end diff --git a/db/post_migrate/20221021145820_create_routing_table_for_builds_metadata_v2.rb b/db/post_migrate/20221021145820_create_routing_table_for_builds_metadata_v2.rb new file mode 100644 index 0000000000000000000000000000000000000000..e5f1ba5cb876d56e978513c91e0c625a3cb222cb --- /dev/null +++ b/db/post_migrate/20221021145820_create_routing_table_for_builds_metadata_v2.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +class CreateRoutingTableForBuildsMetadataV2 < Gitlab::Database::Migration[2.0] + include Gitlab::Database::PartitioningMigrationHelpers::TableManagementHelpers + + disable_ddl_transaction! + + TABLE_NAME = :ci_builds_metadata + PARENT_TABLE_NAME = :p_ci_builds_metadata + FIRST_PARTITION = 100 + PARTITION_COLUMN = :partition_id + + def up + return if connection.table_exists?(PARENT_TABLE_NAME) && partition_attached? + + convert_table_to_first_list_partition( + table_name: TABLE_NAME, + partitioning_column: PARTITION_COLUMN, + parent_table_name: PARENT_TABLE_NAME, + initial_partitioning_value: FIRST_PARTITION, + lock_tables: [:ci_builds, :ci_builds_metadata] + ) + end + + def down + revert_converting_table_to_first_list_partition( + table_name: TABLE_NAME, + partitioning_column: PARTITION_COLUMN, + parent_table_name: PARENT_TABLE_NAME, + initial_partitioning_value: FIRST_PARTITION + ) + end + + private + + def partition_attached? + connection.select_value(<<~SQL) + SELECT true FROM postgres_partitions WHERE name = '#{TABLE_NAME}'; + SQL + end +end diff --git a/db/schema_migrations/20221021145820 b/db/schema_migrations/20221021145820 new file mode 100644 index 0000000000000000000000000000000000000000..e3d50c654ba07b92c6295dfd627fbc56e4e024d2 --- /dev/null +++ b/db/schema_migrations/20221021145820 @@ -0,0 +1 @@ +e9fd4d60833624e20fcf9b01b883dca15e6c135aa99f1afd1c7a365eebac17fb \ No newline at end of file diff --git a/lib/gitlab/database/partitioning/convert_table_to_first_list_partition.rb b/lib/gitlab/database/partitioning/convert_table_to_first_list_partition.rb index 23a8dc0b44fe2fcf724a7b197e1a1b0cefe82dbd..58447481e600d47d213ce9b3d18ad6c1bd78b5d2 100644 --- a/lib/gitlab/database/partitioning/convert_table_to_first_list_partition.rb +++ b/lib/gitlab/database/partitioning/convert_table_to_first_list_partition.rb @@ -10,13 +10,17 @@ class ConvertTableToFirstListPartition attr_reader :partitioning_column, :table_name, :parent_table_name, :zero_partition_value - def initialize(migration_context:, table_name:, parent_table_name:, partitioning_column:, zero_partition_value:) + def initialize( + migration_context:, table_name:, parent_table_name:, partitioning_column:, + zero_partition_value:, lock_tables: []) + @migration_context = migration_context @connection = migration_context.connection @table_name = table_name @parent_table_name = parent_table_name @partitioning_column = partitioning_column @zero_partition_value = zero_partition_value + @lock_tables = Array.wrap(lock_tables) end def prepare_for_partitioning @@ -35,7 +39,12 @@ def partition create_parent_table attach_foreign_keys_to_parent - migration_context.with_lock_retries(raise_on_exhaustion: true) do + lock_args = { + raise_on_exhaustion: true, + timing_configuration: lock_timing_configuration + } + + migration_context.with_lock_retries(**lock_args) do migration_context.execute(sql_to_convert_table) end end @@ -74,6 +83,7 @@ def sql_to_convert_table # but they acquire the same locks so it's much faster to incude them # here. [ + lock_tables_statement, attach_table_to_parent_statement, alter_sequence_statements(old_table: table_name, new_table: parent_table_name), remove_constraint_statement @@ -162,6 +172,16 @@ def attach_foreign_keys_to_parent end end + def lock_tables_statement + return if @lock_tables.empty? + + table_names = @lock_tables.map { |name| quote_table_name(name) }.join(', ') + + <<~SQL + LOCK #{table_names} IN ACCESS EXCLUSIVE MODE + SQL + end + def attach_table_to_parent_statement <<~SQL ALTER TABLE #{quote_table_name(parent_table_name)} @@ -235,6 +255,13 @@ def set_current_user_owns_table_statement(table_name) ALTER TABLE #{connection.quote_table_name(table_name)} OWNER TO CURRENT_USER SQL end + + def lock_timing_configuration + iterations = Gitlab::Database::WithLockRetries::DEFAULT_TIMING_CONFIGURATION + aggressive_iterations = Array.new(5) { [10.seconds, 1.minute] } + + iterations + aggressive_iterations + end end end end diff --git a/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb b/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb index 695a5d7ec7757843911a72e0e1877de9a8dfa0cf..f9790bf53b93c3a81fabf08e1ad400955b66fcfc 100644 --- a/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb +++ b/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb @@ -275,7 +275,7 @@ def revert_preparing_constraint_for_list_partitioning(table_name:, partitioning_ ).revert_preparation_for_partitioning end - def convert_table_to_first_list_partition(table_name:, partitioning_column:, parent_table_name:, initial_partitioning_value:) + def convert_table_to_first_list_partition(table_name:, partitioning_column:, parent_table_name:, initial_partitioning_value:, lock_tables: []) validate_not_in_transaction!(:convert_table_to_first_list_partition) Gitlab::Database::Partitioning::ConvertTableToFirstListPartition @@ -283,7 +283,8 @@ def convert_table_to_first_list_partition(table_name:, partitioning_column:, par table_name: table_name, parent_table_name: parent_table_name, partitioning_column: partitioning_column, - zero_partition_value: initial_partitioning_value + zero_partition_value: initial_partitioning_value, + lock_tables: lock_tables ).partition end diff --git a/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb b/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb index 0e804b4feac08a70949fbced12807cfb728b0489..cd3a94f5737fc27c16129c7c114cef3c311da8e9 100644 --- a/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb +++ b/spec/lib/gitlab/database/partitioning/convert_table_to_first_list_partition_spec.rb @@ -16,6 +16,7 @@ let(:referenced_table_name) { '_test_referenced_table' } let(:other_referenced_table_name) { '_test_other_referenced_table' } let(:parent_table_name) { "#{table_name}_parent" } + let(:lock_tables) { [] } let(:model) { define_batchable_model(table_name, connection: connection) } @@ -27,7 +28,8 @@ table_name: table_name, partitioning_column: partitioning_column, parent_table_name: parent_table_name, - zero_partition_value: partitioning_default + zero_partition_value: partitioning_default, + lock_tables: lock_tables ) end @@ -168,6 +170,16 @@ end end + context 'with locking tables' do + let(:lock_tables) { [table_name] } + + it 'locks the table' do + recorder = ActiveRecord::QueryRecorder.new { partition } + + expect(recorder.log).to include(/LOCK "_test_table_to_partition" IN ACCESS EXCLUSIVE MODE/) + end + end + context 'when an error occurs during the conversion' do def fail_first_time # We can't directly use a boolean here, as we need something that will be passed by-reference to the proc diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb index 8bb9ad2737a8e92856361eac4ddcf5c88642968d..e76b1da3834670f734c184127276964e99a08a00 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers_spec.rb @@ -43,6 +43,7 @@ context 'list partitioning conversion helpers' do shared_examples_for 'delegates to ConvertTableToFirstListPartition' do + let(:extra_options) { {} } it 'throws an error if in a transaction' do allow(migration).to receive(:transaction_open?).and_return(true) expect { migrate }.to raise_error(/cannot be run inside a transaction/) @@ -54,7 +55,8 @@ table_name: source_table, parent_table_name: partitioned_table, partitioning_column: partition_column, - zero_partition_value: min_date) do |converter| + zero_partition_value: min_date, + **extra_options) do |converter| expect(converter).to receive(expected_method) end @@ -64,12 +66,15 @@ describe '#convert_table_to_first_list_partition' do it_behaves_like 'delegates to ConvertTableToFirstListPartition' do + let(:lock_tables) { [source_table] } + let(:extra_options) { { lock_tables: lock_tables } } let(:expected_method) { :partition } let(:migrate) do migration.convert_table_to_first_list_partition(table_name: source_table, partitioning_column: partition_column, parent_table_name: partitioned_table, - initial_partitioning_value: min_date) + initial_partitioning_value: min_date, + lock_tables: lock_tables) end end end diff --git a/spec/migrations/20221021145820_create_routing_table_for_builds_metadata_v2_spec.rb b/spec/migrations/20221021145820_create_routing_table_for_builds_metadata_v2_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..48a00df430d96f1da69131bdfa132171df33e7a6 --- /dev/null +++ b/spec/migrations/20221021145820_create_routing_table_for_builds_metadata_v2_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe CreateRoutingTableForBuildsMetadataV2, :migration do + let_it_be(:migration) { described_class.new } + + describe '#up' do + context 'when the table is already partitioned' do + before do + # `convert_table_to_first_list_partition` checks if it's being executed + # inside a transaction, but we're using transactional fixtures here so we + # need to tell it that it's not inside a transaction. + # We toggle the behavior depending on how many transactions we have open + # instead of just returning `false` because the migration could have the + # DDL transaction enabled. + # + open_transactions = ActiveRecord::Base.connection.open_transactions + allow(migration).to receive(:transaction_open?) do + ActiveRecord::Base.connection.open_transactions > open_transactions + end + + migration.convert_table_to_first_list_partition( + table_name: :ci_builds_metadata, + partitioning_column: :partition_id, + parent_table_name: :p_ci_builds_metadata, + initial_partitioning_value: 100) + end + + it 'skips the migration' do + expect { migrate! }.not_to raise_error + end + end + end +end