diff --git a/.rubocop_todo/database/avoid_using_connection_execute.yml b/.rubocop_todo/database/avoid_using_connection_execute.yml index 8e7ec4c68e963a3506dd9b70d66a88ba4de3d86f..59cc319eadfdabf68da7ed5182327dceb73156a7 100644 --- a/.rubocop_todo/database/avoid_using_connection_execute.yml +++ b/.rubocop_todo/database/avoid_using_connection_execute.yml @@ -27,6 +27,7 @@ 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/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/db/post_migrate/20250924141629_test_async_index_execution.rb b/db/post_migrate/20250924141629_test_async_index_execution.rb new file mode 100644 index 0000000000000000000000000000000000000000..004d8af5176b079f6ac2266d5fe0c005a0fe5fc3 --- /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/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 0000000000000000000000000000000000000000..2dc14bb568623c45bc7c41978f80afd914186e8e --- /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/20250924141629 b/db/schema_migrations/20250924141629 new file mode 100644 index 0000000000000000000000000000000000000000..dd995d03ae16c5acfcfd7d062aeee8ab8e537bd0 --- /dev/null +++ b/db/schema_migrations/20250924141629 @@ -0,0 +1 @@ +283e9a5856041aff171d64ba2abaf509a71e4be96cc94bb4d7c77fcc1e9e254b \ No newline at end of file diff --git a/db/schema_migrations/20251001195353 b/db/schema_migrations/20251001195353 new file mode 100644 index 0000000000000000000000000000000000000000..722959a508988fb2036fabf1fb8417d39275ec30 --- /dev/null +++ b/db/schema_migrations/20251001195353 @@ -0,0 +1 @@ +c26feeaab986d409c9d9f7684601707ae6581c8a43caa228b12bbae8a87e4222 \ No newline at end of file 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 0000000000000000000000000000000000000000..0ec5f09e4aa2c41949ba77a669591713e24fb45b --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Migrations + module PreparedAsyncDmlOperationsTesting + module AsyncIndexMixin + extend ActiveSupport::Concern + + def prepare_async_index(...) + mark_for_testing_execution(super) + end + + def prepare_async_index_from_sql(...) + mark_for_testing_execution(super) + end + + private + + def mark_for_testing_execution(async_index) + return unless async_index + + async_index.update!(definition: "#{async_index.definition} /* SYNC_TESTING_EXECUTION */") + end + end + + class AsyncOperationsRunner + class << self + def install! + Gitlab::Database::AsyncIndexes::MigrationHelpers.prepend(AsyncIndexMixin) + end + + def execute! + indexes_to_create.each do |async_index| + PreparedAsyncDmlOperationsTesting::IndexCreator.new(async_index).perform + end + end + + private + + def indexes_to_create + Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_create.where( + "definition LIKE '%/* SYNC_TESTING_EXECUTION */%'" + ) + 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 new file mode 100644 index 0000000000000000000000000000000000000000..bca6717234ec2964ae33422d63e3138b45bff55c --- /dev/null +++ b/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator.rb @@ -0,0 +1,34 @@ +# 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 + # and creating async indexes synchronously + class IndexCreator < Gitlab::Database::AsyncIndexes::IndexCreator + TIMEOUT_EXCEPTIONS = [ActiveRecord::StatementTimeout, ActiveRecord::AdapterTimeout, + ActiveRecord::LockWaitTimeout, ActiveRecord::QueryCanceled].freeze + + def perform + connection.transaction do + execute_action + end + rescue *TIMEOUT_EXCEPTIONS => error + Gitlab::AppLogger.info(message: error, index: async_index.name) + ensure + async_index.destroy! + end + + private + + def execute_action + connection.execute(format("SET statement_timeout TO '%ds'", 3.minutes)) + connection.execute(async_index.definition.gsub('CONCURRENTLY ', '')) + 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 2cc689b558b0a1983c5a0fb19e20c61fa4bd902f..2a4ad4420fc6b5d540ea7289051125642fb66f19 100644 --- a/lib/gitlab/database/migrations/runner.rb +++ b/lib/gitlab/database/migrations/runner.rb @@ -133,10 +133,13 @@ def run instrumentation = Instrumentation.new(result_dir: result_dir) + Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner.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 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 0000000000000000000000000000000000000000..7f9b467553d982411ff569aacd70c63e64e80cd1 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/async_operations_runner_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::PreparedAsyncDmlOperationsTesting::AsyncOperationsRunner, feature_category: :database do + describe '.install!' do + 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 + 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') } + + 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 + 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 + 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 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) + + 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 0000000000000000000000000000000000000000..e74e8caf52ab06b78b07147067157f91f163ae5a --- /dev/null +++ b/spec/lib/gitlab/database/migrations/prepared_async_dml_operations_testing/index_creator_spec.rb @@ -0,0 +1,91 @@ +# 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(: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(:info).with(message: error, index: async_index.name) + + expect(async_index).to receive(:destroy!).and_call_original + + expect { index_creator.perform }.not_to raise_error + 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 + + 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 diff --git a/spec/lib/gitlab/database/migrations/runner_spec.rb b/spec/lib/gitlab/database/migrations/runner_spec.rb index dd2c9f620bd474de95ff6a3ef2175d78db67a2b9..3cc39bfd2826883f1b85a6c787957b0eb5a9ec39 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