diff options
Diffstat (limited to 'spec/lib/gitlab/database')
46 files changed, 1640 insertions, 484 deletions
diff --git a/spec/lib/gitlab/database/indexing_exclusive_lease_guard_spec.rb b/spec/lib/gitlab/database/async_ddl_exclusive_lease_guard_spec.rb index ddc9cdee92f..60ccf5ec685 100644 --- a/spec/lib/gitlab/database/indexing_exclusive_lease_guard_spec.rb +++ b/spec/lib/gitlab/database/async_ddl_exclusive_lease_guard_spec.rb @@ -2,25 +2,25 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::IndexingExclusiveLeaseGuard, feature_category: :database do +RSpec.describe Gitlab::Database::AsyncDdlExclusiveLeaseGuard, feature_category: :database do let(:helper_class) do Class.new do - include Gitlab::Database::IndexingExclusiveLeaseGuard + include Gitlab::Database::AsyncDdlExclusiveLeaseGuard - attr_reader :connection + attr_reader :connection_db_config - def initialize(connection) - @connection = connection + def initialize(connection_db_config) + @connection_db_config = connection_db_config end end end describe '#lease_key' do - let(:helper) { helper_class.new(connection) } - let(:lease_key) { "gitlab/database/indexing/actions/#{database_name}" } + let(:helper) { helper_class.new(connection_db_config) } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{database_name}" } context 'with CI database connection' do - let(:connection) { Ci::ApplicationRecord.connection } + let(:connection_db_config) { Ci::ApplicationRecord.connection_db_config } let(:database_name) { Gitlab::Database::CI_DATABASE_NAME } before do @@ -31,7 +31,7 @@ RSpec.describe Gitlab::Database::IndexingExclusiveLeaseGuard, feature_category: end context 'with MAIN database connection' do - let(:connection) { ApplicationRecord.connection } + let(:connection_db_config) { ApplicationRecord.connection_db_config } let(:database_name) { Gitlab::Database::MAIN_DATABASE_NAME } it { expect(helper.lease_key).to eq(lease_key) } diff --git a/spec/lib/gitlab/database/async_foreign_keys/foreign_key_validator_spec.rb b/spec/lib/gitlab/database/async_foreign_keys/foreign_key_validator_spec.rb new file mode 100644 index 00000000000..90137e259f5 --- /dev/null +++ b/spec/lib/gitlab/database/async_foreign_keys/foreign_key_validator_spec.rb @@ -0,0 +1,152 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::AsyncForeignKeys::ForeignKeyValidator, feature_category: :database do + include ExclusiveLeaseHelpers + + describe '#perform' do + let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } + + let(:fk_model) { Gitlab::Database::AsyncForeignKeys::PostgresAsyncForeignKeyValidation } + let(:table_name) { '_test_async_fks' } + let(:fk_name) { 'fk_parent_id' } + let(:validation) { create(:postgres_async_foreign_key_validation, table_name: table_name, name: fk_name) } + let(:connection) { validation.connection } + + subject { described_class.new(validation) } + + before do + connection.create_table(table_name) do |t| + t.references :parent, foreign_key: { to_table: table_name, validate: false, name: fk_name } + end + end + + it 'validates the FK while controlling statement timeout' do + allow(connection).to receive(:execute).and_call_original + expect(connection).to receive(:execute) + .with("SET statement_timeout TO '43200s'").ordered.and_call_original + expect(connection).to receive(:execute) + .with('ALTER TABLE "_test_async_fks" VALIDATE CONSTRAINT "fk_parent_id";').ordered.and_call_original + expect(connection).to receive(:execute) + .with("RESET statement_timeout").ordered.and_call_original + + subject.perform + end + + context 'with fully qualified table names' do + let(:validation) do + create(:postgres_async_foreign_key_validation, + table_name: "public.#{table_name}", + name: fk_name + ) + end + + it 'validates the FK' do + allow(connection).to receive(:execute).and_call_original + + expect(connection).to receive(:execute) + .with('ALTER TABLE "public"."_test_async_fks" VALIDATE CONSTRAINT "fk_parent_id";').ordered.and_call_original + + subject.perform + end + end + + it 'removes the FK validation record from table' do + expect(validation).to receive(:destroy!).and_call_original + + expect { subject.perform }.to change { fk_model.count }.by(-1) + end + + it 'skips logic if not able to acquire exclusive lease' do + expect(lease).to receive(:try_obtain).ordered.and_return(false) + expect(connection).not_to receive(:execute).with(/ALTER TABLE/) + expect(validation).not_to receive(:destroy!) + + expect { subject.perform }.not_to change { fk_model.count } + end + + it 'logs messages around execution' do + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + subject.perform + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: 'Starting to validate foreign key')) + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: 'Finished validating foreign key')) + end + + context 'when the FK does not exist' do + before do + connection.create_table(table_name, force: true) + end + + it 'skips validation and removes the record' do + expect(connection).not_to receive(:execute).with(/ALTER TABLE/) + + expect { subject.perform }.to change { fk_model.count }.by(-1) + end + + it 'logs an appropriate message' do + expected_message = "Skipping #{fk_name} validation since it does not exist. The queuing entry will be deleted" + + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + subject.perform + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: expected_message)) + end + end + + context 'with error handling' do + before do + allow(connection).to receive(:execute).and_call_original + + allow(connection).to receive(:execute) + .with('ALTER TABLE "_test_async_fks" VALIDATE CONSTRAINT "fk_parent_id";') + .and_raise(ActiveRecord::StatementInvalid) + end + + context 'on production' do + before do + allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false) + end + + it 'increases execution attempts' do + expect { subject.perform }.to change { validation.attempts }.by(1) + + expect(validation.last_error).to be_present + expect(validation).not_to be_destroyed + end + + it 'logs an error message including the fk_name' do + expect(Gitlab::AppLogger) + .to receive(:error) + .with(a_hash_including(:message, :fk_name)) + .and_call_original + + subject.perform + end + end + + context 'on development' do + it 'also raises errors' do + expect { subject.perform } + .to raise_error(ActiveRecord::StatementInvalid) + .and change { validation.attempts }.by(1) + + expect(validation.last_error).to be_present + expect(validation).not_to be_destroyed + end + end + end + end +end diff --git a/spec/lib/gitlab/database/async_foreign_keys/migration_helpers_spec.rb b/spec/lib/gitlab/database/async_foreign_keys/migration_helpers_spec.rb new file mode 100644 index 00000000000..0bd0e8045ff --- /dev/null +++ b/spec/lib/gitlab/database/async_foreign_keys/migration_helpers_spec.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::AsyncForeignKeys::MigrationHelpers, feature_category: :database do + let(:migration) { Gitlab::Database::Migration[2.1].new } + let(:connection) { ApplicationRecord.connection } + let(:fk_model) { Gitlab::Database::AsyncForeignKeys::PostgresAsyncForeignKeyValidation } + let(:table_name) { '_test_async_fks' } + let(:column_name) { 'parent_id' } + let(:fk_name) { nil } + + context 'with regular tables' do + before do + allow(migration).to receive(:puts) + allow(migration.connection).to receive(:transaction_open?).and_return(false) + + connection.create_table(table_name) do |t| + t.integer column_name + end + + migration.add_concurrent_foreign_key( + table_name, table_name, + column: column_name, validate: false, name: fk_name) + end + + describe '#prepare_async_foreign_key_validation' do + it 'creates the record for the async FK validation' do + expect do + migration.prepare_async_foreign_key_validation(table_name, column_name) + end.to change { fk_model.where(table_name: table_name).count }.by(1) + + record = fk_model.find_by(table_name: table_name) + + expect(record.name).to start_with('fk_') + end + + context 'when an explicit name is given' do + let(:fk_name) { 'my_fk_name' } + + it 'creates the record with the given name' do + expect do + migration.prepare_async_foreign_key_validation(table_name, name: fk_name) + end.to change { fk_model.where(name: fk_name).count }.by(1) + + record = fk_model.find_by(name: fk_name) + + expect(record.table_name).to eq(table_name) + end + end + + context 'when the FK does not exist' do + it 'returns an error' do + expect do + migration.prepare_async_foreign_key_validation(table_name, name: 'no_fk') + end.to raise_error RuntimeError, /Could not find foreign key "no_fk" on table "_test_async_fks"/ + end + end + + context 'when the record already exists' do + let(:fk_name) { 'my_fk_name' } + + it 'does attempt to create the record' do + create(:postgres_async_foreign_key_validation, table_name: table_name, name: fk_name) + + expect do + migration.prepare_async_foreign_key_validation(table_name, name: fk_name) + end.not_to change { fk_model.where(name: fk_name).count } + end + end + + context 'when the async FK validation table does not exist' do + it 'does not raise an error' do + connection.drop_table(:postgres_async_foreign_key_validations) + + expect(fk_model).not_to receive(:safe_find_or_create_by!) + + expect { migration.prepare_async_foreign_key_validation(table_name, column_name) }.not_to raise_error + end + end + end + + describe '#unprepare_async_foreign_key_validation' do + before do + migration.prepare_async_foreign_key_validation(table_name, column_name, name: fk_name) + end + + it 'destroys the record' do + expect do + migration.unprepare_async_foreign_key_validation(table_name, column_name) + end.to change { fk_model.where(table_name: table_name).count }.by(-1) + end + + context 'when an explicit name is given' do + let(:fk_name) { 'my_test_async_fk' } + + it 'destroys the record' do + expect do + migration.unprepare_async_foreign_key_validation(table_name, name: fk_name) + end.to change { fk_model.where(name: fk_name).count }.by(-1) + end + end + + context 'when the async fk validation table does not exist' do + it 'does not raise an error' do + connection.drop_table(:postgres_async_foreign_key_validations) + + expect(fk_model).not_to receive(:find_by) + + expect { migration.unprepare_async_foreign_key_validation(table_name, column_name) }.not_to raise_error + end + end + end + end + + context 'with partitioned tables' do + let(:partition_schema) { 'gitlab_partitions_dynamic' } + let(:partition1_name) { "#{partition_schema}.#{table_name}_202001" } + let(:partition2_name) { "#{partition_schema}.#{table_name}_202002" } + let(:fk_name) { 'my_partitioned_fk_name' } + + before do + connection.execute(<<~SQL) + CREATE TABLE #{table_name} ( + id serial NOT NULL, + #{column_name} int NOT NULL, + created_at timestamptz NOT NULL, + PRIMARY KEY (id, created_at) + ) PARTITION BY RANGE (created_at); + + CREATE TABLE #{partition1_name} PARTITION OF #{table_name} + FOR VALUES FROM ('2020-01-01') TO ('2020-02-01'); + + CREATE TABLE #{partition2_name} PARTITION OF #{table_name} + FOR VALUES FROM ('2020-02-01') TO ('2020-03-01'); + SQL + end + + describe '#prepare_partitioned_async_foreign_key_validation' do + it 'delegates to prepare_async_foreign_key_validation for each partition' do + expect(migration) + .to receive(:prepare_async_foreign_key_validation) + .with(partition1_name, column_name, name: fk_name) + + expect(migration) + .to receive(:prepare_async_foreign_key_validation) + .with(partition2_name, column_name, name: fk_name) + + migration.prepare_partitioned_async_foreign_key_validation(table_name, column_name, name: fk_name) + end + end + + describe '#unprepare_partitioned_async_foreign_key_validation' do + it 'delegates to unprepare_async_foreign_key_validation for each partition' do + expect(migration) + .to receive(:unprepare_async_foreign_key_validation) + .with(partition1_name, column_name, name: fk_name) + + expect(migration) + .to receive(:unprepare_async_foreign_key_validation) + .with(partition2_name, column_name, name: fk_name) + + migration.unprepare_partitioned_async_foreign_key_validation(table_name, column_name, name: fk_name) + end + end + end +end diff --git a/spec/lib/gitlab/database/async_foreign_keys/postgres_async_foreign_key_validation_spec.rb b/spec/lib/gitlab/database/async_foreign_keys/postgres_async_foreign_key_validation_spec.rb new file mode 100644 index 00000000000..ba201d93f52 --- /dev/null +++ b/spec/lib/gitlab/database/async_foreign_keys/postgres_async_foreign_key_validation_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::AsyncForeignKeys::PostgresAsyncForeignKeyValidation, type: :model, + feature_category: :database do + it { is_expected.to be_a Gitlab::Database::SharedModel } + + describe 'validations' do + let_it_be(:fk_validation) { create(:postgres_async_foreign_key_validation) } + let(:identifier_limit) { described_class::MAX_IDENTIFIER_LENGTH } + let(:last_error_limit) { described_class::MAX_LAST_ERROR_LENGTH } + + subject { fk_validation } + + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_uniqueness_of(:name) } + it { is_expected.to validate_length_of(:name).is_at_most(identifier_limit) } + it { is_expected.to validate_presence_of(:table_name) } + it { is_expected.to validate_length_of(:table_name).is_at_most(identifier_limit) } + it { is_expected.to validate_length_of(:last_error).is_at_most(last_error_limit) } + end + + describe 'scopes' do + let!(:failed_validation) { create(:postgres_async_foreign_key_validation, attempts: 1) } + let!(:new_validation) { create(:postgres_async_foreign_key_validation) } + + describe '.ordered' do + subject { described_class.ordered } + + it { is_expected.to eq([new_validation, failed_validation]) } + end + end + + describe '#handle_exception!' do + let_it_be_with_reload(:fk_validation) { create(:postgres_async_foreign_key_validation) } + + let(:error) { instance_double(StandardError, message: 'Oups', backtrace: %w[this that]) } + + subject { fk_validation.handle_exception!(error) } + + it 'increases the attempts number' do + expect { subject }.to change { fk_validation.reload.attempts }.by(1) + end + + it 'saves error details' do + subject + + expect(fk_validation.reload.last_error).to eq("Oups\nthis\nthat") + end + end +end diff --git a/spec/lib/gitlab/database/async_foreign_keys_spec.rb b/spec/lib/gitlab/database/async_foreign_keys_spec.rb new file mode 100644 index 00000000000..f15eb364929 --- /dev/null +++ b/spec/lib/gitlab/database/async_foreign_keys_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::AsyncForeignKeys, feature_category: :database do + describe '.validate_pending_entries!' do + subject { described_class.validate_pending_entries! } + + before do + create_list(:postgres_async_foreign_key_validation, 3) + end + + it 'takes 2 pending FK validations and executes them' do + validations = described_class::PostgresAsyncForeignKeyValidation.ordered.limit(2).to_a + + expect_next_instances_of(described_class::ForeignKeyValidator, 2, validations) do |validator| + expect(validator).to receive(:perform) + end + + subject + end + end +end diff --git a/spec/lib/gitlab/database/async_indexes/index_base_spec.rb b/spec/lib/gitlab/database/async_indexes/index_base_spec.rb new file mode 100644 index 00000000000..d6070ff215e --- /dev/null +++ b/spec/lib/gitlab/database/async_indexes/index_base_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::AsyncIndexes::IndexBase, feature_category: :database do + describe '#perform' do + subject { described_class.new(async_index) } + + let(:async_index) { create(:postgres_async_index) } + let(:model) { Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME] } + let(:connection) { model.connection } + + around do |example| + Gitlab::Database::SharedModel.using_connection(connection) do + example.run + end + end + + describe '#preconditions_met?' do + it 'raises errors if preconditions is not defined' do + expect { subject.perform }.to raise_error NotImplementedError, 'must implement preconditions_met?' + end + end + + describe '#action_type' do + before do + allow(subject).to receive(:preconditions_met?).and_return(true) + end + + it 'raises errors if action_type is not defined' do + expect { subject.perform }.to raise_error NotImplementedError, 'must implement action_type' + end + end + + context 'with error handling' do + before do + allow(subject).to receive(:preconditions_met?).and_return(true) + allow(subject).to receive(:action_type).and_return('test') + allow(async_index.connection).to receive(:execute).and_call_original + + allow(async_index.connection) + .to receive(:execute) + .with(async_index.definition) + .and_raise(ActiveRecord::StatementInvalid) + end + + context 'on production' do + before do + allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false) + end + + it 'increases execution attempts' do + expect { subject.perform }.to change { async_index.attempts }.by(1) + + expect(async_index.last_error).to be_present + expect(async_index).not_to be_destroyed + end + + it 'logs an error message including the index_name' do + expect(Gitlab::AppLogger) + .to receive(:error) + .with(a_hash_including(:message, :index_name)) + .and_call_original + + subject.perform + end + end + + context 'on development' do + it 'also raises errors' do + expect { subject.perform } + .to raise_error(ActiveRecord::StatementInvalid) + .and change { async_index.attempts }.by(1) + + expect(async_index.last_error).to be_present + expect(async_index).not_to be_destroyed + end + end + end + end +end diff --git a/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb b/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb index 207aedd1a38..51a09ba0b5e 100644 --- a/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator do +RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator, feature_category: :database do include ExclusiveLeaseHelpers describe '#perform' do @@ -16,7 +16,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator do let(:connection) { model.connection } let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } - let(:lease_key) { "gitlab/database/indexing/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } around do |example| @@ -35,6 +35,24 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator do subject.perform end + + it 'removes the index preparation record from postgres_async_indexes' do + expect(async_index).to receive(:destroy!).and_call_original + + expect { subject.perform }.to change { index_model.count }.by(-1) + end + + it 'logs an appropriate message' do + expected_message = 'Skipping index creation since preconditions are not met. The queuing entry will be deleted' + + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + subject.perform + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: expected_message)) + end end it 'creates the index while controlling statement timeout' do @@ -47,7 +65,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator do end it 'removes the index preparation record from postgres_async_indexes' do - expect(async_index).to receive(:destroy).and_call_original + expect(async_index).to receive(:destroy!).and_call_original expect { subject.perform }.to change { index_model.count }.by(-1) end @@ -55,9 +73,23 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator do it 'skips logic if not able to acquire exclusive lease' do expect(lease).to receive(:try_obtain).ordered.and_return(false) expect(connection).not_to receive(:execute).with(/CREATE INDEX/) - expect(async_index).not_to receive(:destroy) + expect(async_index).not_to receive(:destroy!) expect { subject.perform }.not_to change { index_model.count } end + + it 'logs messages around execution' do + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + subject.perform + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: 'Starting async index creation')) + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: 'Finished async index creation')) + end end end diff --git a/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb index 11039ad4f7e..7f0febdcacd 100644 --- a/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor do +RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor, feature_category: :database do include ExclusiveLeaseHelpers describe '#perform' do @@ -16,7 +16,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor do let(:connection) { model.connection } let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } - let(:lease_key) { "gitlab/database/indexing/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } before do @@ -39,6 +39,24 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor do subject.perform end + + it 'removes the index preparation record from postgres_async_indexes' do + expect(async_index).to receive(:destroy!).and_call_original + + expect { subject.perform }.to change { index_model.count }.by(-1) + end + + it 'logs an appropriate message' do + expected_message = 'Skipping index removal since preconditions are not met. The queuing entry will be deleted' + + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + subject.perform + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: expected_message)) + end end it 'creates the index while controlling lock timeout' do @@ -53,7 +71,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor do end it 'removes the index preparation record from postgres_async_indexes' do - expect(async_index).to receive(:destroy).and_call_original + expect(async_index).to receive(:destroy!).and_call_original expect { subject.perform }.to change { index_model.count }.by(-1) end @@ -61,9 +79,23 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor do it 'skips logic if not able to acquire exclusive lease' do expect(lease).to receive(:try_obtain).ordered.and_return(false) expect(connection).not_to receive(:execute).with(/DROP INDEX/) - expect(async_index).not_to receive(:destroy) + expect(async_index).not_to receive(:destroy!) expect { subject.perform }.not_to change { index_model.count } end + + it 'logs messages around execution' do + allow(Gitlab::AppLogger).to receive(:info).and_call_original + + subject.perform + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: 'Starting async index removal')) + + expect(Gitlab::AppLogger) + .to have_received(:info) + .with(a_hash_including(message: 'Finished async index removal')) + end end end diff --git a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb index 52f5e37eff2..7c5c368fcb5 100644 --- a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::AsyncIndexes::MigrationHelpers do +RSpec.describe Gitlab::Database::AsyncIndexes::MigrationHelpers, feature_category: :database do let(:migration) { ActiveRecord::Migration.new.extend(described_class) } let(:index_model) { Gitlab::Database::AsyncIndexes::PostgresAsyncIndex } let(:connection) { ApplicationRecord.connection } diff --git a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb index 806d57af4b3..5e9d4f78a4a 100644 --- a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb @@ -2,12 +2,13 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model do +RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model, feature_category: :database do it { is_expected.to be_a Gitlab::Database::SharedModel } describe 'validations' do let(:identifier_limit) { described_class::MAX_IDENTIFIER_LENGTH } let(:definition_limit) { described_class::MAX_DEFINITION_LENGTH } + let(:last_error_limit) { described_class::MAX_LAST_ERROR_LENGTH } it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_length_of(:name).is_at_most(identifier_limit) } @@ -15,11 +16,12 @@ RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model it { is_expected.to validate_length_of(:table_name).is_at_most(identifier_limit) } it { is_expected.to validate_presence_of(:definition) } it { is_expected.to validate_length_of(:definition).is_at_most(definition_limit) } + it { is_expected.to validate_length_of(:last_error).is_at_most(last_error_limit) } end describe 'scopes' do - let!(:async_index_creation) { create(:postgres_async_index) } - let!(:async_index_destruction) { create(:postgres_async_index, :with_drop) } + let_it_be(:async_index_creation) { create(:postgres_async_index) } + let_it_be(:async_index_destruction) { create(:postgres_async_index, :with_drop) } describe '.to_create' do subject { described_class.to_create } @@ -32,5 +34,33 @@ RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model it { is_expected.to contain_exactly(async_index_destruction) } end + + describe '.ordered' do + before do + async_index_creation.update!(attempts: 3) + end + + subject { described_class.ordered.limit(1) } + + it { is_expected.to contain_exactly(async_index_destruction) } + end + end + + describe '#handle_exception!' do + let_it_be_with_reload(:async_index_creation) { create(:postgres_async_index) } + + let(:error) { instance_double(StandardError, message: 'Oups', backtrace: %w[this that]) } + + subject { async_index_creation.handle_exception!(error) } + + it 'increases the attempts number' do + expect { subject }.to change { async_index_creation.reload.attempts }.by(1) + end + + it 'saves error details' do + subject + + expect(async_index_creation.reload.last_error).to eq("Oups\nthis\nthat") + end end end diff --git a/spec/lib/gitlab/database/async_indexes_spec.rb b/spec/lib/gitlab/database/async_indexes_spec.rb index 8a5509f892f..c6991bf4e06 100644 --- a/spec/lib/gitlab/database/async_indexes_spec.rb +++ b/spec/lib/gitlab/database/async_indexes_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::AsyncIndexes do +RSpec.describe Gitlab::Database::AsyncIndexes, feature_category: :database do describe '.create_pending_indexes!' do subject { described_class.create_pending_indexes! } @@ -11,9 +11,9 @@ RSpec.describe Gitlab::Database::AsyncIndexes do end it 'takes 2 pending indexes and creates those' do - Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_create.order(:id).limit(2).each do |index| - creator = double('index creator') - expect(Gitlab::Database::AsyncIndexes::IndexCreator).to receive(:new).with(index).and_return(creator) + indexes = described_class::PostgresAsyncIndex.to_create.order(:id).limit(2).to_a + + expect_next_instances_of(described_class::IndexCreator, 2, indexes) do |creator| expect(creator).to receive(:perform) end @@ -29,13 +29,56 @@ RSpec.describe Gitlab::Database::AsyncIndexes do end it 'takes 2 pending indexes and destroys those' do - Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_drop.order(:id).limit(2).each do |index| - destructor = double('index destructor') - expect(Gitlab::Database::AsyncIndexes::IndexDestructor).to receive(:new).with(index).and_return(destructor) + indexes = described_class::PostgresAsyncIndex.to_drop.order(:id).limit(2).to_a + + expect_next_instances_of(described_class::IndexDestructor, 2, indexes) do |destructor| expect(destructor).to receive(:perform) end subject end end + + describe '.execute_pending_actions!' do + subject { described_class.execute_pending_actions!(how_many: how_many) } + + let_it_be(:failed_creation_entry) { create(:postgres_async_index, attempts: 5) } + let_it_be(:failed_removal_entry) { create(:postgres_async_index, :with_drop, attempts: 1) } + let_it_be(:creation_entry) { create(:postgres_async_index) } + let_it_be(:removal_entry) { create(:postgres_async_index, :with_drop) } + + context 'with one entry' do + let(:how_many) { 1 } + + it 'executes instructions ordered by attempts and ids' do + expect { subject } + .to change { queued_entries_exist?(creation_entry) }.to(false) + .and change { described_class::PostgresAsyncIndex.count }.by(-how_many) + end + end + + context 'with two entries' do + let(:how_many) { 2 } + + it 'executes instructions ordered by attempts' do + expect { subject } + .to change { queued_entries_exist?(creation_entry, removal_entry) }.to(false) + .and change { described_class::PostgresAsyncIndex.count }.by(-how_many) + end + end + + context 'when the budget allows more instructions' do + let(:how_many) { 3 } + + it 'retries failed attempts' do + expect { subject } + .to change { queued_entries_exist?(creation_entry, removal_entry, failed_removal_entry) }.to(false) + .and change { described_class::PostgresAsyncIndex.count }.by(-how_many) + end + end + + def queued_entries_exist?(*records) + described_class::PostgresAsyncIndex.where(id: records).exists? + end + end end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb index 31ae5e9b55d..d132559acea 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -897,7 +897,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end it 'doesn not filter by gitlab schemas available for the connection if the column is nor present' do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect(described_class).to receive(:gitlab_schema_column_exists?).and_return(false) diff --git a/spec/lib/gitlab/database/batch_count_spec.rb b/spec/lib/gitlab/database/batch_count_spec.rb index 852cc719d01..53f8fe3dcd2 100644 --- a/spec/lib/gitlab/database/batch_count_spec.rb +++ b/spec/lib/gitlab/database/batch_count_spec.rb @@ -58,10 +58,10 @@ RSpec.describe Gitlab::Database::BatchCount do it 'reduces batch size by half and retry fetch' do too_big_batch_relation_mock = instance_double(ActiveRecord::Relation) allow(model).to receive_message_chain(:select, public_send: relation) - allow(relation).to receive(:where).with("id" => 0..calculate_batch_size(batch_size)).and_return(too_big_batch_relation_mock) + allow(relation).to receive(:where).with({ "id" => 0..calculate_batch_size(batch_size) }).and_return(too_big_batch_relation_mock) allow(too_big_batch_relation_mock).to receive(:send).and_raise(ActiveRecord::QueryCanceled) - expect(relation).to receive(:where).with("id" => 0..calculate_batch_size(batch_size / 2)).and_return(double(send: 1)) + expect(relation).to receive(:where).with({ "id" => 0..calculate_batch_size(batch_size / 2) }).and_return(double(send: 1)) subject.call(model, column, batch_size: batch_size, start: 0) end @@ -146,7 +146,7 @@ RSpec.describe Gitlab::Database::BatchCount do allow(model).to receive_message_chain(:select, public_send: relation) batch_end_id = min_id + calculate_batch_size(Gitlab::Database::BatchCounter::DEFAULT_BATCH_SIZE) - expect(relation).to receive(:where).with("id" => min_id..batch_end_id).and_return(double(send: 1)) + expect(relation).to receive(:where).with({ "id" => min_id..batch_end_id }).and_return(double(send: 1)) described_class.batch_count(model) end @@ -382,7 +382,7 @@ RSpec.describe Gitlab::Database::BatchCount do allow(model).to receive_message_chain(:select, public_send: relation) batch_end_id = min_id + calculate_batch_size(Gitlab::Database::BatchCounter::DEFAULT_DISTINCT_BATCH_SIZE) - expect(relation).to receive(:where).with("id" => min_id..batch_end_id).and_return(double(send: 1)) + expect(relation).to receive(:where).with({ "id" => min_id..batch_end_id }).and_return(double(send: 1)) described_class.batch_distinct_count(model) end @@ -468,7 +468,7 @@ RSpec.describe Gitlab::Database::BatchCount do allow(model).to receive_message_chain(:select, public_send: relation) batch_end_id = min_id + calculate_batch_size(Gitlab::Database::BatchCounter::DEFAULT_SUM_BATCH_SIZE) - expect(relation).to receive(:where).with("id" => min_id..batch_end_id).and_return(double(send: 1)) + expect(relation).to receive(:where).with({ "id" => min_id..batch_end_id }).and_return(double(send: 1)) described_class.batch_sum(model, column) end diff --git a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb index 1e316c55786..ff31a5cd6cb 100644 --- a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb @@ -11,304 +11,319 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do Gitlab::Database::LoadBalancing::Session.clear_session end - describe '#stick_or_unstick_request' do - it 'sticks or unsticks a single object and updates the Rack environment' do - expect(sticking) - .to receive(:unstick_or_continue_sticking) - .with(:user, 42) - - env = {} - - sticking.stick_or_unstick_request(env, :user, 42) - - expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a) - .to eq([[sticking, :user, 42]]) + shared_examples 'sticking' do + before do + allow(ActiveRecord::Base.load_balancer) + .to receive(:primary_write_location) + .and_return('foo') end - it 'sticks or unsticks multiple objects and updates the Rack environment' do - expect(sticking) - .to receive(:unstick_or_continue_sticking) - .with(:user, 42) - .ordered + it 'sticks an entity to the primary', :aggregate_failures do + allow(ActiveRecord::Base.load_balancer) + .to receive(:primary_only?) + .and_return(false) - expect(sticking) - .to receive(:unstick_or_continue_sticking) - .with(:runner, '123456789') - .ordered + ids.each do |id| + expect(sticking) + .to receive(:set_write_location_for) + .with(:user, id, 'foo') + end - env = {} + expect(Gitlab::Database::LoadBalancing::Session.current) + .to receive(:use_primary!) - sticking.stick_or_unstick_request(env, :user, 42) - sticking.stick_or_unstick_request(env, :runner, '123456789') + subject + end - expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a).to eq( - [ - [sticking, :user, 42], - [sticking, :runner, - '123456789'] - ]) + it 'does not update the write location when no replicas are used' do + expect(sticking).not_to receive(:set_write_location_for) + + subject end end - describe '#stick_if_necessary' do - it 'does not stick if no write was performed' do - allow(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:performed_write?) - .and_return(false) + shared_examples 'tracking status in redis' do + describe '#stick_or_unstick_request' do + it 'sticks or unsticks a single object and updates the Rack environment' do + expect(sticking) + .to receive(:unstick_or_continue_sticking) + .with(:user, 42) - expect(sticking).not_to receive(:stick) + env = {} - sticking.stick_if_necessary(:user, 42) - end + sticking.stick_or_unstick_request(env, :user, 42) - it 'sticks to the primary if a write was performed' do - allow(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:performed_write?) - .and_return(true) + expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a) + .to eq([[sticking, :user, 42]]) + end - expect(sticking) - .to receive(:stick) - .with(:user, 42) + it 'sticks or unsticks multiple objects and updates the Rack environment' do + expect(sticking) + .to receive(:unstick_or_continue_sticking) + .with(:user, 42) + .ordered - sticking.stick_if_necessary(:user, 42) - end - end + expect(sticking) + .to receive(:unstick_or_continue_sticking) + .with(:runner, '123456789') + .ordered - describe '#all_caught_up?' do - let(:lb) { ActiveRecord::Base.load_balancer } - let(:last_write_location) { 'foo' } + env = {} - before do - allow(ActiveSupport::Notifications).to receive(:instrument).and_call_original + sticking.stick_or_unstick_request(env, :user, 42) + sticking.stick_or_unstick_request(env, :runner, '123456789') - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return(last_write_location) + expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a).to eq( + [ + [sticking, :user, 42], + [sticking, :runner, + '123456789'] + ]) + end end - context 'when no write location could be found' do - let(:last_write_location) { nil } + describe '#stick_if_necessary' do + it 'does not stick if no write was performed' do + allow(Gitlab::Database::LoadBalancing::Session.current) + .to receive(:performed_write?) + .and_return(false) - it 'returns true' do - expect(lb).not_to receive(:select_up_to_date_host) + expect(sticking).not_to receive(:stick) - expect(sticking.all_caught_up?(:user, 42)).to eq(true) + sticking.stick_if_necessary(:user, 42) end - end - context 'when all secondaries have caught up' do - before do - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) - end + it 'sticks to the primary if a write was performed' do + allow(Gitlab::Database::LoadBalancing::Session.current) + .to receive(:performed_write?) + .and_return(true) - it 'returns true, and unsticks' do expect(sticking) - .to receive(:unstick) + .to receive(:stick) .with(:user, 42) - expect(sticking.all_caught_up?(:user, 42)).to eq(true) - end - - it 'notifies with the proper event payload' do - expect(ActiveSupport::Notifications) - .to receive(:instrument) - .with('caught_up_replica_pick.load_balancing', { result: true }) - .and_call_original - - sticking.all_caught_up?(:user, 42) + sticking.stick_if_necessary(:user, 42) end end - context 'when the secondaries have not yet caught up' do + describe '#all_caught_up?' do + let(:lb) { ActiveRecord::Base.load_balancer } + let(:last_write_location) { 'foo' } + before do - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) - end + allow(ActiveSupport::Notifications).to receive(:instrument).and_call_original - it 'returns false' do - expect(sticking.all_caught_up?(:user, 42)).to eq(false) + allow(sticking) + .to receive(:last_write_location_for) + .with(:user, 42) + .and_return(last_write_location) end - it 'notifies with the proper event payload' do - expect(ActiveSupport::Notifications) - .to receive(:instrument) - .with('caught_up_replica_pick.load_balancing', { result: false }) - .and_call_original + context 'when no write location could be found' do + let(:last_write_location) { nil } + + it 'returns true' do + expect(lb).not_to receive(:select_up_to_date_host) - sticking.all_caught_up?(:user, 42) + expect(sticking.all_caught_up?(:user, 42)).to eq(true) + end end - end - end - describe '#unstick_or_continue_sticking' do - let(:lb) { ActiveRecord::Base.load_balancer } + context 'when all secondaries have caught up' do + before do + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) + end - it 'simply returns if no write location could be found' do - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return(nil) + it 'returns true, and unsticks' do + expect(sticking) + .to receive(:unstick) + .with(:user, 42) - expect(lb).not_to receive(:select_up_to_date_host) + expect(sticking.all_caught_up?(:user, 42)).to eq(true) + end - sticking.unstick_or_continue_sticking(:user, 42) - end + it 'notifies with the proper event payload' do + expect(ActiveSupport::Notifications) + .to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: true }) + .and_call_original - it 'unsticks if all secondaries have caught up' do - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return('foo') + sticking.all_caught_up?(:user, 42) + end + end - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) + context 'when the secondaries have not yet caught up' do + before do + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) + end - expect(sticking) - .to receive(:unstick) - .with(:user, 42) + it 'returns false' do + expect(sticking.all_caught_up?(:user, 42)).to eq(false) + end - sticking.unstick_or_continue_sticking(:user, 42) + it 'notifies with the proper event payload' do + expect(ActiveSupport::Notifications) + .to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: false }) + .and_call_original + + sticking.all_caught_up?(:user, 42) + end + end end - it 'continues using the primary if the secondaries have not yet caught up' do - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return('foo') + describe '#unstick_or_continue_sticking' do + let(:lb) { ActiveRecord::Base.load_balancer } - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) + it 'simply returns if no write location could be found' do + allow(sticking) + .to receive(:last_write_location_for) + .with(:user, 42) + .and_return(nil) - expect(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:use_primary!) + expect(lb).not_to receive(:select_up_to_date_host) - sticking.unstick_or_continue_sticking(:user, 42) - end - end + sticking.unstick_or_continue_sticking(:user, 42) + end - RSpec.shared_examples 'sticking' do - before do - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_write_location) - .and_return('foo') - end + it 'unsticks if all secondaries have caught up' do + allow(sticking) + .to receive(:last_write_location_for) + .with(:user, 42) + .and_return('foo') - it 'sticks an entity to the primary', :aggregate_failures do - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_only?) - .and_return(false) + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) - ids.each do |id| expect(sticking) - .to receive(:set_write_location_for) - .with(:user, id, 'foo') + .to receive(:unstick) + .with(:user, 42) + + sticking.unstick_or_continue_sticking(:user, 42) end - expect(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:use_primary!) + it 'continues using the primary if the secondaries have not yet caught up' do + allow(sticking) + .to receive(:last_write_location_for) + .with(:user, 42) + .and_return('foo') - subject - end + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) - it 'does not update the write location when no replicas are used' do - expect(sticking).not_to receive(:set_write_location_for) + expect(Gitlab::Database::LoadBalancing::Session.current) + .to receive(:use_primary!) - subject + sticking.unstick_or_continue_sticking(:user, 42) + end end - end - describe '#stick' do - it_behaves_like 'sticking' do - let(:ids) { [42] } - subject { sticking.stick(:user, ids.first) } + describe '#stick' do + it_behaves_like 'sticking' do + let(:ids) { [42] } + subject { sticking.stick(:user, ids.first) } + end end - end - describe '#bulk_stick' do - it_behaves_like 'sticking' do - let(:ids) { [42, 43] } - subject { sticking.bulk_stick(:user, ids) } + describe '#bulk_stick' do + it_behaves_like 'sticking' do + let(:ids) { [42, 43] } + subject { sticking.bulk_stick(:user, ids) } + end end - end - describe '#mark_primary_write_location' do - it 'updates the write location with the load balancer' do - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_write_location) - .and_return('foo') + describe '#mark_primary_write_location' do + it 'updates the write location with the load balancer' do + allow(ActiveRecord::Base.load_balancer) + .to receive(:primary_write_location) + .and_return('foo') - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_only?) - .and_return(false) + allow(ActiveRecord::Base.load_balancer) + .to receive(:primary_only?) + .and_return(false) + + expect(sticking) + .to receive(:set_write_location_for) + .with(:user, 42, 'foo') + + sticking.mark_primary_write_location(:user, 42) + end - expect(sticking) - .to receive(:set_write_location_for) - .with(:user, 42, 'foo') + it 'does nothing when no replicas are used' do + expect(sticking).not_to receive(:set_write_location_for) - sticking.mark_primary_write_location(:user, 42) + sticking.mark_primary_write_location(:user, 42) + end end - it 'does nothing when no replicas are used' do - expect(sticking).not_to receive(:set_write_location_for) + describe '#unstick' do + it 'removes the sticking data from Redis' do + sticking.set_write_location_for(:user, 4, 'foo') + sticking.unstick(:user, 4) - sticking.mark_primary_write_location(:user, 42) + expect(sticking.last_write_location_for(:user, 4)).to be_nil + end end - end - describe '#unstick' do - it 'removes the sticking data from Redis' do - sticking.set_write_location_for(:user, 4, 'foo') - sticking.unstick(:user, 4) + describe '#last_write_location_for' do + it 'returns the last WAL write location for a user' do + sticking.set_write_location_for(:user, 4, 'foo') - expect(sticking.last_write_location_for(:user, 4)).to be_nil + expect(sticking.last_write_location_for(:user, 4)).to eq('foo') + end end - end - describe '#last_write_location_for' do - it 'returns the last WAL write location for a user' do - sticking.set_write_location_for(:user, 4, 'foo') + describe '#select_caught_up_replicas' do + let(:lb) { ActiveRecord::Base.load_balancer } + + context 'with no write location' do + before do + allow(sticking) + .to receive(:last_write_location_for) + .with(:project, 42) + .and_return(nil) + end + + it 'returns false and does not try to find caught up hosts' do + expect(lb).not_to receive(:select_up_to_date_host) + expect(sticking.select_caught_up_replicas(:project, 42)).to be false + end + end - expect(sticking.last_write_location_for(:user, 4)).to eq('foo') + context 'with write location' do + before do + allow(sticking) + .to receive(:last_write_location_for) + .with(:project, 42) + .and_return('foo') + end + + it 'returns true, selects hosts, and unsticks if any secondary has caught up' do + expect(lb).to receive(:select_up_to_date_host).and_return(true) + expect(sticking) + .to receive(:unstick) + .with(:project, 42) + expect(sticking.select_caught_up_replicas(:project, 42)).to be true + end + end end end - describe '#redis_key_for' do - it 'returns a String' do - expect(sticking.redis_key_for(:user, 42)) - .to eq('database-load-balancing/write-location/main/user/42') - end + context 'with multi-store feature flags turned on' do + it_behaves_like 'tracking status in redis' end - describe '#select_caught_up_replicas' do - let(:lb) { ActiveRecord::Base.load_balancer } - - context 'with no write location' do - before do - allow(sticking) - .to receive(:last_write_location_for) - .with(:project, 42) - .and_return(nil) - end - - it 'returns false and does not try to find caught up hosts' do - expect(lb).not_to receive(:select_up_to_date_host) - expect(sticking.select_caught_up_replicas(:project, 42)).to be false - end + context 'when both multi-store feature flags are off' do + before do + stub_feature_flags(use_primary_and_secondary_stores_for_db_load_balancing: false) + stub_feature_flags(use_primary_store_as_default_for_db_load_balancing: false) end - context 'with write location' do - before do - allow(sticking) - .to receive(:last_write_location_for) - .with(:project, 42) - .and_return('foo') - end + it_behaves_like 'tracking status in redis' + end - it 'returns true, selects hosts, and unsticks if any secondary has caught up' do - expect(lb).to receive(:select_up_to_date_host).and_return(true) - expect(sticking) - .to receive(:unstick) - .with(:project, 42) - expect(sticking.select_caught_up_replicas(:project, 42)).to be true - end + describe '#redis_key_for' do + it 'returns a String' do + expect(sticking.redis_key_for(:user, 42)) + .to eq('database-load-balancing/write-location/main/user/42') end end end diff --git a/spec/lib/gitlab/database/load_balancing/transaction_leaking_spec.rb b/spec/lib/gitlab/database/load_balancing/transaction_leaking_spec.rb index 1eb077fe6ca..56fbaef031d 100644 --- a/spec/lib/gitlab/database/load_balancing/transaction_leaking_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/transaction_leaking_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'Load balancer behavior with errors inside a transaction', :redis, :delete do +RSpec.describe 'Load balancer behavior with errors inside a transaction', :redis, :delete, feature_category: :database do # rubocop:disable Layout/LineLength include StubENV let(:model) { ActiveRecord::Base } let(:db_host) { model.connection_pool.db_config.host } @@ -55,50 +55,8 @@ RSpec.describe 'Load balancer behavior with errors inside a transaction', :redis conn.execute("INSERT INTO #{test_table_name} (value) VALUES (2)") end - context 'with the PREVENT_LOAD_BALANCER_RETRIES_IN_TRANSACTION environment variable not set' do - it 'logs a warning when violating transaction semantics with writes' do - conn = model.connection - - expect(::Gitlab::Database::LoadBalancing::Logger).to receive(:warn).with(hash_including(event: :transaction_leak)) - expect(::Gitlab::Database::LoadBalancing::Logger).to receive(:warn).with(hash_including(event: :read_write_retry)) - - conn.transaction do - expect(conn).to be_transaction_open - - execute(conn) - - expect(conn).not_to be_transaction_open - end - - values = conn.execute("SELECT value FROM #{test_table_name}").to_a.map { |row| row['value'] } - expect(values).to contain_exactly(2) # Does not include 1 because the transaction was aborted and leaked - end - - it 'does not log a warning when no transaction is open to be leaked' do - conn = model.connection - - expect(::Gitlab::Database::LoadBalancing::Logger) - .not_to receive(:warn).with(hash_including(event: :transaction_leak)) - expect(::Gitlab::Database::LoadBalancing::Logger) - .to receive(:warn).with(hash_including(event: :read_write_retry)) - - expect(conn).not_to be_transaction_open - - execute(conn) - - expect(conn).not_to be_transaction_open - - values = conn.execute("SELECT value FROM #{test_table_name}").to_a.map { |row| row['value'] } - expect(values).to contain_exactly(1, 2) # Includes both rows because there was no transaction to roll back - end - end - - context 'with the PREVENT_LOAD_BALANCER_RETRIES_IN_TRANSACTION environment variable set' do - before do - stub_env('PREVENT_LOAD_BALANCER_RETRIES_IN_TRANSACTION' => '1') - end - - it 'raises an exception when a retry would occur during a transaction' do + context 'in a transaction' do + it 'raises an exception when a retry would occur' do expect(::Gitlab::Database::LoadBalancing::Logger) .not_to receive(:warn).with(hash_including(event: :transaction_leak)) @@ -108,8 +66,10 @@ RSpec.describe 'Load balancer behavior with errors inside a transaction', :redis end end.to raise_error(ActiveRecord::StatementInvalid) { |e| expect(e.cause).to be_a(PG::ConnectionBad) } end + end - it 'retries when not in a transaction' do + context 'without a transaction' do + it 'retries' do expect(::Gitlab::Database::LoadBalancing::Logger) .not_to receive(:warn).with(hash_including(event: :transaction_leak)) expect(::Gitlab::Database::LoadBalancing::Logger) diff --git a/spec/lib/gitlab/database/load_balancing_spec.rb b/spec/lib/gitlab/database/load_balancing_spec.rb index 1c85abac91c..59e16e6ca8b 100644 --- a/spec/lib/gitlab/database/load_balancing_spec.rb +++ b/spec/lib/gitlab/database/load_balancing_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing, :suppress_gitlab_schemas_validate_connection do +RSpec.describe Gitlab::Database::LoadBalancing, :suppress_gitlab_schemas_validate_connection, feature_category: :pods do describe '.base_models' do it 'returns the models to apply load balancing to' do models = described_class.base_models diff --git a/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb b/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb index 089c7a779f2..be9346e3829 100644 --- a/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb @@ -86,7 +86,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, let(:create_gitlab_shared_table_migration_class) { create_table_migration(gitlab_shared_table_name) } before do - skip_if_multiple_databases_are_setup + skip_if_multiple_databases_are_setup(:ci) end it 'does not lock any newly created tables' do @@ -106,7 +106,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when multiple databases' do before do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) end let(:migration_class) { create_table_migration(table_name, skip_automatic_lock_on_writes) } @@ -238,7 +238,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when renaming a table' do before do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) create_table_migration(old_table_name).migrate(:up) # create the table first before renaming it end @@ -277,7 +277,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, let(:config_model) { Gitlab::Database.database_base_models[:main] } before do - skip_if_multiple_databases_are_setup + skip_if_multiple_databases_are_setup(:ci) end it 'does not lock any newly created tables' do @@ -305,7 +305,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when multiple databases' do before do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) migration_class.connection.execute("CREATE TABLE #{table_name}()") migration_class.migrate(:up) end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 12fa115cc4e..9df23776be8 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -1047,59 +1047,63 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe '#foreign_key_exists?' do + let(:referenced_table_name) { '_test_gitlab_main_referenced' } + let(:referencing_table_name) { '_test_gitlab_main_referencing' } + before do model.connection.execute(<<~SQL) - create table referenced ( + create table #{referenced_table_name} ( id bigserial primary key not null ); - create table referencing ( + create table #{referencing_table_name} ( id bigserial primary key not null, non_standard_id bigint not null, - constraint fk_referenced foreign key (non_standard_id) references referenced(id) on delete cascade + constraint fk_referenced foreign key (non_standard_id) + references #{referenced_table_name}(id) on delete cascade ); SQL end shared_examples_for 'foreign key checks' do it 'finds existing foreign keys by column' do - expect(model.foreign_key_exists?(:referencing, target_table, column: :non_standard_id)).to be_truthy + expect(model.foreign_key_exists?(referencing_table_name, target_table, column: :non_standard_id)).to be_truthy end it 'finds existing foreign keys by name' do - expect(model.foreign_key_exists?(:referencing, target_table, name: :fk_referenced)).to be_truthy + expect(model.foreign_key_exists?(referencing_table_name, target_table, name: :fk_referenced)).to be_truthy end it 'finds existing foreign_keys by name and column' do - expect(model.foreign_key_exists?(:referencing, target_table, name: :fk_referenced, column: :non_standard_id)).to be_truthy + expect(model.foreign_key_exists?(referencing_table_name, target_table, name: :fk_referenced, column: :non_standard_id)).to be_truthy end it 'finds existing foreign_keys by name, column and on_delete' do - expect(model.foreign_key_exists?(:referencing, target_table, name: :fk_referenced, column: :non_standard_id, on_delete: :cascade)).to be_truthy + expect(model.foreign_key_exists?(referencing_table_name, target_table, name: :fk_referenced, column: :non_standard_id, on_delete: :cascade)).to be_truthy end it 'finds existing foreign keys by target table only' do - expect(model.foreign_key_exists?(:referencing, target_table)).to be_truthy + expect(model.foreign_key_exists?(referencing_table_name, target_table)).to be_truthy end it 'compares by column name if given' do - expect(model.foreign_key_exists?(:referencing, target_table, column: :user_id)).to be_falsey + expect(model.foreign_key_exists?(referencing_table_name, target_table, column: :user_id)).to be_falsey end it 'compares by target column name if given' do - expect(model.foreign_key_exists?(:referencing, target_table, primary_key: :user_id)).to be_falsey - expect(model.foreign_key_exists?(:referencing, target_table, primary_key: :id)).to be_truthy + expect(model.foreign_key_exists?(referencing_table_name, target_table, primary_key: :user_id)).to be_falsey + expect(model.foreign_key_exists?(referencing_table_name, target_table, primary_key: :id)).to be_truthy end it 'compares by foreign key name if given' do - expect(model.foreign_key_exists?(:referencing, target_table, name: :non_existent_foreign_key_name)).to be_falsey + expect(model.foreign_key_exists?(referencing_table_name, target_table, name: :non_existent_foreign_key_name)).to be_falsey end it 'compares by foreign key name and column if given' do - expect(model.foreign_key_exists?(:referencing, target_table, name: :non_existent_foreign_key_name, column: :non_standard_id)).to be_falsey + expect(model.foreign_key_exists?(referencing_table_name, target_table, name: :non_existent_foreign_key_name, column: :non_standard_id)).to be_falsey end it 'compares by foreign key name, column and on_delete if given' do - expect(model.foreign_key_exists?(:referencing, target_table, name: :fk_referenced, column: :non_standard_id, on_delete: :nullify)).to be_falsey + expect(model.foreign_key_exists?(referencing_table_name, target_table, name: :fk_referenced, column: :non_standard_id, on_delete: :nullify)).to be_falsey end end @@ -1110,7 +1114,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end context 'specifying a target table' do - let(:target_table) { :referenced } + let(:target_table) { referenced_table_name } it_behaves_like 'foreign key checks' end @@ -1121,64 +1125,78 @@ RSpec.describe Gitlab::Database::MigrationHelpers do it 'raises an error if an invalid on_delete is specified' do # The correct on_delete key is "nullify" - expect { model.foreign_key_exists?(:referenced, on_delete: :set_null) }.to raise_error(ArgumentError) + expect { model.foreign_key_exists?(referenced_table_name, on_delete: :set_null) }.to raise_error(ArgumentError) end context 'with foreign key using multiple columns' do + let(:p_referenced_table_name) { '_test_gitlab_main_p_referenced' } + let(:p_referencing_table_name) { '_test_gitlab_main_p_referencing' } + before do model.connection.execute(<<~SQL) - create table p_referenced ( - id bigserial not null, - partition_number bigint not null default 100, - primary key (partition_number, id) - ); - create table p_referencing ( - id bigserial primary key not null, - partition_number bigint not null, - constraint fk_partitioning foreign key (partition_number, id) references p_referenced(partition_number, id) on delete cascade - ); + create table #{p_referenced_table_name} ( + id bigserial not null, + partition_number bigint not null default 100, + primary key (partition_number, id) + ); + create table #{p_referencing_table_name} ( + id bigserial primary key not null, + partition_number bigint not null, + constraint fk_partitioning foreign key (partition_number, id) + references #{p_referenced_table_name} (partition_number, id) on delete cascade + ); SQL end it 'finds existing foreign keys by columns' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, column: [:partition_number, :id])).to be_truthy + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + column: [:partition_number, :id])).to be_truthy end it 'finds existing foreign keys by name' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :fk_partitioning)).to be_truthy + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + name: :fk_partitioning)).to be_truthy end it 'finds existing foreign_keys by name and column' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :fk_partitioning, column: [:partition_number, :id])).to be_truthy + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + name: :fk_partitioning, column: [:partition_number, :id])).to be_truthy end it 'finds existing foreign_keys by name, column and on_delete' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :fk_partitioning, column: [:partition_number, :id], on_delete: :cascade)).to be_truthy + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + name: :fk_partitioning, column: [:partition_number, :id], on_delete: :cascade)).to be_truthy end it 'finds existing foreign keys by target table only' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced)).to be_truthy + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name)).to be_truthy end it 'compares by column name if given' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, column: :id)).to be_falsey + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + column: :id)).to be_falsey end it 'compares by target column name if given' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, primary_key: :user_id)).to be_falsey - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, primary_key: [:partition_number, :id])).to be_truthy + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + primary_key: :user_id)).to be_falsey + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + primary_key: [:partition_number, :id])).to be_truthy end it 'compares by foreign key name if given' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :non_existent_foreign_key_name)).to be_falsey + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + name: :non_existent_foreign_key_name)).to be_falsey end it 'compares by foreign key name and column if given' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :non_existent_foreign_key_name, column: [:partition_number, :id])).to be_falsey + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + name: :non_existent_foreign_key_name, column: [:partition_number, :id])).to be_falsey end it 'compares by foreign key name, column and on_delete if given' do - expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :fk_partitioning, column: [:partition_number, :id], on_delete: :nullify)).to be_falsey + expect(model.foreign_key_exists?(p_referencing_table_name, p_referenced_table_name, + name: :fk_partitioning, column: [:partition_number, :id], on_delete: :nullify)).to be_falsey end end end diff --git a/spec/lib/gitlab/database/migrations/batched_migration_last_id_spec.rb b/spec/lib/gitlab/database/migrations/batched_migration_last_id_spec.rb index 97b432406eb..1365adb8993 100644 --- a/spec/lib/gitlab/database/migrations/batched_migration_last_id_spec.rb +++ b/spec/lib/gitlab/database/migrations/batched_migration_last_id_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Migrations::BatchedMigrationLastId, feature_category: :pipeline_insights do +RSpec.describe Gitlab::Database::Migrations::BatchedMigrationLastId, feature_category: :database do subject(:test_sampling) { described_class.new(connection, base_dir) } let(:base_dir) { Pathname.new(Dir.mktmpdir) } diff --git a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb index b0bdbf5c371..4f347034c0b 100644 --- a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb +++ b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb @@ -18,6 +18,7 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do let(:migration_name) { 'test' } let(:migration_version) { '12345' } let(:migration_meta) { { 'max_batch_size' => 1, 'total_tuple_count' => 10, 'interval' => 60 } } + let(:expected_json_keys) { %w[version name walltime success total_database_size_change query_statistics] } it 'executes the given block' do expect do |b| @@ -81,7 +82,10 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do expect(subject.success).to be_truthy expect(subject.version).to eq(migration_version) expect(subject.name).to eq(migration_name) - expect(subject.meta).to eq(migration_meta) + end + + it 'transforms observation to expected json' do + expect(Gitlab::Json.parse(subject.to_json).keys).to contain_exactly(*expected_json_keys) end end @@ -114,7 +118,10 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do expect(subject['success']).to be_falsey expect(subject['version']).to eq(migration_version) expect(subject['name']).to eq(migration_name) - expect(subject['meta']).to include(migration_meta) + end + + it 'transforms observation to expected json' do + expect(Gitlab::Json.parse(subject.to_json).keys).to contain_exactly(*expected_json_keys) end end end diff --git a/spec/lib/gitlab/database/migrations/observers/batch_details_spec.rb b/spec/lib/gitlab/database/migrations/observers/batch_details_spec.rb new file mode 100644 index 00000000000..5b3c23736ed --- /dev/null +++ b/spec/lib/gitlab/database/migrations/observers/batch_details_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::Observers::BatchDetails, feature_category: :database do + subject(:observe) { described_class.new(observation, path, connection) } + + let(:connection) { ActiveRecord::Migration.connection } + let(:observation) { Gitlab::Database::Migrations::Observation.new(meta: meta) } + let(:path) { Dir.mktmpdir } + let(:file_name) { 'batch-details.json' } + let(:file_path) { Pathname.new(path).join(file_name) } + let(:json_file) { Gitlab::Json.parse(File.read(file_path)) } + let(:job_meta) do + { "min_value" => 1, "max_value" => 19, "batch_size" => 20, "sub_batch_size" => 5, "pause_ms" => 100 } + end + + where(:meta, :expected_keys) do + [ + [lazy { { job_meta: job_meta } }, %w[time_spent min_value max_value batch_size sub_batch_size pause_ms]], + [nil, %w[time_spent]], + [{ job_meta: nil }, %w[time_spent]] + ] + end + + with_them do + before do + observe.before + observe.after + end + + after do + FileUtils.remove_entry(path) + end + + it 'records expected information to file' do + observe.record + + expect(json_file.keys).to match_array(expected_keys) + end + end +end diff --git a/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb b/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb index 0b048617ce1..57c5011590c 100644 --- a/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freeze_time do +RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freeze_time, feature_category: :database do include Gitlab::Database::MigrationHelpers include Database::MigrationTestingHelpers @@ -45,18 +45,15 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez end with_them do - let(:result_dir) { Dir.mktmpdir } + let(:result_dir) { Pathname.new(Dir.mktmpdir) } + let(:connection) { base_model.connection } + let(:table_name) { "_test_column_copying" } + let(:from_id) { 0 } after do FileUtils.rm_rf(result_dir) end - let(:connection) { base_model.connection } - - let(:table_name) { "_test_column_copying" } - - let(:from_id) { 0 } - before do connection.execute(<<~SQL) CREATE TABLE #{table_name} ( @@ -70,26 +67,15 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez context 'running a real background migration' do let(:interval) { 5.minutes } - let(:meta) { { "max_batch_size" => nil, "total_tuple_count" => nil, "interval" => interval } } - - let(:params) do - { - version: nil, - connection: connection, - meta: { - interval: 300, - max_batch_size: nil, - total_tuple_count: nil - } - } - end + let(:params) { { version: nil, connection: connection } } + let(:migration_name) { 'CopyColumnUsingBackgroundMigrationJob' } + let(:migration_file_path) { result_dir.join('CopyColumnUsingBackgroundMigrationJob', 'details.json') } + let(:json_file) { Gitlab::Json.parse(File.read(migration_file_path)) } + let(:expected_file_keys) { %w[interval total_tuple_count max_batch_size] } before do - queue_migration('CopyColumnUsingBackgroundMigrationJob', - table_name, :id, - :id, :data, - batch_size: 100, - job_interval: interval) # job_interval is skipped when testing + # job_interval is skipped when testing + queue_migration(migration_name, table_name, :id, :id, :data, batch_size: 100, job_interval: interval) end subject(:sample_migration) do @@ -113,6 +99,20 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez subject end + + it 'uses the filtering clause from the migration' do + expect_next_instance_of(Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchingStrategy) do |s| + expect(s).to receive(:filter_batch).at_least(:once).and_call_original + end + + subject + end + + it 'exports migration details to a file' do + subject + + expect(json_file.keys).to match_array(expected_file_keys) + end end context 'with jobs to run' do diff --git a/spec/lib/gitlab/database/migrations/timeout_helpers_spec.rb b/spec/lib/gitlab/database/migrations/timeout_helpers_spec.rb index d35211af680..ee63ea7174b 100644 --- a/spec/lib/gitlab/database/migrations/timeout_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/timeout_helpers_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Migrations::TimeoutHelpers do +RSpec.describe Gitlab::Database::Migrations::TimeoutHelpers, feature_category: :database do let(:model) do ActiveRecord::Migration.new.extend(described_class) end diff --git a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb index 8027990a546..2212cb09888 100644 --- a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb @@ -6,22 +6,14 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do include Database::PartitioningHelpers include ExclusiveLeaseHelpers - def has_partition(model, month) - Gitlab::Database::PostgresPartition.for_parent_table(model.table_name).any? do |partition| - Gitlab::Database::Partitioning::TimePartition.from_sql( - model.table_name, - partition.name, - partition.condition - ).from == month - end - end + let(:partitioned_table_name) { "_test_gitlab_main_my_model_example_table" } context 'creating partitions (mocked)' do subject(:sync_partitions) { described_class.new(model).sync_partitions } let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table, connection: connection) } let(:connection) { ActiveRecord::Base.connection } - let(:table) { "my_model_example_table" } + let(:table) { partitioned_table_name } let(:partitioning_strategy) do double(missing_partitions: partitions, extra_partitions: [], after_adding_partitions: nil) end @@ -57,7 +49,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do let(:connection) { Ci::ApplicationRecord.connection } it 'uses the explicitly provided connection when any' do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect(connection).to receive(:execute).with("LOCK TABLE \"#{table}\" IN ACCESS EXCLUSIVE MODE") expect(connection).to receive(:execute).with(partitions.first.to_sql) @@ -102,14 +94,14 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do Class.new(ApplicationRecord) do include PartitionedTable - self.table_name = 'my_model_example_table' - partitioned_by :created_at, strategy: :monthly end end before do - create_partitioned_table(connection, 'my_model_example_table') + my_model.table_name = partitioned_table_name + + create_partitioned_table(connection, partitioned_table_name) end it 'creates partitions' do @@ -184,27 +176,27 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do Class.new(ApplicationRecord) do include PartitionedTable - self.table_name = 'my_model_example_table' - partitioned_by :created_at, strategy: :monthly, retain_for: 1.month end end before do connection.execute(<<~SQL) - CREATE TABLE my_model_example_table + CREATE TABLE #{partitioned_table_name} (id serial not null, created_at timestamptz not null, primary key (id, created_at)) PARTITION BY RANGE (created_at); - CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.my_model_example_table_202104 - PARTITION OF my_model_example_table + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.#{partitioned_table_name}_202104 + PARTITION OF #{partitioned_table_name} FOR VALUES FROM ('2021-04-01') TO ('2021-05-01'); - CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.my_model_example_table_202105 - PARTITION OF my_model_example_table + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.#{partitioned_table_name}_202105 + PARTITION OF #{partitioned_table_name} FOR VALUES FROM ('2021-05-01') TO ('2021-06-01'); SQL + my_model.table_name = partitioned_table_name + # Also create all future partitions so that the sync is only trying to detach old partitions my_model.partitioning_strategy.missing_partitions.each do |p| connection.execute p.to_sql @@ -234,7 +226,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do it 'creates the appropriate PendingPartitionDrop entry' do subject - pending_drop = Postgresql::DetachedPartition.find_by!(table_name: 'my_model_example_table_202104') + pending_drop = Postgresql::DetachedPartition.find_by!(table_name: "#{partitioned_table_name}_202104") expect(pending_drop.drop_after).to eq(Time.current + described_class::RETAIN_DETACHED_PARTITIONS_FOR) end @@ -243,11 +235,11 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do context 'when the model is the target of a foreign key' do before do connection.execute(<<~SQL) - create unique index idx_for_fk ON my_model_example_table(created_at); + create unique index idx_for_fk ON #{partitioned_table_name}(created_at); - create table referencing_table ( + create table _test_gitlab_main_referencing_table ( id bigserial primary key not null, - referencing_created_at timestamptz references my_model_example_table(created_at) + referencing_created_at timestamptz references #{partitioned_table_name}(created_at) ); SQL end @@ -265,15 +257,15 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do Class.new(ApplicationRecord) do include PartitionedTable - self.table_name = 'my_model_example_table' - partitioned_by :created_at, strategy: :monthly, retain_for: 1.month end end before do + my_model.table_name = partitioned_table_name + connection.execute(<<~SQL) - CREATE TABLE my_model_example_table + CREATE TABLE #{partitioned_table_name} (id serial not null, created_at timestamptz not null, primary key (id, created_at)) PARTITION BY RANGE (created_at); SQL @@ -294,6 +286,16 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do end end + def has_partition(model, month) + Gitlab::Database::PostgresPartition.for_parent_table(model.table_name).any? do |partition| + Gitlab::Database::Partitioning::TimePartition.from_sql( + model.table_name, + partition.name, + partition.condition + ).from == month + end + end + def create_partitioned_table(connection, table) connection.execute(<<~SQL) CREATE TABLE #{table} diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index 855d0bc46a4..ae74ee60a4b 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -117,7 +117,7 @@ RSpec.describe Gitlab::Database::Partitioning do end it 'creates partitions in each database' do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect { described_class.sync_partitions(models) } .to change { find_partitions(table_names.first, conn: connection).size }.from(0) @@ -176,7 +176,7 @@ RSpec.describe Gitlab::Database::Partitioning do end it 'manages partitions for models for the given database', :aggregate_failures do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect { described_class.sync_partitions([models.first, ci_model], only_on: 'ci') } .to change { find_partitions(ci_model.table_name, conn: ci_connection).size }.from(0) diff --git a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb index a8dbc4be16f..ae56f66737d 100644 --- a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb +++ b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb @@ -6,26 +6,32 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ # PostgresForeignKey does not `behaves_like 'a postgres model'` because it does not correspond 1-1 with a single entry # in pg_class + let(:table_prefix) { '_test_gitlab_main' } + before do ApplicationRecord.connection.execute(<<~SQL) - CREATE TABLE public.referenced_table ( + CREATE TABLE #{schema_table_name('referenced_table')} ( id bigserial primary key not null, id_b bigserial not null, UNIQUE (id, id_b) ); - CREATE TABLE public.other_referenced_table ( + CREATE TABLE #{schema_table_name('other_referenced_table')} ( id bigserial primary key not null ); - CREATE TABLE public.constrained_table ( + CREATE TABLE #{schema_table_name('constrained_table')} ( id bigserial primary key not null, referenced_table_id bigint not null, referenced_table_id_b bigint not null, other_referenced_table_id bigint not null, - CONSTRAINT fk_constrained_to_referenced FOREIGN KEY(referenced_table_id, referenced_table_id_b) REFERENCES referenced_table(id, id_b) on delete restrict, + CONSTRAINT fk_constrained_to_referenced + FOREIGN KEY(referenced_table_id, referenced_table_id_b) + REFERENCES #{table_name('referenced_table')}(id, id_b) + ON DELETE restrict + ON UPDATE restrict, CONSTRAINT fk_constrained_to_other_referenced FOREIGN KEY(other_referenced_table_id) - REFERENCES other_referenced_table(id) + REFERENCES #{table_name('other_referenced_table')}(id) ); SQL @@ -33,13 +39,13 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ describe '#by_referenced_table_identifier' do it 'throws an error when the identifier name is not fully qualified' do - expect { described_class.by_referenced_table_identifier('referenced_table') }.to raise_error(ArgumentError, /not fully qualified/) + expect { described_class.by_referenced_table_identifier(table_name("referenced_table")) }.to raise_error(ArgumentError, /not fully qualified/) end it 'finds the foreign keys for the referenced table' do expected = described_class.find_by!(name: 'fk_constrained_to_referenced') - expect(described_class.by_referenced_table_identifier('public.referenced_table')).to contain_exactly(expected) + expect(described_class.by_referenced_table_identifier(schema_table_name("referenced_table"))).to contain_exactly(expected) end end @@ -47,19 +53,19 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ it 'finds the foreign keys for the referenced table' do expected = described_class.find_by!(name: 'fk_constrained_to_referenced') - expect(described_class.by_referenced_table_name('referenced_table')).to contain_exactly(expected) + expect(described_class.by_referenced_table_name(table_name("referenced_table"))).to contain_exactly(expected) end end describe '#by_constrained_table_identifier' do it 'throws an error when the identifier name is not fully qualified' do - expect { described_class.by_constrained_table_identifier('constrained_table') }.to raise_error(ArgumentError, /not fully qualified/) + expect { described_class.by_constrained_table_identifier(table_name("constrained_table")) }.to raise_error(ArgumentError, /not fully qualified/) end it 'finds the foreign keys for the constrained table' do expected = described_class.where(name: %w[fk_constrained_to_referenced fk_constrained_to_other_referenced]).to_a - expect(described_class.by_constrained_table_identifier('public.constrained_table')).to match_array(expected) + expect(described_class.by_constrained_table_identifier(schema_table_name('constrained_table'))).to match_array(expected) end end @@ -67,7 +73,7 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ it 'finds the foreign keys for the constrained table' do expected = described_class.where(name: %w[fk_constrained_to_referenced fk_constrained_to_other_referenced]).to_a - expect(described_class.by_constrained_table_name('constrained_table')).to match_array(expected) + expect(described_class.by_constrained_table_name(table_name("constrained_table"))).to match_array(expected) end end @@ -80,7 +86,7 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ context 'when finding columns for foreign keys' do using RSpec::Parameterized::TableSyntax - let(:fks) { described_class.by_constrained_table_name('constrained_table') } + let(:fks) { described_class.by_constrained_table_name(table_name("constrained_table")) } where(:fk, :expected_constrained, :expected_referenced) do lazy { described_class.find_by(name: 'fk_constrained_to_referenced') } | %w[referenced_table_id referenced_table_id_b] | %w[id id_b] @@ -110,25 +116,34 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ end end - describe '#on_delete_action' do + describe '#on_delete_action and #on_update_action' do before do ApplicationRecord.connection.execute(<<~SQL) - create table public.referenced_table_all_on_delete_actions ( + create table #{schema_table_name('referenced_table_all_on_delete_actions')} ( id bigserial primary key not null ); - create table public.constrained_table_all_on_delete_actions ( + create table #{schema_table_name('constrained_table_all_on_delete_actions')} ( id bigserial primary key not null, - ref_id_no_action bigint not null constraint fk_no_action references referenced_table_all_on_delete_actions(id), - ref_id_restrict bigint not null constraint fk_restrict references referenced_table_all_on_delete_actions(id) on delete restrict, - ref_id_nullify bigint not null constraint fk_nullify references referenced_table_all_on_delete_actions(id) on delete set null, - ref_id_cascade bigint not null constraint fk_cascade references referenced_table_all_on_delete_actions(id) on delete cascade, - ref_id_set_default bigint not null constraint fk_set_default references referenced_table_all_on_delete_actions(id) on delete set default + ref_id_no_action bigint not null constraint fk_no_action + references #{table_name('referenced_table_all_on_delete_actions')}(id), + ref_id_restrict bigint not null constraint fk_restrict + references #{table_name('referenced_table_all_on_delete_actions')}(id) + on delete restrict on update restrict, + ref_id_nullify bigint not null constraint fk_nullify + references #{table_name('referenced_table_all_on_delete_actions')}(id) + on delete set null on update set null, + ref_id_cascade bigint not null constraint fk_cascade + references #{table_name('referenced_table_all_on_delete_actions')}(id) + on delete cascade on update cascade, + ref_id_set_default bigint not null constraint fk_set_default + references #{table_name('referenced_table_all_on_delete_actions')}(id) + on delete set default on update set default ) SQL end - let(:fks) { described_class.by_constrained_table_name('constrained_table_all_on_delete_actions') } + let(:fks) { described_class.by_constrained_table_name(table_name('constrained_table_all_on_delete_actions')) } context 'with an invalid on_delete_action' do it 'raises an error' do @@ -137,7 +152,7 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ end end - where(:fk_name, :expected_on_delete_action) do + where(:fk_name, :expected_action) do [ %w[fk_no_action no_action], %w[fk_restrict restrict], @@ -151,12 +166,22 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ subject(:fk) { fks.find_by(name: fk_name) } it 'has the appropriate on delete action' do - expect(fk.on_delete_action).to eq(expected_on_delete_action) + expect(fk.on_delete_action).to eq(expected_action) + end + + it 'has the appropriate on update action' do + expect(fk.on_update_action).to eq(expected_action) end describe '#by_on_delete_action' do it 'finds the key by on delete action' do - expect(fks.by_on_delete_action(expected_on_delete_action)).to contain_exactly(fk) + expect(fks.by_on_delete_action(expected_action)).to contain_exactly(fk) + end + end + + describe '#by_on_update_action' do + it 'finds the key by on update action' do + expect(fks.by_on_update_action(expected_action)).to contain_exactly(fk) end end end @@ -167,16 +192,17 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ skip('not supported before postgres 12') if ApplicationRecord.database.version.to_f < 12 ApplicationRecord.connection.execute(<<~SQL) - create table public.parent ( - id bigserial primary key not null - ) partition by hash(id); + create table #{schema_table_name('parent')} ( + id bigserial primary key not null + ) partition by hash(id); - create table public.child partition of parent for values with (modulus 2, remainder 1); + create table #{schema_table_name('child')} partition of #{table_name('parent')} + for values with (modulus 2, remainder 1); - create table public.referencing_partitioned ( - id bigserial not null primary key, - constraint fk_inherited foreign key (id) references parent(id) - ) + create table #{schema_table_name('referencing_partitioned')} ( + id bigserial not null primary key, + constraint fk_inherited foreign key (id) references #{table_name('parent')}(id) + ) SQL end @@ -185,7 +211,7 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ where(:fk, :inherited) do lazy { described_class.find_by(name: 'fk_inherited') } | false - lazy { described_class.by_referenced_table_identifier('public.child').first! } | true + lazy { described_class.by_referenced_table_identifier(schema_table_name('child')).first! } | true lazy { described_class.find_by(name: 'fk_constrained_to_referenced') } | false end @@ -197,12 +223,20 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_categ end describe '#not_inherited' do - let(:fks) { described_class.by_constrained_table_identifier('public.referencing_partitioned') } + let(:fks) { described_class.by_constrained_table_identifier(schema_table_name('referencing_partitioned')) } it 'lists all non-inherited foreign keys' do - expect(fks.pluck(:referenced_table_name)).to contain_exactly('parent', 'child') - expect(fks.not_inherited.pluck(:referenced_table_name)).to contain_exactly('parent') + expect(fks.pluck(:referenced_table_name)).to contain_exactly(table_name('parent'), table_name('child')) + expect(fks.not_inherited.pluck(:referenced_table_name)).to contain_exactly(table_name('parent')) end end end + + def schema_table_name(name) + "public.#{table_name(name)}" + end + + def table_name(name) + "#{table_prefix}_#{name}" + end end diff --git a/spec/lib/gitlab/database/postgres_index_spec.rb b/spec/lib/gitlab/database/postgres_index_spec.rb index db66736676b..d8a2612caf3 100644 --- a/spec/lib/gitlab/database/postgres_index_spec.rb +++ b/spec/lib/gitlab/database/postgres_index_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Gitlab::Database::PostgresIndex do CREATE INDEX #{name} ON public.users (name); CREATE UNIQUE INDEX bar_key ON public.users (id); - CREATE TABLE example_table (id serial primary key); + CREATE TABLE _test_gitlab_main_example_table (id serial primary key); SQL end @@ -144,7 +144,7 @@ RSpec.describe Gitlab::Database::PostgresIndex do end it 'returns true for a primary key index' do - expect(find('public.example_table_pkey')).to be_unique + expect(find('public._test_gitlab_main_example_table_pkey')).to be_unique end end diff --git a/spec/lib/gitlab/database/postgres_partitioned_table_spec.rb b/spec/lib/gitlab/database/postgres_partitioned_table_spec.rb index 21a46f1a0a6..170cc894071 100644 --- a/spec/lib/gitlab/database/postgres_partitioned_table_spec.rb +++ b/spec/lib/gitlab/database/postgres_partitioned_table_spec.rb @@ -3,25 +3,29 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PostgresPartitionedTable, type: :model do - let(:schema) { 'public' } - let(:name) { 'foo_range' } - let(:identifier) { "#{schema}.#{name}" } + let_it_be(:foo_range_table_name) { '_test_gitlab_main_foo_range' } + let_it_be(:foo_list_table_name) { '_test_gitlab_main_foo_list' } + let_it_be(:foo_hash_table_name) { '_test_gitlab_main_foo_hash' } - before do + let_it_be(:schema) { 'public' } + let_it_be(:name) { foo_range_table_name } + let_it_be(:identifier) { "#{schema}.#{name}" } + + before_all do ActiveRecord::Base.connection.execute(<<~SQL) - CREATE TABLE #{identifier} ( + CREATE TABLE #{schema}.#{foo_range_table_name} ( id serial NOT NULL, created_at timestamptz NOT NULL, PRIMARY KEY (id, created_at) ) PARTITION BY RANGE(created_at); - CREATE TABLE public.foo_list ( + CREATE TABLE #{schema}.#{foo_list_table_name} ( id serial NOT NULL, row_type text NOT NULL, PRIMARY KEY (id, row_type) ) PARTITION BY LIST(row_type); - CREATE TABLE public.foo_hash ( + CREATE TABLE #{schema}.#{foo_hash_table_name} ( id serial NOT NULL, row_value int NOT NULL, PRIMARY KEY (id, row_value) @@ -56,31 +60,63 @@ RSpec.describe Gitlab::Database::PostgresPartitionedTable, type: :model do end end + describe '.each_partition' do + context 'without partitions' do + it 'does not yield control' do + expect { |b| described_class.each_partition(name, &b) }.not_to yield_control + end + end + + context 'with partitions' do + let(:partition_schema) { 'gitlab_partitions_dynamic' } + let(:partition1_name) { "#{partition_schema}.#{name}_202001" } + let(:partition2_name) { "#{partition_schema}.#{name}_202002" } + + before do + ActiveRecord::Base.connection.execute(<<~SQL) + CREATE TABLE #{partition1_name} PARTITION OF #{identifier} + FOR VALUES FROM ('2020-01-01') TO ('2020-02-01'); + + CREATE TABLE #{partition2_name} PARTITION OF #{identifier} + FOR VALUES FROM ('2020-02-01') TO ('2020-03-01'); + SQL + end + + it 'yields control with partition as argument' do + args = Gitlab::Database::PostgresPartition + .where(identifier: [partition1_name, partition2_name]) + .order(:name).to_a + + expect { |b| described_class.each_partition(name, &b) }.to yield_successive_args(*args) + end + end + end + describe '#dynamic?' do it 'returns true for tables partitioned by range' do - expect(find('public.foo_range')).to be_dynamic + expect(find("#{schema}.#{foo_range_table_name}")).to be_dynamic end it 'returns true for tables partitioned by list' do - expect(find('public.foo_list')).to be_dynamic + expect(find("#{schema}.#{foo_list_table_name}")).to be_dynamic end it 'returns false for tables partitioned by hash' do - expect(find('public.foo_hash')).not_to be_dynamic + expect(find("#{schema}.#{foo_hash_table_name}")).not_to be_dynamic end end describe '#static?' do it 'returns false for tables partitioned by range' do - expect(find('public.foo_range')).not_to be_static + expect(find("#{schema}.#{foo_range_table_name}")).not_to be_static end it 'returns false for tables partitioned by list' do - expect(find('public.foo_list')).not_to be_static + expect(find("#{schema}.#{foo_list_table_name}")).not_to be_static end it 'returns true for tables partitioned by hash' do - expect(find('public.foo_hash')).to be_static + expect(find("#{schema}.#{foo_hash_table_name}")).to be_static end end diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb index 47038bbd138..d31be6cb883 100644 --- a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb @@ -28,19 +28,19 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection model: ApplicationRecord, sql: "SELECT 1 FROM projects LEFT JOIN ci_builds ON ci_builds.project_id=projects.id", expect_error: /The query tried to access \["projects", "ci_builds"\]/, - setup: -> (_) { skip_if_multiple_databases_not_setup } + setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } }, "for query accessing gitlab_ci and gitlab_main the gitlab_schemas is always ordered" => { model: ApplicationRecord, sql: "SELECT 1 FROM ci_builds LEFT JOIN projects ON ci_builds.project_id=projects.id", expect_error: /The query tried to access \["ci_builds", "projects"\]/, - setup: -> (_) { skip_if_multiple_databases_not_setup } + setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } }, "for query accessing main table from CI database" => { model: Ci::ApplicationRecord, sql: "SELECT 1 FROM projects", expect_error: /The query tried to access \["projects"\]/, - setup: -> (_) { skip_if_multiple_databases_not_setup } + setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } }, "for query accessing CI database" => { model: Ci::ApplicationRecord, @@ -51,13 +51,13 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection model: ::ApplicationRecord, sql: "SELECT 1 FROM ci_builds", expect_error: /The query tried to access \["ci_builds"\]/, - setup: -> (_) { skip_if_multiple_databases_not_setup } + setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } }, "for query accessing unknown gitlab_schema" => { model: ::ApplicationRecord, sql: "SELECT 1 FROM new_table", expect_error: /The query tried to access \["new_table"\] \(of undefined_new_table\)/, - setup: -> (_) { skip_if_multiple_databases_not_setup } + setup: -> (_) { skip_if_multiple_databases_not_setup(:ci) } } } end @@ -77,7 +77,7 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection context "when analyzer is enabled for tests", :query_analyzers do before do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) end it "throws an error when trying to access a table that belongs to the gitlab_main schema from the ci database" do diff --git a/spec/lib/gitlab/database/reflection_spec.rb b/spec/lib/gitlab/database/reflection_spec.rb index 389e93364c8..779bdbe50f0 100644 --- a/spec/lib/gitlab/database/reflection_spec.rb +++ b/spec/lib/gitlab/database/reflection_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reflection do +RSpec.describe Gitlab::Database::Reflection, feature_category: :database do let(:database) { described_class.new(ApplicationRecord) } describe '#username' do diff --git a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb index bf993e85cb8..9482700df5f 100644 --- a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb +++ b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator, feature_category: :dat end let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) } - let(:lease_key) { "gitlab/database/indexing/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/asyncddl/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } let(:lease_timeout) { 1.day } let(:uuid) { 'uuid' } diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index 6575c92e313..a8af9bb5a38 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -68,6 +68,25 @@ RSpec.describe Gitlab::Database::Reindexing, feature_category: :database, time_t end end end + + context 'when async FK validation is enabled' do + it 'executes FK validation for each database prior to any reindexing actions' do + expect(Gitlab::Database::AsyncForeignKeys).to receive(:validate_pending_entries!).ordered.exactly(databases_count).times + expect(described_class).to receive(:automatic_reindexing).ordered.exactly(databases_count).times + + described_class.invoke + end + end + + context 'when async FK validation is disabled' do + it 'does not execute FK validation' do + stub_feature_flags(database_async_foreign_key_validation: false) + + expect(Gitlab::Database::AsyncForeignKeys).not_to receive(:validate_pending_entries!) + + described_class.invoke + end + end end describe '.automatic_reindexing' do diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb index 1edcd890370..2cb84e2f02a 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete do +RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameBase, :delete, feature_category: :subgroups do let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb index c507bce634e..5b5661020b0 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_namespaces_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :delete do +RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameNamespaces, :delete, +feature_category: :subgroups do let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } let(:namespace) { create(:group, name: 'the-path') } diff --git a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb index aa2a3329477..787c9e87038 100644 --- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb +++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :delete do +RSpec.describe Gitlab::Database::RenameReservedPathsMigration::V1::RenameProjects, :delete, +feature_category: :projects do let(:migration) { FakeRenameReservedPathMigrationV1.new } let(:subject) { described_class.new(['the-path'], migration) } let(:project) do diff --git a/spec/lib/gitlab/database/schema_migrations/context_spec.rb b/spec/lib/gitlab/database/schema_migrations/context_spec.rb index 07c97ea0ec3..6a614e2488f 100644 --- a/spec/lib/gitlab/database/schema_migrations/context_spec.rb +++ b/spec/lib/gitlab/database/schema_migrations/context_spec.rb @@ -17,7 +17,7 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do let(:connection_class) { Ci::ApplicationRecord } it 'returns a directory path that is database specific' do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect(context.schema_directory).to eq(File.join(Rails.root, described_class.default_schema_migrations_path)) end @@ -40,7 +40,7 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do end it 'returns a configured directory path that' do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect(context.schema_directory).to eq(File.join(Rails.root, 'db/ci_schema_migrations')) end @@ -52,7 +52,7 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do end it 'returns a configured directory path that' do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) expect(context.schema_directory).to eq(File.join(Rails.root, 'db/ci_schema_migrations')) end diff --git a/spec/lib/gitlab/database/schema_validation/database_spec.rb b/spec/lib/gitlab/database/schema_validation/database_spec.rb new file mode 100644 index 00000000000..c0026f91b46 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/database_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Database, feature_category: :database do + let(:database_name) { 'main' } + let(:database_indexes) do + [['index', 'CREATE UNIQUE INDEX "index" ON public.achievements USING btree (namespace_id, lower(name))']] + end + + let(:query_result) { instance_double('ActiveRecord::Result', rows: database_indexes) } + let(:database_model) { Gitlab::Database.database_base_models[database_name] } + let(:connection) { database_model.connection } + + subject(:database) { described_class.new(connection) } + + before do + allow(connection).to receive(:exec_query).and_return(query_result) + end + + describe '#fetch_index_by_name' do + context 'when index does not exist' do + it 'returns nil' do + index = database.fetch_index_by_name('non_existing_index') + + expect(index).to be_nil + end + end + + it 'returns index by name' do + index = database.fetch_index_by_name('index') + + expect(index.name).to eq('index') + end + end + + describe '#indexes' do + it 'returns indexes' do + indexes = database.indexes + + expect(indexes).to all(be_a(Gitlab::Database::SchemaValidation::Index)) + expect(indexes.map(&:name)).to eq(['index']) + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/index_spec.rb b/spec/lib/gitlab/database/schema_validation/index_spec.rb new file mode 100644 index 00000000000..297211d79ed --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/index_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Index, feature_category: :database do + let(:index_statement) { 'CREATE INDEX index_name ON public.achievements USING btree (namespace_id)' } + + let(:stmt) { PgQuery.parse(index_statement).tree.stmts.first.stmt.index_stmt } + + let(:index) { described_class.new(stmt) } + + describe '#name' do + it 'returns index name' do + expect(index.name).to eq('index_name') + end + end + + describe '#statement' do + it 'returns index statement' do + expect(index.statement).to eq(index_statement) + end + end +end diff --git a/spec/lib/gitlab/database/schema_validation/indexes_spec.rb b/spec/lib/gitlab/database/schema_validation/indexes_spec.rb new file mode 100644 index 00000000000..4351031a4b4 --- /dev/null +++ b/spec/lib/gitlab/database/schema_validation/indexes_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaValidation::Indexes, feature_category: :database do + let(:structure_file_path) { Rails.root.join('spec/fixtures/structure.sql') } + let(:database_indexes) do + [ + ['wrong_index', 'CREATE UNIQUE INDEX wrong_index ON public.table_name (column_name)'], + ['extra_index', 'CREATE INDEX extra_index ON public.table_name (column_name)'], + ['index', 'CREATE UNIQUE INDEX "index" ON public.achievements USING btree (namespace_id, lower(name))'] + ] + end + + let(:database_name) { 'main' } + + let(:database_model) { Gitlab::Database.database_base_models[database_name] } + + let(:connection) { database_model.connection } + + let(:query_result) { instance_double('ActiveRecord::Result', rows: database_indexes) } + + let(:database) { Gitlab::Database::SchemaValidation::Database.new(connection) } + let(:structure_file) { Gitlab::Database::SchemaValidation::StructureSql.new(structure_file_path) } + + subject(:schema_validation) { described_class.new(structure_file, database) } + + before do + allow(connection).to receive(:exec_query).and_return(query_result) + end + + describe '#missing_indexes' do + it 'returns missing indexes' do + missing_indexes = %w[ + missing_index + index_namespaces_public_groups_name_id + index_on_deploy_keys_id_and_type_and_public + index_users_on_public_email_excluding_null_and_empty + ] + + expect(schema_validation.missing_indexes).to match_array(missing_indexes) + end + end + + describe '#extra_indexes' do + it 'returns extra indexes' do + expect(schema_validation.extra_indexes).to match_array(['extra_index']) + end + end + + describe '#wrong_indexes' do + it 'returns wrong indexes' do + expect(schema_validation.wrong_indexes).to match_array(['wrong_index']) + end + end +end diff --git a/spec/lib/gitlab/database/shared_model_spec.rb b/spec/lib/gitlab/database/shared_model_spec.rb index 7e0ba3397d1..2ae6ccf6c6a 100644 --- a/spec/lib/gitlab/database/shared_model_spec.rb +++ b/spec/lib/gitlab/database/shared_model_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::SharedModel do +RSpec.describe Gitlab::Database::SharedModel, feature_category: :database do describe 'using an external connection' do let!(:original_connection) { described_class.connection } let(:new_connection) { double('connection') } @@ -85,28 +85,51 @@ RSpec.describe Gitlab::Database::SharedModel do end end end - - def expect_original_connection_around - # For safety, ensure our original connection is distinct from our double - # This should be the case, but in case of something leaking we should verify - expect(original_connection).not_to be(new_connection) - expect(described_class.connection).to be(original_connection) - - yield - - expect(described_class.connection).to be(original_connection) - end end describe '#connection_db_config' do - it 'returns the class connection_db_config' do - shared_model_class = Class.new(described_class) do + let!(:original_connection) { shared_model_class.connection } + let!(:original_connection_db_config) { shared_model_class.connection_db_config } + let(:shared_model) { shared_model_class.new } + let(:shared_model_class) do + Class.new(described_class) do self.table_name = 'postgres_async_indexes' end + end - shared_model = shared_model_class.new - + it 'returns the class connection_db_config' do expect(shared_model.connection_db_config).to eq(described_class.connection_db_config) end + + context 'when switching the class connection' do + before do + skip_if_multiple_databases_not_setup + end + + let(:new_base_model) { Ci::ApplicationRecord } + let(:new_connection) { new_base_model.connection } + + it 'returns the db_config of the used connection when using load balancing' do + expect_original_connection_around do + described_class.using_connection(new_connection) do + expect(shared_model.connection_db_config).to eq(new_base_model.connection_db_config) + end + end + + # it restores the connection_db_config afterwards + expect(shared_model.connection_db_config).to eq(original_connection_db_config) + end + end + end + + def expect_original_connection_around + # For safety, ensure our original connection is distinct from our double + # This should be the case, but in case of something leaking we should verify + expect(original_connection).not_to be(new_connection) + expect(described_class.connection).to be(original_connection) + + yield + + expect(described_class.connection).to be(original_connection) end end diff --git a/spec/lib/gitlab/database/tables_locker_spec.rb b/spec/lib/gitlab/database/tables_locker_spec.rb new file mode 100644 index 00000000000..d74f455eaad --- /dev/null +++ b/spec/lib/gitlab/database/tables_locker_spec.rb @@ -0,0 +1,226 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::TablesLocker, :reestablished_active_record_base, :delete, :silence_stdout, + :suppress_gitlab_schemas_validate_connection, feature_category: :pods do + let(:detached_partition_table) { '_test_gitlab_main_part_20220101' } + let(:lock_writes_manager) do + instance_double(Gitlab::Database::LockWritesManager, lock_writes: nil, unlock_writes: nil) + end + + before do + allow(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + end + + before(:all) do + create_detached_partition_sql = <<~SQL + CREATE TABLE IF NOT EXISTS gitlab_partitions_dynamic._test_gitlab_main_part_20220101 ( + id bigserial primary key not null + ) + SQL + + ApplicationRecord.connection.execute(create_detached_partition_sql) + Ci::ApplicationRecord.connection.execute(create_detached_partition_sql) + + Gitlab::Database::SharedModel.using_connection(ApplicationRecord.connection) do + Postgresql::DetachedPartition.create!( + table_name: '_test_gitlab_main_part_20220101', + drop_after: Time.current + ) + end + end + + after(:all) do + drop_detached_partition_sql = <<~SQL + DROP TABLE IF EXISTS gitlab_partitions_dynamic._test_gitlab_main_part_20220101 + SQL + + ApplicationRecord.connection.execute(drop_detached_partition_sql) + Ci::ApplicationRecord.connection.execute(drop_detached_partition_sql) + + Gitlab::Database::SharedModel.using_connection(ApplicationRecord.connection) do + Postgresql::DetachedPartition.delete_all + end + end + + shared_examples "lock tables" do |table_schema, database_name| + let(:table_name) do + Gitlab::Database::GitlabSchema + .tables_to_schema.filter_map { |table_name, schema| table_name if schema == table_schema } + .first + end + + let(:database) { database_name } + + it "locks table in schema #{table_schema} and database #{database_name}" do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with( + table_name: table_name, + connection: anything, + database_name: database, + with_retries: true, + logger: anything, + dry_run: anything + ).once.and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:lock_writes) + + subject + end + end + + shared_examples "unlock tables" do |table_schema, database_name| + let(:table_name) do + Gitlab::Database::GitlabSchema + .tables_to_schema.filter_map { |table_name, schema| table_name if schema == table_schema } + .first + end + + let(:database) { database_name } + + it "unlocks table in schema #{table_schema} and database #{database_name}" do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with( + table_name: table_name, + connection: anything, + database_name: database, + with_retries: true, + logger: anything, + dry_run: anything + ).once.and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:unlock_writes) + + subject + end + end + + context 'when running on single database' do + before do + skip_if_multiple_databases_are_setup(:ci) + end + + describe '#lock_writes' do + subject { described_class.new.lock_writes } + + it 'does not call Gitlab::Database::LockWritesManager.lock_writes' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + expect(lock_writes_manager).not_to receive(:lock_writes) + + subject + end + + include_examples "unlock tables", :gitlab_main, 'main' + include_examples "unlock tables", :gitlab_ci, 'ci' + include_examples "unlock tables", :gitlab_shared, 'main' + include_examples "unlock tables", :gitlab_internal, 'main' + end + + describe '#unlock_writes' do + subject { described_class.new.lock_writes } + + it 'does call Gitlab::Database::LockWritesManager.unlock_writes' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:unlock_writes) + + subject + end + end + end + + context 'when running on multiple databases' do + before do + skip_if_multiple_databases_not_setup(:ci) + end + + describe '#lock_writes' do + subject { described_class.new.lock_writes } + + include_examples "lock tables", :gitlab_ci, 'main' + include_examples "lock tables", :gitlab_main, 'ci' + + include_examples "unlock tables", :gitlab_main, 'main' + include_examples "unlock tables", :gitlab_ci, 'ci' + include_examples "unlock tables", :gitlab_shared, 'main' + include_examples "unlock tables", :gitlab_shared, 'ci' + include_examples "unlock tables", :gitlab_internal, 'main' + include_examples "unlock tables", :gitlab_internal, 'ci' + end + + describe '#unlock_writes' do + subject { described_class.new.unlock_writes } + + include_examples "unlock tables", :gitlab_ci, 'main' + include_examples "unlock tables", :gitlab_main, 'ci' + include_examples "unlock tables", :gitlab_main, 'main' + include_examples "unlock tables", :gitlab_ci, 'ci' + include_examples "unlock tables", :gitlab_shared, 'main' + include_examples "unlock tables", :gitlab_shared, 'ci' + include_examples "unlock tables", :gitlab_internal, 'main' + include_examples "unlock tables", :gitlab_internal, 'ci' + end + + context 'when running in dry_run mode' do + subject { described_class.new(dry_run: true).lock_writes } + + it 'passes dry_run flag to LockManger' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with( + table_name: 'users', + connection: anything, + database_name: 'ci', + with_retries: true, + logger: anything, + dry_run: true + ).and_return(lock_writes_manager) + expect(lock_writes_manager).to receive(:lock_writes) + + subject + end + end + + context 'when running on multiple shared databases' do + subject { described_class.new.lock_writes } + + before do + allow(::Gitlab::Database).to receive(:db_config_share_with).and_return(nil) + ci_db_config = Ci::ApplicationRecord.connection_db_config + allow(::Gitlab::Database).to receive(:db_config_share_with).with(ci_db_config).and_return('main') + end + + it 'does not lock any tables if the ci database is shared with main database' do + expect(Gitlab::Database::LockWritesManager).to receive(:new).with(any_args).and_return(lock_writes_manager) + expect(lock_writes_manager).not_to receive(:lock_writes) + + subject + end + end + end + + context 'when geo database is configured' do + let(:geo_table) do + Gitlab::Database::GitlabSchema + .tables_to_schema.filter_map { |table_name, schema| table_name if schema == :gitlab_geo } + .first + end + + subject { described_class.new.unlock_writes } + + before do + skip "Geo database is not configured" unless Gitlab::Database.has_config?(:geo) + end + + it 'does not lock table in geo database' do + expect(Gitlab::Database::LockWritesManager).not_to receive(:new).with( + table_name: geo_table, + connection: anything, + database_name: 'geo', + with_retries: true, + logger: anything, + dry_run: anything + ) + + subject + end + end +end + +def number_of_triggers(connection) + connection.select_value("SELECT count(*) FROM information_schema.triggers") +end diff --git a/spec/lib/gitlab/database/tables_truncate_spec.rb b/spec/lib/gitlab/database/tables_truncate_spec.rb index 9af0b964221..3bb2f4e982c 100644 --- a/spec/lib/gitlab/database/tables_truncate_spec.rb +++ b/spec/lib/gitlab/database/tables_truncate_spec.rb @@ -48,7 +48,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba end before do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) # Creating some test tables on the main database main_tables_sql = <<~SQL @@ -313,7 +313,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba context 'when running with multiple shared databases' do before do - skip_if_multiple_databases_not_setup + skip_if_multiple_databases_not_setup(:ci) ci_db_config = Ci::ApplicationRecord.connection_db_config allow(::Gitlab::Database).to receive(:db_config_share_with).with(ci_db_config).and_return('main') end @@ -333,7 +333,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba context 'when running in a single database mode' do before do - skip_if_multiple_databases_are_setup + skip_if_multiple_databases_are_setup(:ci) end it 'raises an error when truncating the main database that it is a single database setup' do diff --git a/spec/lib/gitlab/database/transaction/observer_spec.rb b/spec/lib/gitlab/database/transaction/observer_spec.rb index 074c18d406e..d1cb014a594 100644 --- a/spec/lib/gitlab/database/transaction/observer_spec.rb +++ b/spec/lib/gitlab/database/transaction/observer_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Transaction::Observer do +RSpec.describe Gitlab::Database::Transaction::Observer, feature_category: :database do # Use the delete DB strategy so that the test won't be wrapped in a transaction describe '.instrument_transactions', :delete do let(:transaction_context) { ActiveRecord::Base.connection.transaction_manager.transaction_context } diff --git a/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb b/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb new file mode 100644 index 00000000000..5b68f9a3757 --- /dev/null +++ b/spec/lib/gitlab/database/transaction_timeout_settings_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::TransactionTimeoutSettings, feature_category: :pods do + let(:connection) { ActiveRecord::Base.connection } + + subject { described_class.new(connection) } + + after(:all) do + described_class.new(ActiveRecord::Base.connection).restore_timeouts + end + + describe '#disable_timeouts' do + it 'sets timeouts to 0' do + subject.disable_timeouts + + expect(current_timeout).to eq("0") + end + end + + describe '#restore_timeouts' do + before do + subject.disable_timeouts + end + + it 'resets value' do + expect(connection).to receive(:execute).with('RESET idle_in_transaction_session_timeout').and_call_original + + subject.restore_timeouts + end + end + + def current_timeout + connection.execute("show idle_in_transaction_session_timeout").first['idle_in_transaction_session_timeout'] + end +end diff --git a/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb b/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb index 836332524a9..9ccae754a92 100644 --- a/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::WithLockRetriesOutsideTransaction do +RSpec.describe Gitlab::Database::WithLockRetriesOutsideTransaction, feature_category: :database do let(:env) { {} } let(:logger) { Gitlab::Database::WithLockRetries::NULL_LOGGER } let(:subject) { described_class.new(connection: connection, env: env, logger: logger, timing_configuration: timing_configuration) } diff --git a/spec/lib/gitlab/database/with_lock_retries_spec.rb b/spec/lib/gitlab/database/with_lock_retries_spec.rb index 797a01c482d..7fe6362634b 100644 --- a/spec/lib/gitlab/database/with_lock_retries_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::WithLockRetries do +RSpec.describe Gitlab::Database::WithLockRetries, feature_category: :database do let(:env) { {} } let(:logger) { Gitlab::Database::WithLockRetries::NULL_LOGGER } let(:subject) { described_class.new(connection: connection, env: env, logger: logger, allow_savepoints: allow_savepoints, timing_configuration: timing_configuration) } |