Merge branch 'make-sliding-list-partitioning-safer' into 'master'

Make list partitioning strategy safer

See merge request gitlab-org/gitlab!77744

lib/gitlab/database/partitioning/partition_manager.rb

@@ -64,6 +64,10 @@ module Gitlab
           # 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
 

lib/gitlab/database/partitioning/sliding_list_strategy.rb

@@ -44,7 +44,18 @@ module Gitlab
         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 @@ module Gitlab
 
         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"

spec/lib/gitlab/database/partitioning/partition_manager_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do
     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 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do
     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)
 

spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 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 @@ RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy do
       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 @@ RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy do
       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