From d134a8f9107f76d1468619c25e0085b9b08dce02 Mon Sep 17 00:00:00 2001
From: Adam Hegyi <ahegyi@gitlab.com>
Date: Fri, 7 Jan 2022 09:38:38 +0100
Subject: [PATCH] Make list partitioning strategy safer
This change adds two safeguard to the list partitioning strategy:
- Explicitly lock the table as the first step when adding partitions.
- Do not detach partition when its value is used as the default value
of the partition key.
---
.../partitioning/partition_manager.rb | 4 +++
.../partitioning/sliding_list_strategy.rb | 28 ++++++++++++++++++-
.../partitioning/partition_manager_spec.rb | 3 +-
.../sliding_list_strategy_spec.rb | 7 +++--
4 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/lib/gitlab/database/partitioning/partition_manager.rb b/lib/gitlab/database/partitioning/partition_manager.rb
index aa824dfbd2fb..ba6fa0cf2784 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 21b86b43ae7e..e9865fb91d63 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 5e107109fc98..64dcdb9628a1 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 636a09e57104..1cec04630550 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
--
GitLab