diff --git a/lib/gitlab/database/partitioning/partition_manager.rb b/lib/gitlab/database/partitioning/partition_manager.rb index aa824dfbd2fb8b700d3c6f39f72e60bfda0dd37c..ba6fa0cf27842e998c0118633a4567164b2cbb4e 100644 --- a/lib/gitlab/database/partitioning/partition_manager.rb +++ b/lib/gitlab/database/partitioning/partition_manager.rb @@ -64,6 +64,10 @@ def create(partitions) # with_lock_retries starts a requires_new transaction most of the time, but not on the last iteration with_lock_retries do connection.transaction(requires_new: false) do # so we open a transaction here if not already in progress + # Partitions might not get created (IF NOT EXISTS) so explicit locking will not happen. + # This LOCK TABLE ensures to have exclusive lock as the first step. + connection.execute "LOCK TABLE #{connection.quote_table_name(model.table_name)} IN ACCESS EXCLUSIVE MODE" + partitions.each do |partition| connection.execute partition.to_sql diff --git a/lib/gitlab/database/partitioning/sliding_list_strategy.rb b/lib/gitlab/database/partitioning/sliding_list_strategy.rb index 21b86b43ae7e8aa5c8f343ca6a7307e367cbf525..e9865fb91d630ef804135e4a4c7f273011782510 100644 --- a/lib/gitlab/database/partitioning/sliding_list_strategy.rb +++ b/lib/gitlab/database/partitioning/sliding_list_strategy.rb @@ -44,7 +44,18 @@ def next_partition def extra_partitions possibly_extra = current_partitions[0...-1] # Never consider the most recent partition - possibly_extra.take_while { |p| detach_partition_if.call(p.value) } + extra = possibly_extra.take_while { |p| detach_partition_if.call(p.value) } + + default_value = current_default_value + if extra.any? { |p| p.value == default_value } + Gitlab::AppLogger.error(message: "Inconsistent partition detected: partition with value #{current_default_value} should not be deleted because it's used as the default value.", + partition_number: current_default_value, + table_name: model.table_name) + + extra = extra.reject { |p| p.value == default_value } + end + + extra end def after_adding_partitions @@ -64,6 +75,21 @@ def no_partitions_exist? private + def current_default_value + column_name = model.connection.quote(partitioning_key) + table_name = model.connection.quote(model.table_name) + + value = model.connection.select_value <<~SQL + SELECT columns.column_default AS default_value + FROM information_schema.columns columns + WHERE columns.column_name = #{column_name} AND columns.table_name = #{table_name} + SQL + + raise "No default value found for the #{partitioning_key} column within #{model.name}" if value.nil? + + Integer(value) + end + def ensure_partitioning_column_ignored! unless model.ignored_columns.include?(partitioning_key.to_s) raise "Add #{partitioning_key} to #{model.name}.ignored_columns to use it with SlidingListStrategy" diff --git a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb index 5e107109fc98b2710ba29d497fb3bcef20f499d3..64dcdb9628a1d2230db565f00909de6edd1665c6 100644 --- a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb @@ -18,7 +18,7 @@ def has_partition(model, month) let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table, connection: connection) } let(:partitioning_strategy) { double(missing_partitions: partitions, extra_partitions: [], after_adding_partitions: nil) } let(:connection) { ActiveRecord::Base.connection } - let(:table) { "some_table" } + let(:table) { "issues" } before do allow(connection).to receive(:table_exists?).and_call_original @@ -36,6 +36,7 @@ def has_partition(model, month) end it 'creates the partition' do + expect(connection).to receive(:execute).with("LOCK TABLE \"#{table}\" IN ACCESS EXCLUSIVE MODE") expect(connection).to receive(:execute).with(partitions.first.to_sql) expect(connection).to receive(:execute).with(partitions.second.to_sql) diff --git a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb index 636a09e571043114a8e3a6df927f712ec5824118..1cec04630550fab57b8bc5d50fc78b6e561302b4 100644 --- a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy do let(:connection) { ActiveRecord::Base.connection } let(:table_name) { :_test_partitioned_test } - let(:model) { double('model', table_name: table_name, ignored_columns: %w[partition]) } + let(:model) { double('model', table_name: table_name, ignored_columns: %w[partition], connection: connection) } let(:next_partition_if) { double('next_partition_if') } let(:detach_partition_if) { double('detach_partition_if') } @@ -94,7 +94,8 @@ let(:detach_partition_if) { ->(p) { p != 5 } } it 'is the leading set of partitions before that value' do - expect(strategy.extra_partitions.map(&:value)).to contain_exactly(1, 2, 3, 4) + # should not contain partition 2 since it's the default value for the partition column + expect(strategy.extra_partitions.map(&:value)).to contain_exactly(1, 3, 4) end end @@ -102,7 +103,7 @@ let(:detach_partition_if) { proc { true } } it 'is all but the most recent partition', :aggregate_failures do - expect(strategy.extra_partitions.map(&:value)).to contain_exactly(1, 2, 3, 4, 5, 6, 7, 8, 9) + expect(strategy.extra_partitions.map(&:value)).to contain_exactly(1, 3, 4, 5, 6, 7, 8, 9) expect(strategy.current_partitions.map(&:value).max).to eq(10) end