Add latest changes from gitlab-org/gitlab@master

7 files changed, 68 insertions, 12 deletions

diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index a4190514de2..a477f40697c 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -380,6 +380,8 @@ module Gitlab
       # The timings can be controlled via the +timing_configuration+ parameter.
       # If the lock was not acquired within the retry period, a last attempt is made without using +lock_timeout+.
       #
+      # Note this helper uses subtransactions when run inside an already open transaction.
+      #
       # ==== Examples
       #   # Invoking without parameters
       #   with_lock_retries do
@@ -411,7 +413,8 @@ module Gitlab
         raise_on_exhaustion = !!kwargs.delete(:raise_on_exhaustion)
         merged_args = {
           klass: self.class,
-          logger: Gitlab::BackgroundMigration::Logger
+          logger: Gitlab::BackgroundMigration::Logger,
+          allow_savepoints: true
         }.merge(kwargs)
 
         Gitlab::Database::WithLockRetries.new(**merged_args)
@@ -1376,13 +1379,11 @@ into similar problems in the future (e.g. when new tables are created).
       # validate - Whether to validate the constraint in this call
       #
       def add_check_constraint(table, check, constraint_name, validate: true)
-        validate_check_constraint_name!(constraint_name)
-
         # 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_check_constraint can not be run inside a transaction'
-        end
+        validate_not_in_transaction!(:add_check_constraint)
+
+        validate_check_constraint_name!(constraint_name)
 
         if check_constraint_exists?(table, constraint_name)
           warning_message = <<~MESSAGE
@@ -1427,6 +1428,10 @@ into similar problems in the future (e.g. when new tables are created).
       end
 
       def remove_check_constraint(table, constraint_name)
+        # This is technically not necessary, but aligned with add_check_constraint
+        # and allows us to continue use with_lock_retries here
+        validate_not_in_transaction!(:remove_check_constraint)
+
         validate_check_constraint_name!(constraint_name)
 
         # DROP CONSTRAINT requires an EXCLUSIVE lock
diff --git a/lib/gitlab/database/migration_helpers/v2.rb b/lib/gitlab/database/migration_helpers/v2.rb
index f20a9b30fa7..79d69676c44 100644
--- a/lib/gitlab/database/migration_helpers/v2.rb
+++ b/lib/gitlab/database/migration_helpers/v2.rb
@@ -6,6 +6,44 @@ module Gitlab
       module V2
         include Gitlab::Database::MigrationHelpers
 
+        # Executes the block with a retry mechanism that alters the +lock_timeout+ and +sleep_time+ between attempts.
+        # The timings can be controlled via the +timing_configuration+ parameter.
+        # If the lock was not acquired within the retry period, a last attempt is made without using +lock_timeout+.
+        #
+        # In order to retry the block, the method wraps the block into a transaction.
+        # Note it cannot be used inside an already open transaction and will raise an error in that case.
+        #
+        # ==== Examples
+        #   # Invoking without parameters
+        #   with_lock_retries do
+        #     drop_table :my_table
+        #   end
+        #
+        #   # Invoking with custom +timing_configuration+
+        #   t = [
+        #     [1.second, 1.second],
+        #     [2.seconds, 2.seconds]
+        #   ]
+        #
+        #   with_lock_retries(timing_configuration: t) do
+        #     drop_table :my_table # this will be retried twice
+        #   end
+        #
+        #   # Disabling the retries using an environment variable
+        #   > export DISABLE_LOCK_RETRIES=true
+        #
+        #   with_lock_retries do
+        #     drop_table :my_table # one invocation, it will not retry at all
+        #   end
+        #
+        # ==== Parameters
+        # * +timing_configuration+ - [[ActiveSupport::Duration, ActiveSupport::Duration], ...] lock timeout for the block, sleep time before the next iteration, defaults to `Gitlab::Database::WithLockRetries::DEFAULT_TIMING_CONFIGURATION`
+        # * +logger+ - [Gitlab::JsonLogger]
+        # * +env+ - [Hash] custom environment hash, see the example with `DISABLE_LOCK_RETRIES`
+        def with_lock_retries(*args, **kwargs, &block)
+          super(*args, **kwargs.merge(allow_savepoints: false), &block)
+        end
+
         # Renames a column without requiring downtime.
         #
         # Concurrent renames work by using database triggers to ensure both the
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 f1aa7871245..bd8ed677d77 100644
--- a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb
+++ b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb
@@ -6,6 +6,8 @@ module Gitlab
       module ForeignKeyHelpers
         include ::Gitlab::Database::SchemaHelpers
 
+        ERROR_SCOPE = 'foreign keys'
+
         # Adds a foreign key with only minimal locking on the tables involved.
         #
         # In concept it works similarly to add_concurrent_foreign_key, but we have
@@ -32,6 +34,8 @@ module Gitlab
         # name - The name of the foreign key.
         #
         def add_concurrent_partitioned_foreign_key(source, target, column:, on_delete: :cascade, name: nil)
+          assert_not_in_transaction_block(scope: ERROR_SCOPE)
+
           partition_options = {
             column: column,
             on_delete: on_delete,
diff --git a/lib/gitlab/database/partitioning_migration_helpers/index_helpers.rb b/lib/gitlab/database/partitioning_migration_helpers/index_helpers.rb
index c0cc97de276..c9a3b5caf79 100644
--- a/lib/gitlab/database/partitioning_migration_helpers/index_helpers.rb
+++ b/lib/gitlab/database/partitioning_migration_helpers/index_helpers.rb
@@ -7,6 +7,8 @@ module Gitlab
         include Gitlab::Database::MigrationHelpers
         include Gitlab::Database::SchemaHelpers
 
+        ERROR_SCOPE = 'index'
+
         # Concurrently creates a new index on a partitioned table. In concept this works similarly to
         # `add_concurrent_index`, and won't block reads or writes on the table while the index is being built.
         #
@@ -21,6 +23,8 @@ module Gitlab
         #
         # See Rails' `add_index` for more info on the available arguments.
         def add_concurrent_partitioned_index(table_name, column_names, options = {})
+          assert_not_in_transaction_block(scope: ERROR_SCOPE)
+
           raise ArgumentError, 'A name is required for indexes added to partitioned tables' unless options[:name]
 
           partitioned_table = find_partitioned_table(table_name)
@@ -57,6 +61,8 @@ module Gitlab
         #
         #     remove_concurrent_partitioned_index_by_name :users, 'index_name_goes_here'
         def remove_concurrent_partitioned_index_by_name(table_name, index_name)
+          assert_not_in_transaction_block(scope: ERROR_SCOPE)
+
           find_partitioned_table(table_name)
 
           unless index_name_exists?(table_name, index_name)
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 9ccbdc9930e..0dc9f92e4c8 100644
--- a/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb
+++ b/lib/gitlab/database/partitioning_migration_helpers/table_management_helpers.rb
@@ -431,7 +431,7 @@ module Gitlab
           replace_table = Gitlab::Database::Partitioning::ReplaceTable.new(original_table_name.to_s,
               replacement_table_name, replaced_table_name, primary_key_name)
 
-          with_lock_retries do
+          transaction do
             drop_sync_trigger(original_table_name)
 
             replace_table.perform do |sql|
diff --git a/lib/gitlab/database/rename_table_helpers.rb b/lib/gitlab/database/rename_table_helpers.rb
index 7f5af038c6d..e881c0e5455 100644
--- a/lib/gitlab/database/rename_table_helpers.rb
+++ b/lib/gitlab/database/rename_table_helpers.rb
@@ -4,27 +4,27 @@ module Gitlab
   module Database
     module RenameTableHelpers
       def rename_table_safely(old_table_name, new_table_name)
-        with_lock_retries do
+        transaction do
           rename_table(old_table_name, new_table_name)
           execute("CREATE VIEW #{old_table_name} AS SELECT * FROM #{new_table_name}")
         end
       end
 
       def undo_rename_table_safely(old_table_name, new_table_name)
-        with_lock_retries do
+        transaction do
           execute("DROP VIEW IF EXISTS #{old_table_name}")
           rename_table(new_table_name, old_table_name)
         end
       end
 
       def finalize_table_rename(old_table_name, new_table_name)
-        with_lock_retries do
+        transaction do
           execute("DROP VIEW IF EXISTS #{old_table_name}")
         end
       end
 
       def undo_finalize_table_rename(old_table_name, new_table_name)
-        with_lock_retries do
+        transaction do
           execute("CREATE VIEW #{old_table_name} AS SELECT * FROM #{new_table_name}")
         end
       end
diff --git a/lib/gitlab/database/with_lock_retries.rb b/lib/gitlab/database/with_lock_retries.rb
index 70acbeac9e1..e55390e679a 100644
--- a/lib/gitlab/database/with_lock_retries.rb
+++ b/lib/gitlab/database/with_lock_retries.rb
@@ -61,9 +61,10 @@ module Gitlab
         [10.seconds, 10.minutes]
       ].freeze
 
-      def initialize(logger: NULL_LOGGER, timing_configuration: DEFAULT_TIMING_CONFIGURATION, klass: nil, env: ENV)
+      def initialize(logger: NULL_LOGGER, allow_savepoints: true, timing_configuration: DEFAULT_TIMING_CONFIGURATION, klass: nil, env: ENV)
         @logger = logger
         @klass = klass
+        @allow_savepoints = allow_savepoints
         @timing_configuration = timing_configuration
         @env = env
         @current_iteration = 1
@@ -122,6 +123,8 @@ module Gitlab
       end
 
       def run_block_with_lock_timeout
+        raise "WithLockRetries should not run inside already open transaction" if ActiveRecord::Base.connection.transaction_open? && @allow_savepoints.blank?
+
         ActiveRecord::Base.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
           execute("SET LOCAL lock_timeout TO '#{current_lock_timeout_in_ms}ms'")