From 7afd4211ed90c076b80bd4802294a643fcdc267f Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Wed, 20 Aug 2025 14:36:42 -0300 Subject: [PATCH 01/12] Run prepared async statements synchronously This add classes to run prepared statements synchronously during migration testing Changelog: changed # Conflicts: # lib/tasks/gitlab/db.rake --- .../avoid_using_connection_execute.yml | 4 + .../async_foreign_key_operations.rb | 54 +++++++++++++ .../async_index_operations.rb | 29 +++++++ .../async_operations_runner.rb | 77 +++++++++++++++++++ .../base_async_operations.rb | 24 ++++++ lib/gitlab/database/migrations/runner.rb | 6 ++ 6 files changed, 194 insertions(+) create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb diff --git a/.rubocop_todo/database/avoid_using_connection_execute.yml b/.rubocop_todo/database/avoid_using_connection_execute.yml index 8e7ec4c68e963a..bb03074f346817 100644 --- a/.rubocop_todo/database/avoid_using_connection_execute.yml +++ b/.rubocop_todo/database/avoid_using_connection_execute.yml @@ -1,5 +1,6 @@ --- Database/AvoidUsingConnectionExecute: + Details: grace period Exclude: - 'app/models/analytics/cycle_analytics/stage_event_hash.rb' - 'app/models/concerns/analytics/cycle_analytics/stage_event_model.rb' @@ -27,6 +28,9 @@ Database/AvoidUsingConnectionExecute: - 'lib/gitlab/database/lock_writes_manager.rb' - 'lib/gitlab/database/migrations/observers/query_statistics.rb' - 'lib/gitlab/database/migrations/observers/total_database_size_change.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb' - 'lib/gitlab/database/partitioning/detached_partition_dropper.rb' - 'lib/gitlab/database/partitioning/partition_manager.rb' - 'lib/gitlab/database/partitioning_migration_helpers/bulk_copy.rb' diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb new file mode 100644 index 00000000000000..48d12c73d1578f --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + class AsyncForeignKeyOperations < BaseAsyncOperations + FK_PREFIX = '_tmp_async' + + def execute + records.each do |tmp_table_name, fks| + copy_fks_from_source(tmp_table_name, fks.first.table_name) + + fks.each do |fk| + name = "#{FK_PREFIX}_#{fk.name}" + + next unless constraint_exists?(tmp_table_name, name) + + connection.execute(<<~SQL.squish) + ALTER TABLE #{connection.quote_table_name(tmp_table_name)} + VALIDATE CONSTRAINT #{connection.quote_column_name(name)}; + SQL + end + end + end + + private + + def constraint_exists?(table_name, constraint_name) + Gitlab::Database::PostgresForeignKey + .by_constrained_table_name_or_identifier(table_name) + .by_name(constraint_name) + .exists? + end + + def copy_fks_from_source(tmp_table_name, source_table_name) + fks = connection.select_rows(<<~SQL.squish) + SELECT FORMAT( + 'ALTER TABLE #{connection.quote_table_name(tmp_table_name)} ADD CONSTRAINT %I %s', + '#{FK_PREFIX}_' || conname, + PG_GET_CONSTRAINTDEF(pg_constraint.oid) + ) + FROM pg_constraint + WHERE contype = 'f' + AND conrelid = '#{source_table_name}'::regclass; + SQL + + connection.execute(fks.join("; \n")) + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb new file mode 100644 index 00000000000000..fddd6382a28c6e --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + class AsyncIndexOperations < BaseAsyncOperations + INDEX_DEFINITION_REGEX = /(\bON\s+)("[^"]+"|[^\s(]+)/i + + def execute + records.each do |tmp_table_name, indexes| + indexes.each do |index| + next if connection.index_exists?(tmp_table_name, index.name) + + connection.execute(parse_index_definition(index.definition, tmp_table_name)) + end + end + end + + private + + def parse_index_definition(definition, test_table_name) + definition.gsub(INDEX_DEFINITION_REGEX, "\\1#{connection.quote_table_name(test_table_name)}") + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb new file mode 100644 index 00000000000000..900923cd8f4609 --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + # This class execute async DML operations synchronously. We perform this by copying the + # target table and executing the async operation against the test table. + class AsyncOperationsRunner + TMP_TABLE_PREFIX = '_tmp_async' + + def initialize(connection) + @connection = connection + end + + def execute + create_tmp_tables + AsyncIndexOperations.new(connection, indexes).execute + AsyncForeignKeyOperations.new(connection, foreign_keys).execute + ensure + drop_tmp_tables + end + + private + + attr_reader :connection + + def table_names + names = Set.new + + indexes.each { |tmp_table_name, index| names << [tmp_table_name, index.first.table_name] } + foreign_keys.each { |tmp_table_name, fk| names << [tmp_table_name, fk.first.table_name] } + + names + end + + def indexes + Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_create.ordered.group_by do |index| + tmp_table_name(index.table_name) + end + end + + def foreign_keys + Gitlab::Database::AsyncConstraints::PostgresAsyncConstraintValidation.foreign_key_type.group_by do |fk| + tmp_table_name(fk.table_name) + end + end + + def tmp_table_name(table_name) + "#{TMP_TABLE_PREFIX}_#{table_name}" + end + + def create_tmp_tables + table_names.each do |tmp_table_name, table_name| + next if connection.table_exists?(tmp_table_name) + + next unless connection.table_exists?(table_name) + + connection.execute(<<~SQL.squish) + CREATE TABLE #{connection.quote_table_name(tmp_table_name)} + (LIKE #{connection.quote_table_name(table_name)} INCLUDING ALL) + SQL + end + end + + def drop_tmp_tables + table_names.each_key do |tmp_table_name| + next unless connection.table_exists?(tmp_table_name) + + connection.execute("DROP TABLE #{connection.quote_table_name(tmp_table_name)} CASCADE") + end + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb new file mode 100644 index 00000000000000..f2c66fb92e3ed2 --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + class BaseAsyncOperations + def initialize(connection, records) + @connection = connection + @records = records + end + + def execute + raise NotImplementedError + end + + private + + attr_reader :connection, :records + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/runner.rb b/lib/gitlab/database/migrations/runner.rb index 2cc689b558b0a1..1a77d975134675 100644 --- a/lib/gitlab/database/migrations/runner.rb +++ b/lib/gitlab/database/migrations/runner.rb @@ -76,6 +76,12 @@ def batched_migrations_last_id(for_database) runner end + def execute_prepared_async_dml_operations!(database) + Gitlab::Database::EachDatabase.each_connection(only: database) do |connection| + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.new(connection).execute + end + end + private def migrations_for_up(database) -- GitLab From b8e332691cafe92f528404b9706e8d8ad235cfd2 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Wed, 17 Sep 2025 10:33:45 -0300 Subject: [PATCH 02/12] Patches the async methods to execute FKs and index during testing --- .../avoid_using_connection_execute.yml | 5 +- .../database/rescue_query_canceled.yml | 2 + .../database/rescue_statement_timeout.yml | 2 + .../async_foreign_key_operations.rb | 54 ------------- .../async_index_operations.rb | 29 ------- .../async_operations_helper.rb | 74 ++++++++++++++++++ .../async_operations_runner.rb | 77 ------------------- .../base_async_operations.rb | 24 ------ .../foreign_key_validator.rb | 35 +++++++++ .../index_creator.rb | 33 ++++++++ lib/gitlab/database/migrations/runner.rb | 7 +- 11 files changed, 153 insertions(+), 189 deletions(-) delete mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb delete mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb delete mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb delete mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb diff --git a/.rubocop_todo/database/avoid_using_connection_execute.yml b/.rubocop_todo/database/avoid_using_connection_execute.yml index bb03074f346817..a644f9ed223c4e 100644 --- a/.rubocop_todo/database/avoid_using_connection_execute.yml +++ b/.rubocop_todo/database/avoid_using_connection_execute.yml @@ -28,9 +28,8 @@ Database/AvoidUsingConnectionExecute: - 'lib/gitlab/database/lock_writes_manager.rb' - 'lib/gitlab/database/migrations/observers/query_statistics.rb' - 'lib/gitlab/database/migrations/observers/total_database_size_change.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' - 'lib/gitlab/database/partitioning/detached_partition_dropper.rb' - 'lib/gitlab/database/partitioning/partition_manager.rb' - 'lib/gitlab/database/partitioning_migration_helpers/bulk_copy.rb' diff --git a/.rubocop_todo/database/rescue_query_canceled.yml b/.rubocop_todo/database/rescue_query_canceled.yml index c04721ec9d293e..c06d3fdf10201c 100644 --- a/.rubocop_todo/database/rescue_query_canceled.yml +++ b/.rubocop_todo/database/rescue_query_canceled.yml @@ -3,4 +3,6 @@ Database/RescueQueryCanceled: Exclude: - 'app/services/issues/relative_position_rebalancing_service.rb' - 'lib/gitlab/database/batch_counter.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' - 'lib/gitlab/issuables_count_for_state.rb' diff --git a/.rubocop_todo/database/rescue_statement_timeout.yml b/.rubocop_todo/database/rescue_statement_timeout.yml index ce8e834e93ce4f..fc1a55d046613c 100644 --- a/.rubocop_todo/database/rescue_statement_timeout.yml +++ b/.rubocop_todo/database/rescue_statement_timeout.yml @@ -2,3 +2,5 @@ Database/RescueStatementTimeout: Exclude: - 'app/services/issues/relative_position_rebalancing_service.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb' + - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb deleted file mode 100644 index 48d12c73d1578f..00000000000000 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_foreign_key_operations.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Database - module Migrations - module PreparedAsyncDmlOperationsTesting - class AsyncForeignKeyOperations < BaseAsyncOperations - FK_PREFIX = '_tmp_async' - - def execute - records.each do |tmp_table_name, fks| - copy_fks_from_source(tmp_table_name, fks.first.table_name) - - fks.each do |fk| - name = "#{FK_PREFIX}_#{fk.name}" - - next unless constraint_exists?(tmp_table_name, name) - - connection.execute(<<~SQL.squish) - ALTER TABLE #{connection.quote_table_name(tmp_table_name)} - VALIDATE CONSTRAINT #{connection.quote_column_name(name)}; - SQL - end - end - end - - private - - def constraint_exists?(table_name, constraint_name) - Gitlab::Database::PostgresForeignKey - .by_constrained_table_name_or_identifier(table_name) - .by_name(constraint_name) - .exists? - end - - def copy_fks_from_source(tmp_table_name, source_table_name) - fks = connection.select_rows(<<~SQL.squish) - SELECT FORMAT( - 'ALTER TABLE #{connection.quote_table_name(tmp_table_name)} ADD CONSTRAINT %I %s', - '#{FK_PREFIX}_' || conname, - PG_GET_CONSTRAINTDEF(pg_constraint.oid) - ) - FROM pg_constraint - WHERE contype = 'f' - AND conrelid = '#{source_table_name}'::regclass; - SQL - - connection.execute(fks.join("; \n")) - end - end - end - end - end -end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb deleted file mode 100644 index fddd6382a28c6e..00000000000000 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_index_operations.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Database - module Migrations - module PreparedAsyncDmlOperationsTesting - class AsyncIndexOperations < BaseAsyncOperations - INDEX_DEFINITION_REGEX = /(\bON\s+)("[^"]+"|[^\s(]+)/i - - def execute - records.each do |tmp_table_name, indexes| - indexes.each do |index| - next if connection.index_exists?(tmp_table_name, index.name) - - connection.execute(parse_index_definition(index.definition, tmp_table_name)) - end - end - end - - private - - def parse_index_definition(definition, test_table_name) - definition.gsub(INDEX_DEFINITION_REGEX, "\\1#{connection.quote_table_name(test_table_name)}") - end - end - end - end - end -end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb new file mode 100644 index 00000000000000..9a33a5fdcee746 --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + module AsyncIndexMixin + extend ActiveSupport::Concern + + def prepare_async_index(...) + store_index(super) + end + + def prepare_async_index_from_sql(...) + store_index(super) + end + + def prepare_async_index_removal(...) + store_index(super) + end + + private + + def store_index(async_index) + return unless async_index + + Thread.current[:async_indexes].push(async_index) + end + end + + module ForeignKeyMixin + extend ActiveSupport::Concern + + def prepare_async_foreign_key_validation(...) + store_fk(super) + end + + private + + def store_fk(fk) + return unless fk + + Thread.current[:async_fks].push(fk) + end + end + + class AsyncOperationsHelper + def self.install! + Thread.current[:async_indexes] = [] + Thread.current[:async_fks] = [] + + Gitlab::Database::AsyncIndexes::MigrationHelpers.prepend(AsyncIndexMixin) + Gitlab::Database::AsyncConstraints::MigrationHelpers.prepend(ForeignKeyMixin) + end + end + + class AsyncOperationsRunner + def self.execute! + Thread.current[:async_indexes].each do |async_index| + PreparedAsyncDmlOperationsTesting::IndexCreator.new(async_index).perform + end + + Thread.current[:async_fks].each do |async_fk| + PreparedAsyncDmlOperationsTesting::ForeignKeyValidator.new(async_fk).perform + end + + Thread.current[:async_indexes] = [] + Thread.current[:async_fks] = [] + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb deleted file mode 100644 index 900923cd8f4609..00000000000000 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb +++ /dev/null @@ -1,77 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Database - module Migrations - module PreparedAsyncDmlOperationsTesting - # This class execute async DML operations synchronously. We perform this by copying the - # target table and executing the async operation against the test table. - class AsyncOperationsRunner - TMP_TABLE_PREFIX = '_tmp_async' - - def initialize(connection) - @connection = connection - end - - def execute - create_tmp_tables - AsyncIndexOperations.new(connection, indexes).execute - AsyncForeignKeyOperations.new(connection, foreign_keys).execute - ensure - drop_tmp_tables - end - - private - - attr_reader :connection - - def table_names - names = Set.new - - indexes.each { |tmp_table_name, index| names << [tmp_table_name, index.first.table_name] } - foreign_keys.each { |tmp_table_name, fk| names << [tmp_table_name, fk.first.table_name] } - - names - end - - def indexes - Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_create.ordered.group_by do |index| - tmp_table_name(index.table_name) - end - end - - def foreign_keys - Gitlab::Database::AsyncConstraints::PostgresAsyncConstraintValidation.foreign_key_type.group_by do |fk| - tmp_table_name(fk.table_name) - end - end - - def tmp_table_name(table_name) - "#{TMP_TABLE_PREFIX}_#{table_name}" - end - - def create_tmp_tables - table_names.each do |tmp_table_name, table_name| - next if connection.table_exists?(tmp_table_name) - - next unless connection.table_exists?(table_name) - - connection.execute(<<~SQL.squish) - CREATE TABLE #{connection.quote_table_name(tmp_table_name)} - (LIKE #{connection.quote_table_name(table_name)} INCLUDING ALL) - SQL - end - end - - def drop_tmp_tables - table_names.each_key do |tmp_table_name| - next unless connection.table_exists?(tmp_table_name) - - connection.execute("DROP TABLE #{connection.quote_table_name(tmp_table_name)} CASCADE") - end - end - end - end - end - end -end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb deleted file mode 100644 index f2c66fb92e3ed2..00000000000000 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/base_async_operations.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Database - module Migrations - module PreparedAsyncDmlOperationsTesting - class BaseAsyncOperations - def initialize(connection, records) - @connection = connection - @records = records - end - - def execute - raise NotImplementedError - end - - private - - attr_reader :connection, :records - end - end - end - end -end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb new file mode 100644 index 00000000000000..945bf4603b5305 --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + # This is a wrapper for Gitlab::Database::AsyncConstraints::Validators::ForeignKey, used to control the + # STATEMENT_TIMEOUT of index creating + class ForeignKeyValidator < Gitlab::Database::AsyncConstraints::Validators::ForeignKey + private + + override :validate_constraint_with_error_handling + def validate_constraint_with_error_handling + validate_constraint + rescue ActiveRecord::StatementTimeout, ActiveRecord::QueryCanceled, ActiveRecord::AdapterTimeout, + ActiveRecord::LockWaitTimeout => error + + Gitlab::AppJsonLogger.error(error: error, foreign_key: record.name) + ensure + # We ensure the record is destroyed in all scenarios + record.destroy! + end + + override :set_statement_timeout + def set_statement_timeout + connection.execute(format("SET statement_timeout TO '%ds'", 3.minutes)) + yield + ensure + connection.execute('RESET statement_timeout') + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb new file mode 100644 index 00000000000000..71ec7a6d71bb82 --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + # This is a wrapper for Gitlab::Database::AsyncIndexes::IndexCreator, used to control the STATEMENT_TIMEOUT + # of index creating + class IndexCreator < Gitlab::Database::AsyncIndexes::IndexCreator + private + + def execute_action_with_error_handling + around_execution { execute_action } + rescue ActiveRecord::StatementTimeout, ActiveRecord::QueryCanceled, ActiveRecord::AdapterTimeout, + ActiveRecord::LockWaitTimeout => error + + Gitlab::AppLogger.error(error: error, index: async_index.name) + ensure + # We ensure the record is destroyed in all scenarios + async_index.destroy! + end + + def set_statement_timeout + connection.execute(format("SET statement_timeout TO '%ds'", 3.minutes)) + yield + ensure + connection.execute('RESET statement_timeout') + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/runner.rb b/lib/gitlab/database/migrations/runner.rb index 1a77d975134675..6bd41303d3b2de 100644 --- a/lib/gitlab/database/migrations/runner.rb +++ b/lib/gitlab/database/migrations/runner.rb @@ -77,8 +77,8 @@ def batched_migrations_last_id(for_database) end def execute_prepared_async_dml_operations!(database) - Gitlab::Database::EachDatabase.each_connection(only: database) do |connection| - Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.new(connection).execute + Gitlab::Database::EachDatabase.each_connection(only: database) do |_connection| + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.execute end end @@ -139,10 +139,13 @@ def run instrumentation = Instrumentation.new(result_dir: result_dir) + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsHelper.install! + within_context_for_database(@database) do sorted_migrations.each do |migration| instrumentation.observe(version: migration.version, name: migration.name, connection: ActiveRecord::Migration.connection) do ActiveRecord::Migrator.new(direction, migration_context.migrations, migration_context.schema_migration, migration_context.internal_metadata, migration.version).run + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.execute! end end end -- GitLab From 653de02320540e14787798e27a67cac764be215b Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Wed, 17 Sep 2025 10:36:49 -0300 Subject: [PATCH 03/12] Undo grace period --- .rubocop_todo/database/avoid_using_connection_execute.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.rubocop_todo/database/avoid_using_connection_execute.yml b/.rubocop_todo/database/avoid_using_connection_execute.yml index a644f9ed223c4e..2f7d32d132e813 100644 --- a/.rubocop_todo/database/avoid_using_connection_execute.yml +++ b/.rubocop_todo/database/avoid_using_connection_execute.yml @@ -1,6 +1,5 @@ --- Database/AvoidUsingConnectionExecute: - Details: grace period Exclude: - 'app/models/analytics/cycle_analytics/stage_event_hash.rb' - 'app/models/concerns/analytics/cycle_analytics/stage_event_model.rb' -- GitLab From 9b65e3374041b4121483a0d54239d4756e75eaac Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Wed, 17 Sep 2025 10:41:29 -0300 Subject: [PATCH 04/12] Remove execute_prepared_async_dml_operations! method --- lib/gitlab/database/migrations/runner.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/gitlab/database/migrations/runner.rb b/lib/gitlab/database/migrations/runner.rb index 6bd41303d3b2de..68c01c2742ead3 100644 --- a/lib/gitlab/database/migrations/runner.rb +++ b/lib/gitlab/database/migrations/runner.rb @@ -76,12 +76,6 @@ def batched_migrations_last_id(for_database) runner end - def execute_prepared_async_dml_operations!(database) - Gitlab::Database::EachDatabase.each_connection(only: database) do |_connection| - Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.execute - end - end - private def migrations_for_up(database) -- GitLab From ee8c6b08662c922f6476df600714b7338b01021f Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Tue, 23 Sep 2025 16:11:56 -0300 Subject: [PATCH 05/12] Wraps index execution around a transaction --- .../async_operations_helper.rb | 26 ------------------- .../index_creator.rb | 14 +++++++++- 2 files changed, 13 insertions(+), 27 deletions(-) diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb index 9a33a5fdcee746..f77bdedc9d9720 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb @@ -28,29 +28,10 @@ def store_index(async_index) end end - module ForeignKeyMixin - extend ActiveSupport::Concern - - def prepare_async_foreign_key_validation(...) - store_fk(super) - end - - private - - def store_fk(fk) - return unless fk - - Thread.current[:async_fks].push(fk) - end - end - class AsyncOperationsHelper def self.install! Thread.current[:async_indexes] = [] - Thread.current[:async_fks] = [] - Gitlab::Database::AsyncIndexes::MigrationHelpers.prepend(AsyncIndexMixin) - Gitlab::Database::AsyncConstraints::MigrationHelpers.prepend(ForeignKeyMixin) end end @@ -59,13 +40,6 @@ def self.execute! Thread.current[:async_indexes].each do |async_index| PreparedAsyncDmlOperationsTesting::IndexCreator.new(async_index).perform end - - Thread.current[:async_fks].each do |async_fk| - PreparedAsyncDmlOperationsTesting::ForeignKeyValidator.new(async_fk).perform - end - - Thread.current[:async_indexes] = [] - Thread.current[:async_fks] = [] end end end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb index 71ec7a6d71bb82..651617407e4caf 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb @@ -5,7 +5,7 @@ module Database module Migrations module PreparedAsyncDmlOperationsTesting # This is a wrapper for Gitlab::Database::AsyncIndexes::IndexCreator, used to control the STATEMENT_TIMEOUT - # of index creating + # and creating async indexes synchronously class IndexCreator < Gitlab::Database::AsyncIndexes::IndexCreator private @@ -20,6 +20,18 @@ def execute_action_with_error_handling async_index.destroy! end + def execute_action + connection.execute(async_index.definition.gsub('CONCURRENTLY', '')) + async_index.destroy! + end + + override :around_execution + def around_execution(&block) + connection.transaction do + set_statement_timeout(&block) + end + end + def set_statement_timeout connection.execute(format("SET statement_timeout TO '%ds'", 3.minutes)) yield -- GitLab From a90fcbc662872a229362f2b08380d745314b0aa8 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Tue, 23 Sep 2025 16:44:30 -0300 Subject: [PATCH 06/12] Remove async FK validator class --- .../avoid_using_connection_execute.yml | 1 - .../database/rescue_query_canceled.yml | 1 - .../database/rescue_statement_timeout.yml | 1 - .../foreign_key_validator.rb | 35 ------------------- 4 files changed, 38 deletions(-) delete mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb diff --git a/.rubocop_todo/database/avoid_using_connection_execute.yml b/.rubocop_todo/database/avoid_using_connection_execute.yml index 2f7d32d132e813..59cc319eadfdab 100644 --- a/.rubocop_todo/database/avoid_using_connection_execute.yml +++ b/.rubocop_todo/database/avoid_using_connection_execute.yml @@ -27,7 +27,6 @@ Database/AvoidUsingConnectionExecute: - 'lib/gitlab/database/lock_writes_manager.rb' - 'lib/gitlab/database/migrations/observers/query_statistics.rb' - 'lib/gitlab/database/migrations/observers/total_database_size_change.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb' - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' - 'lib/gitlab/database/partitioning/detached_partition_dropper.rb' - 'lib/gitlab/database/partitioning/partition_manager.rb' diff --git a/.rubocop_todo/database/rescue_query_canceled.yml b/.rubocop_todo/database/rescue_query_canceled.yml index c06d3fdf10201c..44f31cc3f8e6cd 100644 --- a/.rubocop_todo/database/rescue_query_canceled.yml +++ b/.rubocop_todo/database/rescue_query_canceled.yml @@ -3,6 +3,5 @@ Database/RescueQueryCanceled: Exclude: - 'app/services/issues/relative_position_rebalancing_service.rb' - 'lib/gitlab/database/batch_counter.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb' - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' - 'lib/gitlab/issuables_count_for_state.rb' diff --git a/.rubocop_todo/database/rescue_statement_timeout.yml b/.rubocop_todo/database/rescue_statement_timeout.yml index fc1a55d046613c..33525084ae29ec 100644 --- a/.rubocop_todo/database/rescue_statement_timeout.yml +++ b/.rubocop_todo/database/rescue_statement_timeout.yml @@ -2,5 +2,4 @@ Database/RescueStatementTimeout: Exclude: - 'app/services/issues/relative_position_rebalancing_service.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb' - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb deleted file mode 100644 index 945bf4603b5305..00000000000000 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/foreign_key_validator.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Database - module Migrations - module PreparedAsyncDmlOperationsTesting - # This is a wrapper for Gitlab::Database::AsyncConstraints::Validators::ForeignKey, used to control the - # STATEMENT_TIMEOUT of index creating - class ForeignKeyValidator < Gitlab::Database::AsyncConstraints::Validators::ForeignKey - private - - override :validate_constraint_with_error_handling - def validate_constraint_with_error_handling - validate_constraint - rescue ActiveRecord::StatementTimeout, ActiveRecord::QueryCanceled, ActiveRecord::AdapterTimeout, - ActiveRecord::LockWaitTimeout => error - - Gitlab::AppJsonLogger.error(error: error, foreign_key: record.name) - ensure - # We ensure the record is destroyed in all scenarios - record.destroy! - end - - override :set_statement_timeout - def set_statement_timeout - connection.execute(format("SET statement_timeout TO '%ds'", 3.minutes)) - yield - ensure - connection.execute('RESET statement_timeout') - end - end - end - end - end -end -- GitLab From e94677ad8f10f9c176bb346cfdee70f26953c705 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Tue, 23 Sep 2025 22:29:39 -0300 Subject: [PATCH 07/12] Add specs --- .../async_operations_helper.rb | 48 ----------- .../async_operations_runner.rb | 67 +++++++++++++++ .../index_creator.rb | 3 +- lib/gitlab/database/migrations/runner.rb | 2 +- .../async_operations_runner_spec.rb | 85 +++++++++++++++++++ .../index_creator_spec.rb | 76 +++++++++++++++++ .../gitlab/database/migrations/runner_spec.rb | 30 +++++-- 7 files changed, 255 insertions(+), 56 deletions(-) delete mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb create mode 100644 lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb create mode 100644 spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb create mode 100644 spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb deleted file mode 100644 index f77bdedc9d9720..00000000000000 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_helper.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Database - module Migrations - module PreparedAsyncDmlOperationsTesting - module AsyncIndexMixin - extend ActiveSupport::Concern - - def prepare_async_index(...) - store_index(super) - end - - def prepare_async_index_from_sql(...) - store_index(super) - end - - def prepare_async_index_removal(...) - store_index(super) - end - - private - - def store_index(async_index) - return unless async_index - - Thread.current[:async_indexes].push(async_index) - end - end - - class AsyncOperationsHelper - def self.install! - Thread.current[:async_indexes] = [] - Gitlab::Database::AsyncIndexes::MigrationHelpers.prepend(AsyncIndexMixin) - end - end - - class AsyncOperationsRunner - def self.execute! - Thread.current[:async_indexes].each do |async_index| - PreparedAsyncDmlOperationsTesting::IndexCreator.new(async_index).perform - end - end - end - end - end - end -end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb new file mode 100644 index 00000000000000..2f6b60e65f2436 --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + THREAD_KEY = :async_indexes + + module AsyncIndexMixin + extend ActiveSupport::Concern + + def prepare_async_index(...) + store_index(super) + end + + def prepare_async_index_from_sql(...) + store_index(super) + end + + def prepare_async_index_removal(...) + store_index(super) + end + + private + + def store_index(async_index) + return unless async_index + + return unless Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY].is_a?(Array) + + Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY] << async_index + end + end + + class AsyncOperationsRunner + class << self + def install! + reset_thread + + Gitlab::Database::AsyncIndexes::MigrationHelpers.prepend(AsyncIndexMixin) + end + + def execute! + return unless Thread.current[thread_key] + + Thread.current[thread_key].to_a.each do |async_index| + PreparedAsyncDmlOperationsTesting::IndexCreator.new(async_index).perform + end + + reset_thread + end + + private + + def reset_thread + Thread.current[thread_key] = Set.new + end + + def thread_key + @thread_key ||= PreparedAsyncDmlOperationsTesting::THREAD_KEY + end + end + end + end + end + end +end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb index 651617407e4caf..a303474ae79583 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb @@ -21,8 +21,7 @@ def execute_action_with_error_handling end def execute_action - connection.execute(async_index.definition.gsub('CONCURRENTLY', '')) - async_index.destroy! + connection.execute(async_index.definition.gsub('CONCURRENTLY ', '')) end override :around_execution diff --git a/lib/gitlab/database/migrations/runner.rb b/lib/gitlab/database/migrations/runner.rb index 68c01c2742ead3..2a4ad4420fc6b5 100644 --- a/lib/gitlab/database/migrations/runner.rb +++ b/lib/gitlab/database/migrations/runner.rb @@ -133,7 +133,7 @@ def run instrumentation = Instrumentation.new(result_dir: result_dir) - Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsHelper.install! + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.install! within_context_for_database(@database) do sorted_migrations.each do |migration| diff --git a/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb new file mode 100644 index 00000000000000..945d4028c1266c --- /dev/null +++ b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner, feature_category: :database do + describe '.install!' do + it 'initializes thread-local async_indexes set' do + described_class.install! + + expect(Thread.current[:async_indexes]).to eq(Set.new) + end + + it 'prepends AsyncIndexMixin to MigrationHelpers' do + expect(Gitlab::Database::AsyncIndexes::MigrationHelpers).to( + receive(:prepend).with(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncIndexMixin) + ) + + described_class.install! + end + end + + describe '.execute!' do + let(:thread_key) { Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::THREAD_KEY } + + context 'when async indexes thread has indexes' do + let(:async_index_1) { create(:postgres_async_index, name: 'test_index_1') } + let(:async_index_2) { create(:postgres_async_index, name: 'test_index_2') } + + let(:index_creator_1) do + instance_double(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator) + end + + let(:index_creator2) do + instance_double(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator) + end + + before do + Thread.current[thread_key] = [async_index_1, async_index_2] + end + + after do + Thread.current[thread_key] = nil + end + + it 'creates IndexCreator instances for each async_index and calls perform' do + expect(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator).to( + receive(:new).with(async_index_1).and_return(index_creator_1) + ) + + expect(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator).to( + receive(:new).with(async_index_2).and_return(index_creator2) + ) + + expect(index_creator_1).to receive(:perform) + expect(index_creator2).to receive(:perform) + + described_class.execute! + end + end + + context 'when async indexes thread is empty' do + before do + Thread.current[thread_key] = [] + end + + it 'does not calls IndexCreator' do + expect(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator).not_to receive(:new) + + described_class.execute! + end + end + + context 'when async indexes thread is not initialized' do + before do + Thread.current[thread_key] = nil + end + + it 'does not calls IndexCreator' do + expect(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator).not_to receive(:new) + + described_class.execute! + end + end + end +end diff --git a/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb new file mode 100644 index 00000000000000..1e97fa621f5728 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator, feature_category: :database do + include ExclusiveLeaseHelpers + + describe '#perform' do + let(:async_index) { create(:postgres_async_index) } + let(:connection_name) { Gitlab::Database::PRIMARY_DATABASE_NAME } + let(:model) { Gitlab::Database.database_base_models[connection_name] } + let(:connection) { model.connection } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{connection_name}" } + let(:lease_timeout) { 3.minutes } + + let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } + + subject(:index_creator) { described_class.new(async_index) } + + around do |example| + Gitlab::Database::SharedModel.using_connection(connection) do + example.run + end + end + + shared_examples 'handling an error' do + it 'logs the error and destroys the record' do + allow(connection).to receive(:execute) + allow(connection).to receive(:execute).with("SET statement_timeout TO '180s'") + allow(connection).to receive(:execute).with(async_index.definition).and_raise(error) + allow(connection).to receive(:execute).with('RESET statement_timeout') + + expect(Gitlab::AppLogger).to receive(:error).with(error: error, index: async_index.name) + + expect(async_index).to receive(:destroy!).and_call_original + + index_creator.perform + end + end + + context 'when index creation succeeds' do + it 'executes the index definition within a transaction' do + allow(connection).to receive(:execute) + expect(connection).to receive(:execute).with("SET statement_timeout TO '180s'").ordered.and_call_original + expect(connection).to receive(:execute).with(async_index.definition).ordered.and_call_original + expect(connection).to receive(:execute).with('RESET statement_timeout').ordered.and_call_original + + expect { index_creator.perform }.to change { Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.count }.by(-1) + end + end + + context 'when statement timeout occurs' do + let(:error) { ActiveRecord::StatementTimeout.new('statement timeout') } + + it_behaves_like 'handling an error' + end + + context 'when query is canceled' do + let(:error) { ActiveRecord::QueryCanceled.new('query canceled') } + + it_behaves_like 'handling an error' + end + + context 'when adapter timeout occurs' do + let(:error) { ActiveRecord::AdapterTimeout.new('adapter timeout') } + + it_behaves_like 'handling an error' + end + + context 'when lock wait timeout occurs' do + let(:error) { ActiveRecord::LockWaitTimeout.new('lock wait timeout') } + + it_behaves_like 'handling an error' + end + end +end diff --git a/spec/lib/gitlab/database/migrations/runner_spec.rb b/spec/lib/gitlab/database/migrations/runner_spec.rb index dd2c9f620bd474..3cc39bfd282688 100644 --- a/spec/lib/gitlab/database/migrations/runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/runner_spec.rb @@ -64,6 +64,10 @@ ].sort_by(&:version) end + let(:async_operations_runner) do + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner + end + before do skip_if_shared_database(database) @@ -98,6 +102,20 @@ ] end + shared_examples 'using async operations runner' do + it 'installs async operations mixin before running migrations' do + expect(async_operations_runner).to receive(:install!).ordered + + subject.run + end + + it 'executes async operations after each migration' do + expect(async_operations_runner).to receive(:execute!).exactly(pending_migrations.count).times + + subject.run + end + end + with_them do it 'creates the results dir when one does not exist' do FileUtils.rm_rf(result_dir) @@ -108,23 +126,21 @@ end describe '.up' do + subject(:up) { described_class.up(database: database) } + context 'result directory' do it 'uses the /up subdirectory' do - expect(described_class.up(database: database).result_dir).to eq(result_dir.join('up')) + expect(up.result_dir).to eq(result_dir.join('up')) end end context 'migrations to run' do - subject(:up) { described_class.up(database: database) } - it 'is the list of pending migrations' do expect(up.migrations).to eq(pending_migrations) end end context 'running migrations' do - subject(:up) { described_class.up(database: database) } - it 'runs the unapplied migrations in regular/post order, then version order', :aggregate_failures do up.run @@ -147,6 +163,8 @@ expect(migration_runs.map(&:database).uniq).to contain_exactly(database.to_s) end end + + it_behaves_like 'using async operations runner' end describe '.down' do @@ -181,6 +199,8 @@ metadata = Gitlab::Json.parse(File.read(metadata_file)) expect(metadata).to match('version' => expected_schema_version, 'database' => database.to_s) end + + it_behaves_like 'using async operations runner' end describe '.batched_background_migrations' do -- GitLab From e38f5c5e9c8ee6ce524e6aa81e1f58e7d55f4906 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Tue, 23 Sep 2025 23:00:57 -0300 Subject: [PATCH 08/12] It compares if thread structure is a Set --- .../async_operations_runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb index 2f6b60e65f2436..499976cd7adc80 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb @@ -26,7 +26,7 @@ def prepare_async_index_removal(...) def store_index(async_index) return unless async_index - return unless Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY].is_a?(Array) + return unless Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY].is_a?(Set) Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY] << async_index end -- GitLab From 4c9cf69bf8d4d580ff32bb0da87ba5e011b6bfc3 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Wed, 24 Sep 2025 10:57:00 -0300 Subject: [PATCH 09/12] Remove prepare_async_index_removal method --- .../async_operations_runner.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb index 499976cd7adc80..78f2b422832f6e 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb @@ -17,10 +17,6 @@ def prepare_async_index_from_sql(...) store_index(super) end - def prepare_async_index_removal(...) - store_index(super) - end - private def store_index(async_index) -- GitLab From b129a8708b428a21216988ed7f363cfcd17903a4 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Mon, 29 Sep 2025 10:57:16 -0300 Subject: [PATCH 10/12] Remove the thread logic --- .../database/rescue_query_canceled.yml | 1 - .../database/rescue_statement_timeout.yml | 1 - .../async_operations_runner.rb | 30 +++++----------- .../index_creator.rb | 32 ++++++----------- .../async_operations_runner_spec.rb | 36 ++++--------------- .../index_creator_spec.rb | 21 +++++++++-- 6 files changed, 44 insertions(+), 77 deletions(-) diff --git a/.rubocop_todo/database/rescue_query_canceled.yml b/.rubocop_todo/database/rescue_query_canceled.yml index 44f31cc3f8e6cd..c04721ec9d293e 100644 --- a/.rubocop_todo/database/rescue_query_canceled.yml +++ b/.rubocop_todo/database/rescue_query_canceled.yml @@ -3,5 +3,4 @@ Database/RescueQueryCanceled: Exclude: - 'app/services/issues/relative_position_rebalancing_service.rb' - 'lib/gitlab/database/batch_counter.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' - 'lib/gitlab/issuables_count_for_state.rb' diff --git a/.rubocop_todo/database/rescue_statement_timeout.yml b/.rubocop_todo/database/rescue_statement_timeout.yml index 33525084ae29ec..ce8e834e93ce4f 100644 --- a/.rubocop_todo/database/rescue_statement_timeout.yml +++ b/.rubocop_todo/database/rescue_statement_timeout.yml @@ -2,4 +2,3 @@ Database/RescueStatementTimeout: Exclude: - 'app/services/issues/relative_position_rebalancing_service.rb' - - 'lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb' diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb index 78f2b422832f6e..0ec5f09e4aa2c4 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb @@ -4,56 +4,44 @@ module Gitlab module Database module Migrations module PreparedAsyncDmlOperationsTesting - THREAD_KEY = :async_indexes - module AsyncIndexMixin extend ActiveSupport::Concern def prepare_async_index(...) - store_index(super) + mark_for_testing_execution(super) end def prepare_async_index_from_sql(...) - store_index(super) + mark_for_testing_execution(super) end private - def store_index(async_index) + def mark_for_testing_execution(async_index) return unless async_index - return unless Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY].is_a?(Set) - - Thread.current[PreparedAsyncDmlOperationsTesting::THREAD_KEY] << async_index + async_index.update!(definition: "#{async_index.definition} /* SYNC_TESTING_EXECUTION */") end end class AsyncOperationsRunner class << self def install! - reset_thread - Gitlab::Database::AsyncIndexes::MigrationHelpers.prepend(AsyncIndexMixin) end def execute! - return unless Thread.current[thread_key] - - Thread.current[thread_key].to_a.each do |async_index| + indexes_to_create.each do |async_index| PreparedAsyncDmlOperationsTesting::IndexCreator.new(async_index).perform end - - reset_thread end private - def reset_thread - Thread.current[thread_key] = Set.new - end - - def thread_key - @thread_key ||= PreparedAsyncDmlOperationsTesting::THREAD_KEY + def indexes_to_create + Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_create.where( + "definition LIKE '%/* SYNC_TESTING_EXECUTION */%'" + ) end end end diff --git a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb index a303474ae79583..bca6717234ec29 100644 --- a/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb @@ -7,34 +7,24 @@ module PreparedAsyncDmlOperationsTesting # This is a wrapper for Gitlab::Database::AsyncIndexes::IndexCreator, used to control the STATEMENT_TIMEOUT # and creating async indexes synchronously class IndexCreator < Gitlab::Database::AsyncIndexes::IndexCreator - private - - def execute_action_with_error_handling - around_execution { execute_action } - rescue ActiveRecord::StatementTimeout, ActiveRecord::QueryCanceled, ActiveRecord::AdapterTimeout, - ActiveRecord::LockWaitTimeout => error + TIMEOUT_EXCEPTIONS = [ActiveRecord::StatementTimeout, ActiveRecord::AdapterTimeout, + ActiveRecord::LockWaitTimeout, ActiveRecord::QueryCanceled].freeze - Gitlab::AppLogger.error(error: error, index: async_index.name) + def perform + connection.transaction do + execute_action + end + rescue *TIMEOUT_EXCEPTIONS => error + Gitlab::AppLogger.info(message: error, index: async_index.name) ensure - # We ensure the record is destroyed in all scenarios async_index.destroy! end - def execute_action - connection.execute(async_index.definition.gsub('CONCURRENTLY ', '')) - end - - override :around_execution - def around_execution(&block) - connection.transaction do - set_statement_timeout(&block) - end - end + private - def set_statement_timeout + def execute_action connection.execute(format("SET statement_timeout TO '%ds'", 3.minutes)) - yield - ensure + connection.execute(async_index.definition.gsub('CONCURRENTLY ', '')) connection.execute('RESET statement_timeout') end end diff --git a/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb index 945d4028c1266c..7f9b467553d982 100644 --- a/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb @@ -4,12 +4,6 @@ RSpec.describe Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner, feature_category: :database do describe '.install!' do - it 'initializes thread-local async_indexes set' do - described_class.install! - - expect(Thread.current[:async_indexes]).to eq(Set.new) - end - it 'prepends AsyncIndexMixin to MigrationHelpers' do expect(Gitlab::Database::AsyncIndexes::MigrationHelpers).to( receive(:prepend).with(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncIndexMixin) @@ -20,9 +14,7 @@ end describe '.execute!' do - let(:thread_key) { Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::THREAD_KEY } - - context 'when async indexes thread has indexes' do + context 'when async indexes are tagged to be executed' do let(:async_index_1) { create(:postgres_async_index, name: 'test_index_1') } let(:async_index_2) { create(:postgres_async_index, name: 'test_index_2') } @@ -35,11 +27,8 @@ end before do - Thread.current[thread_key] = [async_index_1, async_index_2] - end - - after do - Thread.current[thread_key] = nil + async_index_1.update!(definition: "#{async_index_1.definition} /* SYNC_TESTING_EXECUTION */") + async_index_2.update!(definition: "#{async_index_1.definition} /* SYNC_TESTING_EXECUTION */") end it 'creates IndexCreator instances for each async_index and calls perform' do @@ -58,22 +47,9 @@ end end - context 'when async indexes thread is empty' do - before do - Thread.current[thread_key] = [] - end - - it 'does not calls IndexCreator' do - expect(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator).not_to receive(:new) - - described_class.execute! - end - end - - context 'when async indexes thread is not initialized' do - before do - Thread.current[thread_key] = nil - end + context 'when async indexes are not tagged to be executed' do + let!(:async_index_1) { create(:postgres_async_index, name: 'test_index_1') } + let!(:async_index_2) { create(:postgres_async_index, name: 'test_index_2') } it 'does not calls IndexCreator' do expect(Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::IndexCreator).not_to receive(:new) diff --git a/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb index 1e97fa621f5728..e74e8caf52ab06 100644 --- a/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb +++ b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb @@ -25,16 +25,16 @@ shared_examples 'handling an error' do it 'logs the error and destroys the record' do - allow(connection).to receive(:execute) + allow(connection).to receive(:transaction).and_yield allow(connection).to receive(:execute).with("SET statement_timeout TO '180s'") allow(connection).to receive(:execute).with(async_index.definition).and_raise(error) allow(connection).to receive(:execute).with('RESET statement_timeout') - expect(Gitlab::AppLogger).to receive(:error).with(error: error, index: async_index.name) + expect(Gitlab::AppLogger).to receive(:info).with(message: error, index: async_index.name) expect(async_index).to receive(:destroy!).and_call_original - index_creator.perform + expect { index_creator.perform }.not_to raise_error end end @@ -72,5 +72,20 @@ it_behaves_like 'handling an error' end + + context 'when a invalid statement error occurs' do + let(:async_index) { create(:postgres_async_index, definition: 'CREATE INDEX idx_t ON missing_table (id)') } + + it 'logs the error and destroys the record' do + allow(connection).to receive(:transaction).and_yield + allow(connection).to receive(:execute).with("SET statement_timeout TO '180s'") + allow(connection).to receive(:execute).with(async_index.definition).and_call_original + allow(connection).to receive(:execute).with('RESET statement_timeout') + + expect(async_index).to receive(:destroy!).and_call_original + + expect { index_creator.perform }.to raise_error(ActiveRecord::StatementInvalid) + end + end end end -- GitLab From 2fb97111318850c1fb3b328d35a7131821ae1e42 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Mon, 29 Sep 2025 14:36:11 -0300 Subject: [PATCH 11/12] Adds a migration to test CI --- ...250924141629_test_async_index_execution.rb | 19 +++++++++++++++++++ db/schema_migrations/20250924141629 | 1 + 2 files changed, 20 insertions(+) create mode 100644 db/post_migrate/20250924141629_test_async_index_execution.rb create mode 100644 db/schema_migrations/20250924141629 diff --git a/db/post_migrate/20250924141629_test_async_index_execution.rb b/db/post_migrate/20250924141629_test_async_index_execution.rb new file mode 100644 index 00000000000000..004d8af5176b07 --- /dev/null +++ b/db/post_migrate/20250924141629_test_async_index_execution.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class TestAsyncIndexExecution < Gitlab::Database::Migration[2.3] + milestone '18.5' + + def up + # rubocop:disable Migration/PreventIndexCreation -- only for testing + prepare_async_index(:issues, %i[project_id milestone_id], name: :idx_test_async_index_on_issues) + # rubocop:enable Migration/PreventIndexCreation + prepare_async_index_from_sql( + 'CREATE INDEX CONCURRENTLY "idx_test_async_index_on_projects" ON "projects" ("creator_id", "visibility_level")' + ) + end + + def down + unprepare_async_index(:issues, %i[project_id milestone_id], name: :idx_test_async_index_on_issues) + unprepare_async_index_by_name(:projects, 'idx_test_async_index_on_projects') + end +end diff --git a/db/schema_migrations/20250924141629 b/db/schema_migrations/20250924141629 new file mode 100644 index 00000000000000..dd995d03ae16c5 --- /dev/null +++ b/db/schema_migrations/20250924141629 @@ -0,0 +1 @@ +283e9a5856041aff171d64ba2abaf509a71e4be96cc94bb4d7c77fcc1e9e254b \ No newline at end of file -- GitLab From cd3059ea4d5f91b05570bbebaa654e8d62c39822 Mon Sep 17 00:00:00 2001 From: Leonardo Rosa Date: Wed, 1 Oct 2025 17:58:01 -0300 Subject: [PATCH 12/12] Adds a migration to fail CI --- .../20251001195353_test_async_index_to_fail.rb | 15 +++++++++++++++ db/schema_migrations/20251001195353 | 1 + 2 files changed, 16 insertions(+) create mode 100644 db/post_migrate/20251001195353_test_async_index_to_fail.rb create mode 100644 db/schema_migrations/20251001195353 diff --git a/db/post_migrate/20251001195353_test_async_index_to_fail.rb b/db/post_migrate/20251001195353_test_async_index_to_fail.rb new file mode 100644 index 00000000000000..2dc14bb568623c --- /dev/null +++ b/db/post_migrate/20251001195353_test_async_index_to_fail.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class TestAsyncIndexToFail < Gitlab::Database::Migration[2.3] + milestone '18.5' + + def up + # rubocop:disable Migration/PreventIndexCreation -- only for testing + prepare_async_index(:issues, %i[id inexistent_column], name: :idx_test_async_index_on_issues_two) + # rubocop:enable Migration/PreventIndexCreation + end + + def down + unprepare_async_index(:issues, %i[id inexistent_column], name: :idx_test_async_index_on_issues_two) + end +end diff --git a/db/schema_migrations/20251001195353 b/db/schema_migrations/20251001195353 new file mode 100644 index 00000000000000..722959a508988f --- /dev/null +++ b/db/schema_migrations/20251001195353 @@ -0,0 +1 @@ +c26feeaab986d409c9d9f7684601707ae6581c8a43caa228b12bbae8a87e4222 \ No newline at end of file -- GitLab