diff options
Diffstat (limited to 'spec/lib/gitlab/database')
30 files changed, 912 insertions, 260 deletions
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 7ad3eb395a9..207aedd1a38 100644 --- a/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/index_creator_spec.rb @@ -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/async_indexes/index_creator/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/indexing/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } around do |example| @@ -39,7 +39,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes::IndexCreator do it 'creates the index while controlling statement timeout' do allow(connection).to receive(:execute).and_call_original - expect(connection).to receive(:execute).with("SET statement_timeout TO '32400s'").ordered.and_call_original + expect(connection).to receive(:execute).with("SET statement_timeout TO '72000s'").ordered.and_call_original expect(connection).to receive(:execute).with(async_index.definition).ordered.and_call_original expect(connection).to receive(:execute).with("RESET statement_timeout").ordered.and_call_original 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 adb0f45706d..11039ad4f7e 100644 --- a/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb @@ -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/async_indexes/index_destructor/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/indexing/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } before do diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb index 983f482d464..f3a292abbae 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb @@ -152,6 +152,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' it 'runs the job with the correct arguments' do expect(job_class).to receive(:new).with(no_args).and_return(job_instance) + expect(Gitlab::ApplicationContext).to receive(:push).with(feature_category: :database) expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') perform diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb b/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb index db4383a79d4..1c0f5a0c420 100644 --- a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb +++ b/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable do +RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable, + feature_category: :database do include Database::DatabaseHelpers let(:connection) { Gitlab::Database.database_base_models[:main].connection } @@ -17,7 +18,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: subject { described_class.new(context).evaluate } before do - swapout_view_for_table(:postgres_autovacuum_activity) + swapout_view_for_table(:postgres_autovacuum_activity, connection: connection) end let(:tables) { [table] } diff --git a/spec/lib/gitlab/database/consistency_checker_spec.rb b/spec/lib/gitlab/database/consistency_checker_spec.rb index 2ff79d20786..c0f0c349ddd 100644 --- a/spec/lib/gitlab/database/consistency_checker_spec.rb +++ b/spec/lib/gitlab/database/consistency_checker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::ConsistencyChecker do +RSpec.describe Gitlab::Database::ConsistencyChecker, feature_category: :pods do let(:batch_size) { 10 } let(:max_batches) { 4 } let(:max_runtime) { described_class::MAX_RUNTIME } diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb index 4b37cbda047..28a087d5401 100644 --- a/spec/lib/gitlab/database/gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -8,13 +8,40 @@ RSpec.shared_examples 'validate path globs' do |path_globs| end end +RSpec.shared_examples 'validate schema data' do |tables_and_views| + it 'all tables and views have assigned a known gitlab_schema' do + expect(tables_and_views).to all( + match([be_a(String), be_in(Gitlab::Database.schemas_to_base_models.keys.map(&:to_sym))]) + ) + end +end + RSpec.describe Gitlab::Database::GitlabSchema do - describe '.views_and_tables_to_schema' do - it 'all tables and views have assigned a known gitlab_schema' do - expect(described_class.views_and_tables_to_schema).to all( - match([be_a(String), be_in(Gitlab::Database.schemas_to_base_models.keys.map(&:to_sym))]) - ) + shared_examples 'maps table name to table schema' do + using RSpec::Parameterized::TableSyntax + + where(:name, :classification) do + 'ci_builds' | :gitlab_ci + 'my_schema.ci_builds' | :gitlab_ci + 'information_schema.columns' | :gitlab_internal + 'audit_events_part_5fc467ac26' | :gitlab_main + '_test_gitlab_main_table' | :gitlab_main + '_test_gitlab_ci_table' | :gitlab_ci + '_test_my_table' | :gitlab_shared + 'pg_attribute' | :gitlab_internal + end + + with_them do + it { is_expected.to eq(classification) } end + end + + describe '.deleted_views_and_tables_to_schema' do + include_examples 'validate schema data', described_class.deleted_views_and_tables_to_schema + end + + describe '.views_and_tables_to_schema' do + include_examples 'validate schema data', described_class.views_and_tables_to_schema # This being run across different databases indirectly also tests # a general consistency of structure across databases @@ -55,6 +82,14 @@ RSpec.describe Gitlab::Database::GitlabSchema do include_examples 'validate path globs', described_class.view_path_globs end + describe '.deleted_tables_path_globs' do + include_examples 'validate path globs', described_class.deleted_tables_path_globs + end + + describe '.deleted_views_path_globs' do + include_examples 'validate path globs', described_class.deleted_views_path_globs + end + describe '.tables_to_schema' do let(:database_models) { Gitlab::Database.database_base_models.except(:geo) } let(:views) { database_models.flat_map { |_, m| m.connection.views }.sort.uniq } @@ -81,25 +116,85 @@ RSpec.describe Gitlab::Database::GitlabSchema do end end + describe '.table_schemas' do + let(:tables) { %w[users projects ci_builds] } + + subject { described_class.table_schemas(tables) } + + it 'returns the matched schemas' do + expect(subject).to match_array %i[gitlab_main gitlab_ci].to_set + end + + context 'when one of the tables does not have a matching table schema' do + let(:tables) { %w[users projects unknown ci_builds] } + + context 'and undefined parameter is false' do + subject { described_class.table_schemas(tables, undefined: false) } + + it 'includes a nil value' do + is_expected.to match_array [:gitlab_main, nil, :gitlab_ci].to_set + end + end + + context 'and undefined parameter is true' do + subject { described_class.table_schemas(tables, undefined: true) } + + it 'includes "undefined_<table_name>"' do + is_expected.to match_array [:gitlab_main, :undefined_unknown, :gitlab_ci].to_set + end + end + + context 'and undefined parameter is not specified' do + it 'includes a nil value' do + is_expected.to match_array [:gitlab_main, :undefined_unknown, :gitlab_ci].to_set + end + end + end + end + describe '.table_schema' do - using RSpec::Parameterized::TableSyntax + subject { described_class.table_schema(name) } - where(:name, :classification) do - 'ci_builds' | :gitlab_ci - 'my_schema.ci_builds' | :gitlab_ci - 'information_schema.columns' | :gitlab_internal - 'audit_events_part_5fc467ac26' | :gitlab_main - '_test_gitlab_main_table' | :gitlab_main - '_test_gitlab_ci_table' | :gitlab_ci - '_test_my_table' | :gitlab_shared - 'pg_attribute' | :gitlab_internal - 'my_other_table' | :undefined_my_other_table + it_behaves_like 'maps table name to table schema' + + context 'when mapping fails' do + let(:name) { 'unknown_table' } + + context "and parameter 'undefined' is set to true" do + subject { described_class.table_schema(name, undefined: true) } + + it { is_expected.to eq(:undefined_unknown_table) } + end + + context "and parameter 'undefined' is set to false" do + subject { described_class.table_schema(name, undefined: false) } + + it { is_expected.to be_nil } + end + + context "and parameter 'undefined' is not set" do + subject { described_class.table_schema(name) } + + it { is_expected.to eq(:undefined_unknown_table) } + end end + end - with_them do - subject { described_class.table_schema(name) } + describe '.table_schema!' do + subject { described_class.table_schema!(name) } - it { is_expected.to eq(classification) } + it_behaves_like 'maps table name to table schema' + + context 'when mapping fails' do + let(:name) { 'non_existing_table' } + + it "raises error" do + expect { subject }.to raise_error( + Gitlab::Database::GitlabSchema::UnknownSchemaError, + "Could not find gitlab schema for table #{name}: " \ + "Any new tables must be added to the database dictionary" + ) + end end end end diff --git a/spec/lib/gitlab/database/indexing_exclusive_lease_guard_spec.rb b/spec/lib/gitlab/database/indexing_exclusive_lease_guard_spec.rb new file mode 100644 index 00000000000..ddc9cdee92f --- /dev/null +++ b/spec/lib/gitlab/database/indexing_exclusive_lease_guard_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::IndexingExclusiveLeaseGuard, feature_category: :database do + let(:helper_class) do + Class.new do + include Gitlab::Database::IndexingExclusiveLeaseGuard + + attr_reader :connection + + def initialize(connection) + @connection = connection + end + end + end + + describe '#lease_key' do + let(:helper) { helper_class.new(connection) } + let(:lease_key) { "gitlab/database/indexing/actions/#{database_name}" } + + context 'with CI database connection' do + let(:connection) { Ci::ApplicationRecord.connection } + let(:database_name) { Gitlab::Database::CI_DATABASE_NAME } + + before do + skip_if_multiple_databases_not_setup + end + + it { expect(helper.lease_key).to eq(lease_key) } + end + + context 'with MAIN database connection' do + let(:connection) { ApplicationRecord.connection } + let(:database_name) { Gitlab::Database::MAIN_DATABASE_NAME } + + it { expect(helper.lease_key).to eq(lease_key) } + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/resolver_spec.rb b/spec/lib/gitlab/database/load_balancing/resolver_spec.rb index 0051cf50255..4af36693383 100644 --- a/spec/lib/gitlab/database/load_balancing/resolver_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/resolver_spec.rb @@ -2,15 +2,16 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing::Resolver do +RSpec.describe Gitlab::Database::LoadBalancing::Resolver, :freeze_time, feature_category: :database do describe '#resolve' do let(:ip_addr) { IPAddr.new('127.0.0.2') } context 'when nameserver is an IP' do it 'returns an IPAddr object' do service = described_class.new('127.0.0.2') + response = service.resolve - expect(service.resolve).to eq(ip_addr) + expect(response.address).to eq(ip_addr) end end @@ -22,12 +23,14 @@ RSpec.describe Gitlab::Database::LoadBalancing::Resolver do allow(instance).to receive(:getaddress).with('localhost').and_return('127.0.0.2') end - expect(subject).to eq(ip_addr) + expect(subject.address).to eq(ip_addr) end context 'when nameserver is not in the hosts file' do + let(:raw_ttl) { 10 } + it 'looks the nameserver up in DNS' do - resource = double(:resource, address: ip_addr) + resource = double(:resource, address: ip_addr, ttl: raw_ttl) packet = double(:packet, answer: [resource]) allow_next_instance_of(Resolv::Hosts) do |instance| @@ -38,7 +41,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::Resolver do .with('localhost', Net::DNS::A) .and_return(packet) - expect(subject).to eq(ip_addr) + expect(subject.address).to eq(ip_addr) + expect(subject.ttl).to eq(raw_ttl.seconds.from_now) end context 'when nameserver is not in DNS' do diff --git a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb index 984d60e9962..bfd9c644ffa 100644 --- a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do +RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_category: :database do let(:load_balancer) do configuration = Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) configuration.service_discovery[:record] = 'localhost' @@ -23,6 +23,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do resource = double(:resource, address: IPAddr.new('127.0.0.1')) packet = double(:packet, answer: [resource]) + service.instance_variable_set(:@nameserver_ttl, Gitlab::Database::LoadBalancing::Resolver::FAR_FUTURE_TTL) + allow(Net::DNS::Resolver).to receive(:start) .with('localhost', Net::DNS::A) .and_return(packet) @@ -362,4 +364,52 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do expect(service.addresses_from_load_balancer).to eq(addresses) end end + + describe '#resolver', :freeze_time do + context 'without predefined resolver' do + it 'fetches a new resolver and assigns it to the instance variable' do + expect(service.instance_variable_get(:@resolver)).not_to be_present + + service_resolver = service.resolver + + expect(service.instance_variable_get(:@resolver)).to be_present + expect(service_resolver).to be_present + end + end + + context 'with predefined resolver' do + let(:resolver) do + Net::DNS::Resolver.new( + nameservers: 'localhost', + port: 8600 + ) + end + + before do + service.instance_variable_set(:@resolver, resolver) + end + + context "when nameserver's TTL is in the future" do + it 'returns the existing resolver' do + expect(service.resolver).to eq(resolver) + end + end + + context "when nameserver's TTL is in the past" do + before do + service.instance_variable_set( + :@nameserver_ttl, + 1.minute.ago + ) + end + + it 'fetches new resolver' do + service_resolver = service.resolver + + expect(service_resolver).to be_present + expect(service_resolver).not_to eq(resolver) + end + end + end + end end diff --git a/spec/lib/gitlab/database/lock_writes_manager_spec.rb b/spec/lib/gitlab/database/lock_writes_manager_spec.rb index 242b2040eaa..c06c463d918 100644 --- a/spec/lib/gitlab/database/lock_writes_manager_spec.rb +++ b/spec/lib/gitlab/database/lock_writes_manager_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LockWritesManager do +RSpec.describe Gitlab::Database::LockWritesManager, :delete, feature_category: :pods do let(:connection) { ApplicationRecord.connection } let(:test_table) { '_test_table' } let(:logger) { instance_double(Logger) } @@ -13,12 +13,14 @@ RSpec.describe Gitlab::Database::LockWritesManager do table_name: test_table, connection: connection, database_name: 'main', + with_retries: true, logger: logger, dry_run: dry_run ) end before do + allow(connection).to receive(:execute).and_call_original allow(logger).to receive(:info) connection.execute(<<~SQL) @@ -29,20 +31,24 @@ RSpec.describe Gitlab::Database::LockWritesManager do SQL end + after do + ApplicationRecord.connection.execute("DROP TABLE IF EXISTS #{test_table}") + end + describe "#table_locked_for_writes?" do it 'returns false for a table that is not locked for writes' do - expect(subject.table_locked_for_writes?(test_table)).to eq(false) + expect(subject.table_locked_for_writes?).to eq(false) end it 'returns true for a table that is locked for writes' do - expect { subject.lock_writes }.to change { subject.table_locked_for_writes?(test_table) }.from(false).to(true) + expect { subject.lock_writes }.to change { subject.table_locked_for_writes? }.from(false).to(true) end context 'for detached partition tables in another schema' do let(:test_table) { 'gitlab_partitions_dynamic._test_table_20220101' } it 'returns true for a table that is locked for writes' do - expect { subject.lock_writes }.to change { subject.table_locked_for_writes?(test_table) }.from(false).to(true) + expect { subject.lock_writes }.to change { subject.table_locked_for_writes? }.from(false).to(true) end end end @@ -83,21 +89,19 @@ RSpec.describe Gitlab::Database::LockWritesManager do it 'retries again if it receives a statement_timeout a few number of times' do error_message = "PG::QueryCanceled: ERROR: canceling statement due to statement timeout" call_count = 0 - allow(connection).to receive(:execute) do |statement| - if statement.include?("CREATE TRIGGER") - call_count += 1 - raise(ActiveRecord::QueryCanceled, error_message) if call_count.even? - end + expect(connection).to receive(:execute).twice.with(/^CREATE TRIGGER gitlab_schema_write_trigger_for_/) do + call_count += 1 + raise(ActiveRecord::QueryCanceled, error_message) if call_count.odd? end subject.lock_writes + + expect(call_count).to eq(2) # The first call fails, the 2nd call succeeds end it 'raises the exception if it happened many times' do error_message = "PG::QueryCanceled: ERROR: canceling statement due to statement timeout" - allow(connection).to receive(:execute) do |statement| - if statement.include?("CREATE TRIGGER") - raise(ActiveRecord::QueryCanceled, error_message) - end + allow(connection).to receive(:execute).with(/^CREATE TRIGGER gitlab_schema_write_trigger_for_/) do + raise(ActiveRecord::QueryCanceled, error_message) end expect do @@ -152,6 +156,7 @@ RSpec.describe Gitlab::Database::LockWritesManager do table_name: test_table, connection: connection, database_name: 'main', + with_retries: true, logger: logger, dry_run: false ).lock_writes diff --git a/spec/lib/gitlab/database/loose_foreign_keys_spec.rb b/spec/lib/gitlab/database/loose_foreign_keys_spec.rb index ff99f681b0c..3c2d9ca82f2 100644 --- a/spec/lib/gitlab/database/loose_foreign_keys_spec.rb +++ b/spec/lib/gitlab/database/loose_foreign_keys_spec.rb @@ -112,4 +112,31 @@ RSpec.describe Gitlab::Database::LooseForeignKeys do end end end + + describe '.build_definition' do + context 'when child table schema is not defined' do + let(:loose_foreign_keys_yaml) do + { + 'ci_unknown_table' => [ + { + 'table' => 'projects', + 'column' => 'project_id', + 'on_delete' => 'async_delete' + } + ] + } + end + + subject { described_class.definitions } + + before do + described_class.instance_variable_set(:@definitions, nil) + described_class.instance_variable_set(:@loose_foreign_keys_yaml, loose_foreign_keys_yaml) + end + + it 'raises Gitlab::Database::GitlabSchema::UnknownSchemaError error' do + expect { subject }.to raise_error(Gitlab::Database::GitlabSchema::UnknownSchemaError) + end + end + end end 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 9fd49b312eb..089c7a779f2 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 @@ -3,27 +3,39 @@ require 'spec_helper' RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, - :reestablished_active_record_base, query_analyzers: false do + :reestablished_active_record_base, :delete, query_analyzers: false, feature_category: :pods do using RSpec::Parameterized::TableSyntax let(:schema_class) { Class.new(Gitlab::Database::Migration[2.1]) } + let(:skip_automatic_lock_on_writes) { false } let(:gitlab_main_table_name) { :_test_gitlab_main_table } let(:gitlab_ci_table_name) { :_test_gitlab_ci_table } let(:gitlab_geo_table_name) { :_test_gitlab_geo_table } let(:gitlab_shared_table_name) { :_test_table } + let(:renamed_gitlab_main_table_name) { :_test_gitlab_main_new_table } + let(:renamed_gitlab_ci_table_name) { :_test_gitlab_ci_new_table } + before do stub_feature_flags(automatic_lock_writes_on_table: true) reconfigure_db_connection(model: ActiveRecord::Base, config_model: config_model) end + # Drop the created test tables, because we use non-transactional tests + after do + drop_table_if_exists(gitlab_main_table_name) + drop_table_if_exists(gitlab_ci_table_name) + drop_table_if_exists(gitlab_geo_table_name) + drop_table_if_exists(gitlab_shared_table_name) + drop_table_if_exists(renamed_gitlab_main_table_name) + drop_table_if_exists(renamed_gitlab_ci_table_name) + end + shared_examples 'does not lock writes on table' do |config_model| let(:config_model) { config_model } it 'allows deleting records from the table' do - allow_next_instance_of(Gitlab::Database::LockWritesManager) do |instance| - expect(instance).not_to receive(:lock_writes) - end + expect(Gitlab::Database::LockWritesManager).not_to receive(:new) run_migration @@ -37,9 +49,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, let(:config_model) { config_model } it 'errors on deleting' do - allow_next_instance_of(Gitlab::Database::LockWritesManager) do |instance| + expect_next_instance_of(Gitlab::Database::LockWritesManager) do |instance| expect(instance).to receive(:lock_writes).and_call_original end + expect(Gitlab::Database::WithLockRetries).not_to receive(:new) run_migration @@ -49,22 +62,35 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, end end - context 'when executing create_table migrations' do - let(:create_gitlab_main_table_migration_class) { create_table_migration(gitlab_main_table_name) } - let(:create_gitlab_ci_table_migration_class) { create_table_migration(gitlab_ci_table_name) } - let(:create_gitlab_shared_table_migration_class) { create_table_migration(gitlab_shared_table_name) } + shared_examples 'locks writes on table using WithLockRetries' do |config_model| + let(:config_model) { config_model } + + it 'locks the writes on the table using WithLockRetries' do + expect_next_instance_of(Gitlab::Database::WithLockRetries) do |instance| + expect(instance).to receive(:run).and_call_original + end + run_migration + + expect do + migration_class.connection.execute("DELETE FROM #{table_name}") + end.to raise_error(ActiveRecord::StatementInvalid, /is write protected/) + end + end + + context 'when executing create_table migrations' do context 'when single database' do let(:config_model) { Gitlab::Database.database_base_models[:main] } + let(:create_gitlab_main_table_migration_class) { create_table_migration(gitlab_main_table_name) } + let(:create_gitlab_ci_table_migration_class) { create_table_migration(gitlab_ci_table_name) } + let(:create_gitlab_shared_table_migration_class) { create_table_migration(gitlab_shared_table_name) } before do skip_if_multiple_databases_are_setup end it 'does not lock any newly created tables' do - allow_next_instance_of(Gitlab::Database::LockWritesManager) do |instance| - expect(instance).not_to receive(:lock_writes) - end + expect(Gitlab::Database::LockWritesManager).not_to receive(:new) create_gitlab_main_table_migration_class.migrate(:up) create_gitlab_ci_table_migration_class.migrate(:up) @@ -83,9 +109,12 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, skip_if_multiple_databases_not_setup end - let(:skip_automatic_lock_on_writes) { false } let(:migration_class) { create_table_migration(table_name, skip_automatic_lock_on_writes) } - let(:run_migration) { migration_class.migrate(:up) } + let(:run_migration) do + migration_class.connection.transaction do + migration_class.migrate(:up) + end + end context 'for creating a gitlab_main table' do let(:table_name) { gitlab_main_table_name } @@ -95,7 +124,9 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when table listed as a deleted table' do before do - stub_const("Gitlab::Database::GitlabSchema::DELETED_TABLES", { table_name.to_s => :gitlab_main }) + allow(Gitlab::Database::GitlabSchema).to receive(:deleted_tables_to_schema).and_return( + { table_name.to_s => :gitlab_main } + ) end it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:ci] @@ -107,6 +138,14 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:ci] end + context 'when migration does not run within a transaction' do + let(:run_migration) do + migration_class.migrate(:up) + end + + it_behaves_like 'locks writes on table using WithLockRetries', Gitlab::Database.database_base_models[:ci] + end + context 'when the SKIP_AUTOMATIC_LOCK_ON_WRITES feature flag is set' do before do stub_env('SKIP_AUTOMATIC_LOCK_ON_WRITES' => 'true') @@ -132,7 +171,9 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when table listed as a deleted table' do before do - stub_const("Gitlab::Database::GitlabSchema::DELETED_TABLES", { table_name.to_s => :gitlab_ci }) + allow(Gitlab::Database::GitlabSchema).to receive(:deleted_tables_to_schema).and_return( + { table_name.to_s => :gitlab_ci } + ) end it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:main] @@ -202,11 +243,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, end let(:migration_class) { rename_table_migration(old_table_name, table_name) } - let(:run_migration) { migration_class.migrate(:up) } + let(:run_migration) do + migration_class.connection.transaction do + migration_class.migrate(:up) + end + end context 'when a gitlab_main table' do let(:old_table_name) { gitlab_main_table_name } - let(:table_name) { :_test_gitlab_main_new_table } + let(:table_name) { renamed_gitlab_main_table_name } let(:database_base_model) { Gitlab::Database.database_base_models[:main] } it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:main] @@ -215,7 +260,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, context 'when a gitlab_ci table' do let(:old_table_name) { gitlab_ci_table_name } - let(:table_name) { :_test_gitlab_ci_new_table } + let(:table_name) { renamed_gitlab_ci_table_name } let(:database_base_model) { Gitlab::Database.database_base_models[:ci] } it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:ci] @@ -236,9 +281,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, end it 'does not lock any newly created tables' do - allow_next_instance_of(Gitlab::Database::LockWritesManager) do |instance| - expect(instance).not_to receive(:lock_writes) - end + expect(Gitlab::Database::LockWritesManager).not_to receive(:new) drop_gitlab_main_table_migration_class.connection.execute("CREATE TABLE #{gitlab_main_table_name}()") drop_gitlab_ci_table_migration_class.connection.execute("CREATE TABLE #{gitlab_ci_table_name}()") @@ -268,7 +311,11 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, end let(:migration_class) { drop_table_migration(table_name) } - let(:run_migration) { migration_class.migrate(:down) } + let(:run_migration) do + migration_class.connection.transaction do + migration_class.migrate(:down) + end + end context 'for re-creating a gitlab_main table' do let(:table_name) { gitlab_main_table_name } @@ -293,14 +340,14 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, end end - def create_table_migration(table_name, skip_lock_on_writes = false) + def create_table_migration(table_name, skip_automatic_lock_on_writes = false) migration_class = Class.new(schema_class) do class << self; attr_accessor :table_name; end def change create_table self.class.table_name end end - migration_class.skip_automatic_lock_on_writes = skip_lock_on_writes + migration_class.skip_automatic_lock_on_writes = skip_automatic_lock_on_writes migration_class.tap { |klass| klass.table_name = table_name } end @@ -331,4 +378,11 @@ RSpec.describe Gitlab::Database::MigrationHelpers::AutomaticLockWritesOnTables, def geo_configured? !!ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, name: 'geo') end + + # To drop the test tables that have been created in the test migrations + def drop_table_if_exists(table_name) + Gitlab::Database.database_base_models.each_value do |model| + model.connection.execute("DROP TABLE IF EXISTS #{table_name}") + end + end end diff --git a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb index e8045f5afec..714fbab5aff 100644 --- a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_analyzers: false, stub_feature_flags: false do +RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_analyzers: false, + stub_feature_flags: false, feature_category: :pods do let(:schema_class) { Class.new(Gitlab::Database::Migration[1.0]).include(described_class) } # We keep only the GitlabSchemasValidateConnection analyzer running @@ -125,8 +126,9 @@ RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_a "does add index to ci_builds in gitlab_main and gitlab_ci" => { migration: ->(klass) do def change - # Due to running in transactin we cannot use `add_concurrent_index` - add_index :ci_builds, :tag, where: "type = 'Ci::Build'", name: 'index_ci_builds_on_tag_and_type_eq_ci_build' + # Due to running in transaction we cannot use `add_concurrent_index` + index_name = 'index_ci_builds_on_tag_and_type_eq_ci_build' + add_index :ci_builds, :tag, where: "type = 'Ci::Build'", name: index_name end end, query_matcher: /CREATE INDEX/, diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 30eeff31326..12fa115cc4e 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -743,6 +743,75 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end + context 'ON UPDATE statements' do + context 'on_update: :nullify' do + it 'appends ON UPDATE SET NULL statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).to receive(:execute).with(/ON UPDATE SET NULL/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_update: :nullify) + end + end + + context 'on_update: :cascade' do + it 'appends ON UPDATE CASCADE statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).to receive(:execute).with(/ON UPDATE CASCADE/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_update: :cascade) + end + end + + context 'on_update: nil' do + it 'appends no ON UPDATE statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).not_to receive(:execute).with(/ON UPDATE/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + on_update: nil) + end + end + + context 'when on_update is not provided' do + it 'appends no ON UPDATE statement' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + expect(model).not_to receive(:execute).with(/ON UPDATE/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id) + end + end + end + context 'when no custom key name is supplied' do it 'creates a concurrent foreign key and validates it' do expect(model).to receive(:with_lock_retries).and_call_original @@ -760,6 +829,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do name = model.concurrent_foreign_key_name(:projects, :user_id) expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id, + on_update: nil, on_delete: :cascade, name: name, primary_key: :id).and_return(true) @@ -792,6 +862,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:foreign_key_exists?).with(:projects, :users, name: :foo, primary_key: :id, + on_update: nil, on_delete: :cascade, column: :user_id).and_return(true) @@ -861,6 +932,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do "ADD CONSTRAINT fk_multiple_columns\n" \ "FOREIGN KEY \(partition_number, user_id\)\n" \ "REFERENCES users \(partition_number, id\)\n" \ + "ON UPDATE CASCADE\n" \ "ON DELETE CASCADE\n" \ "NOT VALID;\n" ) @@ -871,7 +943,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do column: [:partition_number, :user_id], target_column: [:partition_number, :id], validate: false, - name: :fk_multiple_columns + name: :fk_multiple_columns, + on_update: :cascade ) end @@ -883,6 +956,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do { column: [:partition_number, :user_id], name: :fk_multiple_columns, + on_update: :cascade, on_delete: :cascade, primary_key: [:partition_number, :id] } @@ -898,6 +972,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do :users, column: [:partition_number, :user_id], target_column: [:partition_number, :id], + on_update: :cascade, validate: false, name: :fk_multiple_columns ) @@ -973,58 +1048,58 @@ RSpec.describe Gitlab::Database::MigrationHelpers do describe '#foreign_key_exists?' do before do - key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( - :projects, :users, - { - column: :non_standard_id, - name: :fk_projects_users_non_standard_id, - on_delete: :cascade, - primary_key: :id - } - ) - allow(model).to receive(:foreign_keys).with(:projects).and_return([key]) + model.connection.execute(<<~SQL) + create table referenced ( + id bigserial primary key not null + ); + create table referencing ( + 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 + ); + SQL end shared_examples_for 'foreign key checks' do it 'finds existing foreign keys by column' do - expect(model.foreign_key_exists?(:projects, target_table, column: :non_standard_id)).to be_truthy + expect(model.foreign_key_exists?(:referencing, target_table, column: :non_standard_id)).to be_truthy end it 'finds existing foreign keys by name' do - expect(model.foreign_key_exists?(:projects, target_table, name: :fk_projects_users_non_standard_id)).to be_truthy + expect(model.foreign_key_exists?(:referencing, target_table, name: :fk_referenced)).to be_truthy end it 'finds existing foreign_keys by name and column' do - expect(model.foreign_key_exists?(:projects, target_table, name: :fk_projects_users_non_standard_id, column: :non_standard_id)).to be_truthy + expect(model.foreign_key_exists?(:referencing, 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?(:projects, target_table, name: :fk_projects_users_non_standard_id, column: :non_standard_id, on_delete: :cascade)).to be_truthy + expect(model.foreign_key_exists?(:referencing, 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?(:projects, target_table)).to be_truthy + expect(model.foreign_key_exists?(:referencing, target_table)).to be_truthy end it 'compares by column name if given' do - expect(model.foreign_key_exists?(:projects, target_table, column: :user_id)).to be_falsey + expect(model.foreign_key_exists?(:referencing, target_table, column: :user_id)).to be_falsey end it 'compares by target column name if given' do - expect(model.foreign_key_exists?(:projects, target_table, primary_key: :user_id)).to be_falsey - expect(model.foreign_key_exists?(:projects, target_table, primary_key: :id)).to be_truthy + 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 end it 'compares by foreign key name if given' do - expect(model.foreign_key_exists?(:projects, target_table, name: :non_existent_foreign_key_name)).to be_falsey + expect(model.foreign_key_exists?(:referencing, 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?(:projects, target_table, name: :non_existent_foreign_key_name, column: :non_standard_id)).to be_falsey + expect(model.foreign_key_exists?(:referencing, 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?(:projects, target_table, name: :fk_projects_users_non_standard_id, column: :non_standard_id, on_delete: :nullify)).to be_falsey + expect(model.foreign_key_exists?(:referencing, target_table, name: :fk_referenced, column: :non_standard_id, on_delete: :nullify)).to be_falsey end end @@ -1035,7 +1110,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end context 'specifying a target table' do - let(:target_table) { :users } + let(:target_table) { :referenced } it_behaves_like 'foreign key checks' end @@ -1044,59 +1119,66 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model.foreign_key_exists?(:projects, :other_table)).to be_falsey end + 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) + end + context 'with foreign key using multiple columns' do before do - key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( - :projects, :users, - { - column: [:partition_number, :id], - name: :fk_projects_users_partition_number_id, - on_delete: :cascade, - primary_key: [:partition_number, :id] - } - ) - allow(model).to receive(:foreign_keys).with(:projects).and_return([key]) + 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 + ); + SQL end it 'finds existing foreign keys by columns' do - expect(model.foreign_key_exists?(:projects, :users, column: [:partition_number, :id])).to be_truthy + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, column: [:partition_number, :id])).to be_truthy end it 'finds existing foreign keys by name' do - expect(model.foreign_key_exists?(:projects, :users, name: :fk_projects_users_partition_number_id)).to be_truthy + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :fk_partitioning)).to be_truthy end it 'finds existing foreign_keys by name and column' do - expect(model.foreign_key_exists?(:projects, :users, name: :fk_projects_users_partition_number_id, column: [:partition_number, :id])).to be_truthy + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, 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?(:projects, :users, name: :fk_projects_users_partition_number_id, column: [:partition_number, :id], on_delete: :cascade)).to be_truthy + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, 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?(:projects, :users)).to be_truthy + expect(model.foreign_key_exists?(:p_referencing, :p_referenced)).to be_truthy end it 'compares by column name if given' do - expect(model.foreign_key_exists?(:projects, :users, column: :id)).to be_falsey + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, column: :id)).to be_falsey end it 'compares by target column name if given' do - expect(model.foreign_key_exists?(:projects, :users, primary_key: :user_id)).to be_falsey - expect(model.foreign_key_exists?(:projects, :users, primary_key: [:partition_number, :id])).to be_truthy + 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 end it 'compares by foreign key name if given' do - expect(model.foreign_key_exists?(:projects, :users, name: :non_existent_foreign_key_name)).to be_falsey + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, 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?(:projects, :users, name: :non_existent_foreign_key_name, column: [:partition_number, :id])).to be_falsey + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, 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?(:projects, :users, name: :fk_projects_users_partition_number_id, column: [:partition_number, :id], on_delete: :nullify)).to be_falsey + expect(model.foreign_key_exists?(:p_referencing, :p_referenced, name: :fk_partitioning, column: [:partition_number, :id], on_delete: :nullify)).to be_falsey end end end @@ -1159,7 +1241,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do Gitlab::Database::LockWritesManager.new( table_name: test_table, connection: model.connection, - database_name: 'main' + database_name: 'main', + with_retries: false ) end @@ -1340,7 +1423,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do Gitlab::Database::LockWritesManager.new( table_name: test_table, connection: model.connection, - database_name: 'main' + database_name: 'main', + with_retries: false ) end diff --git a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb index 3540a120b8f..b0bdbf5c371 100644 --- a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb +++ b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Migrations::Instrumentation do + subject(:instrumentation) { described_class.new(result_dir: result_dir) } + let(:result_dir) { Dir.mktmpdir } let(:connection) { ActiveRecord::Migration.connection } @@ -9,17 +11,18 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do FileUtils.rm_rf(result_dir) end describe '#observe' do - subject { described_class.new(result_dir: result_dir) } - def load_observation(result_dir, migration_name) Gitlab::Json.parse(File.read(File.join(result_dir, migration_name, described_class::STATS_FILENAME))) end let(:migration_name) { 'test' } let(:migration_version) { '12345' } + let(:migration_meta) { { 'max_batch_size' => 1, 'total_tuple_count' => 10, 'interval' => 60 } } it 'executes the given block' do - expect { |b| subject.observe(version: migration_version, name: migration_name, connection: connection, &b) }.to yield_control + expect do |b| + instrumentation.observe(version: migration_version, name: migration_name, connection: connection, meta: migration_meta, &b) + end.to yield_control end context 'behavior with observers' do @@ -68,13 +71,17 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do end context 'on successful execution' do - subject { described_class.new(result_dir: result_dir).observe(version: migration_version, name: migration_name, connection: connection) {} } + subject do + instrumentation.observe(version: migration_version, name: migration_name, + connection: connection, meta: migration_meta) {} + end it 'records a valid observation', :aggregate_failures do expect(subject.walltime).not_to be_nil 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 end @@ -82,9 +89,10 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do where(exception: ['something went wrong', SystemStackError, Interrupt]) with_them do - let(:instance) { described_class.new(result_dir: result_dir) } - - subject(:observe) { instance.observe(version: migration_version, name: migration_name, connection: connection) { raise exception } } + subject(:observe) do + instrumentation.observe(version: migration_version, name: migration_name, + connection: connection, meta: migration_meta) { raise exception } + end it 'raises the exception' do expect { observe }.to raise_error(exception) @@ -106,14 +114,13 @@ 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 end end end context 'sequence of migrations with failures' do - subject { described_class.new(result_dir: result_dir) } - let(:migration1) { double('migration1', call: nil) } let(:migration2) { double('migration2', call: nil) } @@ -121,9 +128,9 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do let(:migration_version_2) { '98765' } it 'records observations for all migrations' do - subject.observe(version: migration_version, name: migration_name, connection: connection) {} + instrumentation.observe(version: migration_version, name: migration_name, connection: connection) {} begin - subject.observe(version: migration_version_2, name: migration_name_2, connection: connection) { raise 'something went wrong' } + instrumentation.observe(version: migration_version_2, name: migration_name_2, connection: connection) { raise 'something went wrong' } rescue StandardError nil 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 73d69d55e5a..0b048617ce1 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 @@ -69,12 +69,27 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez end 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 + before do queue_migration('CopyColumnUsingBackgroundMigrationJob', table_name, :id, :id, :data, batch_size: 100, - job_interval: 5.minutes) # job_interval is skipped when testing + job_interval: interval) # job_interval is skipped when testing end subject(:sample_migration) do @@ -91,10 +106,9 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez }.by_at_most(-1) end - it 'uses the correct connection to instrument the background migration' do + it 'uses the correct params to instrument the background migration' do expect_next_instance_of(Gitlab::Database::Migrations::Instrumentation) do |instrumentation| - expect(instrumentation).to receive(:observe).with(hash_including(connection: connection)) - .at_least(:once).and_call_original + expect(instrumentation).to receive(:observe).with(hash_including(params)).at_least(:once).and_call_original end subject diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index db5ca890155..855d0bc46a4 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -10,15 +10,15 @@ RSpec.describe Gitlab::Database::Partitioning do around do |example| previously_registered_models = described_class.registered_models.dup - described_class.instance_variable_set('@registered_models', Set.new) + described_class.instance_variable_set(:@registered_models, Set.new) previously_registered_tables = described_class.registered_tables.dup - described_class.instance_variable_set('@registered_tables', Set.new) + described_class.instance_variable_set(:@registered_tables, Set.new) example.run - described_class.instance_variable_set('@registered_models', previously_registered_models) - described_class.instance_variable_set('@registered_tables', previously_registered_tables) + described_class.instance_variable_set(:@registered_models, previously_registered_models) + described_class.instance_variable_set(:@registered_tables, previously_registered_tables) end describe '.register_models' do diff --git a/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb b/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb index c1ac8f0c9cd..f24c4559349 100644 --- a/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb +++ b/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model do +RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model, feature_category: :database do include Database::DatabaseHelpers it { is_expected.to be_a Gitlab::Database::SharedModel } @@ -13,7 +13,7 @@ RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model do let(:tables) { %w[foo test] } before do - swapout_view_for_table(:postgres_autovacuum_activity) + swapout_view_for_table(:postgres_autovacuum_activity, connection: ApplicationRecord.connection) # unrelated create(:postgres_autovacuum_activity, table: 'bar') diff --git a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb index b0e08ca1e67..a8dbc4be16f 100644 --- a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb +++ b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb @@ -2,28 +2,32 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model do +RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model, feature_category: :database do # PostgresForeignKey does not `behaves_like 'a postgres model'` because it does not correspond 1-1 with a single entry # in pg_class before do - ActiveRecord::Base.connection.execute(<<~SQL) - CREATE TABLE public.referenced_table ( - id bigserial primary key not null - ); - - CREATE TABLE public.other_referenced_table ( - id bigserial primary key not null - ); - - CREATE TABLE public.constrained_table ( - id bigserial primary key not null, - referenced_table_id bigint not null, - other_referenced_table_id bigint not null, - CONSTRAINT fk_constrained_to_referenced FOREIGN KEY(referenced_table_id) REFERENCES referenced_table(id), - CONSTRAINT fk_constrained_to_other_referenced FOREIGN KEY(other_referenced_table_id) - REFERENCES other_referenced_table(id) - ); + ApplicationRecord.connection.execute(<<~SQL) + CREATE TABLE public.referenced_table ( + id bigserial primary key not null, + id_b bigserial not null, + UNIQUE (id, id_b) + ); + + CREATE TABLE public.other_referenced_table ( + id bigserial primary key not null + ); + + CREATE TABLE public.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_other_referenced FOREIGN KEY(other_referenced_table_id) + REFERENCES other_referenced_table(id) + ); + SQL end @@ -39,6 +43,14 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model do end end + describe '#by_referenced_table_name' do + 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) + 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/) @@ -50,4 +62,147 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model do expect(described_class.by_constrained_table_identifier('public.constrained_table')).to match_array(expected) end end + + describe '#by_constrained_table_name' do + 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) + end + end + + describe '#by_name' do + it 'finds foreign keys by name' do + expect(described_class.by_name('fk_constrained_to_referenced').pluck(:name)).to contain_exactly('fk_constrained_to_referenced') + end + end + + context 'when finding columns for foreign keys' do + using RSpec::Parameterized::TableSyntax + + let(:fks) { described_class.by_constrained_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] + lazy { described_class.find_by(name: 'fk_constrained_to_other_referenced') } | %w[other_referenced_table_id] | %w[id] + end + + with_them do + it 'finds the correct constrained column names' do + expect(fk.constrained_columns).to eq(expected_constrained) + end + + it 'finds the correct referenced column names' do + expect(fk.referenced_columns).to eq(expected_referenced) + end + + describe '#by_constrained_columns' do + it 'finds the correct foreign key' do + expect(fks.by_constrained_columns(expected_constrained)).to contain_exactly(fk) + end + end + + describe '#by_referenced_columns' do + it 'finds the correct foreign key' do + expect(fks.by_referenced_columns(expected_referenced)).to contain_exactly(fk) + end + end + end + end + + describe '#on_delete_action' do + before do + ApplicationRecord.connection.execute(<<~SQL) + create table public.referenced_table_all_on_delete_actions ( + id bigserial primary key not null + ); + + create table public.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 + ) + SQL + end + + let(:fks) { described_class.by_constrained_table_name('constrained_table_all_on_delete_actions') } + + context 'with an invalid on_delete_action' do + it 'raises an error' do + # the correct value is :nullify, not :set_null + expect { fks.by_on_delete_action(:set_null) }.to raise_error(ArgumentError) + end + end + + where(:fk_name, :expected_on_delete_action) do + [ + %w[fk_no_action no_action], + %w[fk_restrict restrict], + %w[fk_nullify nullify], + %w[fk_cascade cascade], + %w[fk_set_default set_default] + ] + end + + with_them do + 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) + 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) + end + end + end + end + + context 'when supporting foreign keys to inherited tables in postgres 12' do + before do + 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 public.child partition of 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) + ) + SQL + end + + describe '#is_inherited' do + using RSpec::Parameterized::TableSyntax + + 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.find_by(name: 'fk_constrained_to_referenced') } | false + end + + with_them do + it 'has the appropriate inheritance value' do + expect(fk.is_inherited).to eq(inherited) + end + end + end + + describe '#not_inherited' do + let(:fks) { described_class.by_constrained_table_identifier('public.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') + end + end + end end diff --git a/spec/lib/gitlab/database/query_analyzer_spec.rb b/spec/lib/gitlab/database/query_analyzer_spec.rb index 6dc9ffc4aba..0b849063562 100644 --- a/spec/lib/gitlab/database/query_analyzer_spec.rb +++ b/spec/lib/gitlab/database/query_analyzer_spec.rb @@ -10,7 +10,6 @@ RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do before do allow(described_class.instance).to receive(:all_analyzers).and_return([analyzer, disabled_analyzer]) allow(analyzer).to receive(:enabled?).and_return(true) - allow(analyzer).to receive(:raw?).and_return(false) allow(analyzer).to receive(:suppressed?).and_return(false) allow(analyzer).to receive(:begin!) allow(analyzer).to receive(:end!) @@ -182,13 +181,6 @@ RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error end - it 'does call analyze with raw sql when raw? is true' do - expect(analyzer).to receive(:raw?).and_return(true) - expect(analyzer).to receive(:analyze).with('SELECT 1 FROM projects') - - expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error - end - def process_sql(sql) described_class.instance.within do ApplicationRecord.load_balancer.read_write do |connection| diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb index 62c5ead855a..3a92f35d585 100644 --- a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb @@ -53,6 +53,14 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasMetrics, query_ana gitlab_schemas: "gitlab_ci", db_config_name: "ci" } + }, + "for query accessing gitlab_main and unknown schema" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM projects LEFT JOIN not_in_schema ON not_in_schema.project_id=projects.id", + expectations: { + gitlab_schemas: "gitlab_main,undefined_not_in_schema", + db_config_name: "main" + } } } 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 ddf5793049d..47038bbd138 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 @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection, query_analyzers: false do +RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection, query_analyzers: false, + feature_category: :pods do let(:analyzer) { described_class } # We keep only the GitlabSchemasValidateConnection analyzer running @@ -51,6 +52,12 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection sql: "SELECT 1 FROM ci_builds", expect_error: /The query tried to access \["ci_builds"\]/, setup: -> (_) { skip_if_multiple_databases_not_setup } + }, + "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 } } } end diff --git a/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb b/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb index 22a70dc7df0..a4322689bf9 100644 --- a/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification, query_analyzers: false do +RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification, query_analyzers: false, + feature_category: :pods do let_it_be(:pipeline, refind: true) { create(:ci_pipeline) } let_it_be(:project, refind: true) { create(:project) } diff --git a/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb b/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb index bcc39c0c3db..22ff66ff55e 100644 --- a/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/query_recorder_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::QueryAnalyzers::QueryRecorder, query_analyzers: false do +RSpec.describe Gitlab::Database::QueryAnalyzers::QueryRecorder, feature_category: :database, query_analyzers: false do # We keep only the QueryRecorder analyzer running around do |example| described_class.with_suppressed(false) do @@ -11,7 +11,6 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::QueryRecorder, query_analyzers: end context 'with query analyzer' do - let(:query) { 'SELECT 1 FROM projects' } let(:log_path) { Rails.root.join(described_class::LOG_PATH) } let(:log_file) { described_class.log_file } @@ -20,14 +19,44 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::QueryRecorder, query_analyzers: end shared_examples_for 'an enabled query recorder' do - it 'logs queries to a file' do - allow(FileUtils).to receive(:mkdir_p) - .with(log_path) - expect(File).to receive(:write) - .with(log_file, /^{"sql":"#{query}/, mode: 'a') - expect(described_class).to receive(:analyze).with(/^#{query}/).and_call_original - - expect { ApplicationRecord.connection.execute(query) }.not_to raise_error + using RSpec::Parameterized::TableSyntax + + normalized_query = <<~SQL.strip.tr("\n", ' ') + SELECT \\\\"projects\\\\".\\\\"id\\\\" + FROM \\\\"projects\\\\" + WHERE \\\\"projects\\\\".\\\\"namespace_id\\\\" = \\? + AND \\\\"projects\\\\".\\\\"id\\\\" IN \\(\\?,\\?,\\?\\); + SQL + + where(:list_parameter, :bind_parameters) do + '$2, $3' | [1, 2, 3] + '$2, $3, $4' | [1, 2, 3, 4] + '$2 ,$3 ,$4 ,$5' | [1, 2, 3, 4, 5] + '$2 , $3 , $4 , $5, $6' | [1, 2, 3, 4, 5, 6] + '$2, $3 ,$4 , $5,$6,$7' | [1, 2, 3, 4, 5, 6, 7] + '$2,$3,$4,$5,$6,$7,$8' | [1, 2, 3, 4, 5, 6, 7, 8] + end + + with_them do + before do + allow(described_class).to receive(:analyze).and_call_original + allow(FileUtils).to receive(:mkdir_p) + .with(log_path) + end + + it 'logs normalized queries to a file' do + expect(File).to receive(:write) + .with(log_file, /^{"normalized":"#{normalized_query}/, mode: 'a') + + expect do + ApplicationRecord.connection.exec_query(<<~SQL.strip.tr("\n", ' '), 'SQL', bind_parameters) + SELECT "projects"."id" + FROM "projects" + WHERE "projects"."namespace_id" = $1 + AND "projects"."id" IN (#{list_parameter}); + SQL + end.not_to raise_error + end end end diff --git a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb index bb91617714a..bf993e85cb8 100644 --- a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb +++ b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb @@ -2,16 +2,18 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::Coordinator do +RSpec.describe Gitlab::Database::Reindexing::Coordinator, feature_category: :database do include Database::DatabaseHelpers include ExclusiveLeaseHelpers - let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) } let(:index) { create(:postgres_index) } let(:connection) { index.connection } + let(:notifier) do + instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) + end let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) } - let(:lease_key) { "gitlab/database/reindexing/coordinator/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_key) { "gitlab/database/indexing/actions/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } let(:lease_timeout) { 1.day } let(:uuid) { 'uuid' } @@ -19,75 +21,83 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do model = Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME] Gitlab::Database::SharedModel.using_connection(model.connection) do + swapout_view_for_table(:postgres_indexes, connection: model.connection) example.run end end - before do - swapout_view_for_table(:postgres_indexes) - end - describe '#perform' do subject { described_class.new(index, notifier).perform } let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ReindexConcurrently, perform: nil) } let(:action) { create(:reindex_action, index: index) } - before do - allow(Gitlab::Database::Reindexing::ReindexConcurrently).to receive(:new).with(index).and_return(reindexer) - allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) - end + context 'when executed during the weekend', time_travel_to: '2023-01-07T09:44:07Z' do + before do + allow(Gitlab::Database::Reindexing::ReindexConcurrently).to receive(:new).with(index).and_return(reindexer) + allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) + end - context 'locking' do - it 'acquires a lock while reindexing' do - expect(lease).to receive(:try_obtain).ordered.and_return(uuid) + context 'locking' do + it 'acquires a lock while reindexing' do + expect(lease).to receive(:try_obtain).ordered.and_return(uuid) - expect(reindexer).to receive(:perform).ordered + expect(reindexer).to receive(:perform).ordered - expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) + expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) - subject - end + subject + end - it 'does not perform reindexing actions if lease is not granted' do - expect(lease).to receive(:try_obtain).ordered.and_return(false) - expect(Gitlab::Database::Reindexing::ReindexConcurrently).not_to receive(:new) + it 'does not perform reindexing actions if lease is not granted' do + expect(lease).to receive(:try_obtain).ordered.and_return(false) + expect(Gitlab::Database::Reindexing::ReindexConcurrently).not_to receive(:new) - subject + subject + end end - end - context 'notifications' do - it 'sends #notify_start before reindexing' do - expect(notifier).to receive(:notify_start).with(action).ordered - expect(reindexer).to receive(:perform).ordered + context 'notifications' do + it 'sends #notify_start before reindexing' do + expect(notifier).to receive(:notify_start).with(action).ordered + expect(reindexer).to receive(:perform).ordered - subject - end + subject + end - it 'sends #notify_end after reindexing and updating the action is done' do - expect(action).to receive(:finish).ordered - expect(notifier).to receive(:notify_end).with(action).ordered + it 'sends #notify_end after reindexing and updating the action is done' do + expect(action).to receive(:finish).ordered + expect(notifier).to receive(:notify_end).with(action).ordered - subject + subject + end end - end - context 'action tracking' do - it 'calls #finish on the action' do - expect(reindexer).to receive(:perform).ordered - expect(action).to receive(:finish).ordered + context 'action tracking' do + it 'calls #finish on the action' do + expect(reindexer).to receive(:perform).ordered + expect(action).to receive(:finish).ordered - subject - end + subject + end - it 'upon error, it still calls finish and raises the error' do - expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong') - expect(action).to receive(:finish).ordered + it 'upon error, it still calls finish and raises the error' do + expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong') + expect(action).to receive(:finish).ordered - expect { subject }.to raise_error(/something went wrong/) + expect { subject }.to raise_error(/something went wrong/) - expect(action).to be_failed + expect(action).to be_failed + end + end + end + + context 'when executed during the week', time_travel_to: '2023-01-09T09:44:07Z' do + it 'does not start reindexing' do + expect(lease).not_to receive(:try_obtain) + expect(Gitlab::Database::Reindexing::ReindexConcurrently).not_to receive(:new) + + expect(subject).to be_nil end end end @@ -97,33 +107,45 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do subject(:drop) { described_class.new(index, notifier).drop } - context 'when exclusive lease is granted' do - it 'drops the index with lock retries' do - expect(lease).to receive(:try_obtain).ordered.and_return(uuid) + context 'when executed during the weekend', time_travel_to: '2023-01-07T09:44:07Z' do + context 'when exclusive lease is granted' do + it 'drops the index with lock retries' do + expect(lease).to receive(:try_obtain).ordered.and_return(uuid) + + expect_query("SET lock_timeout TO '60000ms'") + expect_query("DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{index.name}\"") + expect_query("RESET idle_in_transaction_session_timeout; RESET lock_timeout") - expect_query("SET lock_timeout TO '60000ms'") - expect_query("DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{index.name}\"") - expect_query("RESET idle_in_transaction_session_timeout; RESET lock_timeout") + expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) - expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) + drop + end - drop + def expect_query(sql) + expect(connection).to receive(:execute).ordered.with(sql).and_wrap_original do |method, sql| + method.call(sql.sub(/CONCURRENTLY/, '')) + end + end end - def expect_query(sql) - expect(connection).to receive(:execute).ordered.with(sql).and_wrap_original do |method, sql| - method.call(sql.sub(/CONCURRENTLY/, '')) + context 'when exclusive lease is not granted' do + it 'does not drop the index' do + expect(lease).to receive(:try_obtain).ordered.and_return(false) + expect(Gitlab::Database::WithLockRetriesOutsideTransaction).not_to receive(:new) + expect(connection).not_to receive(:execute) + + drop end end end - context 'when exclusive lease is not granted' do - it 'does not drop the index' do - expect(lease).to receive(:try_obtain).ordered.and_return(false) + context 'when executed during the week', time_travel_to: '2023-01-09T09:44:07Z' do + it 'does not start reindexing' do + expect(lease).not_to receive(:try_obtain) expect(Gitlab::Database::WithLockRetriesOutsideTransaction).not_to receive(:new) expect(connection).not_to receive(:execute) - drop + expect(drop).to be_nil end end end diff --git a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb index 1bccdda3be1..e67c97cbf9c 100644 --- a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb +++ b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Gitlab::Database::Reindexing::GrafanaNotifier do let(:action) { create(:reindex_action) } before do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end let(:headers) do @@ -25,7 +25,9 @@ RSpec.describe Gitlab::Database::Reindexing::GrafanaNotifier do let(:response) { double('response', success?: true) } def expect_api_call(payload) - expect(Gitlab::HTTP).to receive(:post).with("#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true).and_return(response) + expect(Gitlab::HTTP).to receive(:post).with( + "#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true + ).and_return(response) end shared_examples_for 'interacting with Grafana annotations API' do @@ -109,7 +111,9 @@ RSpec.describe Gitlab::Database::Reindexing::GrafanaNotifier do end context 'additional tag is provided' do - subject { described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_start(action) } + subject do + described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_start(action) + end let(:payload) do { @@ -163,7 +167,9 @@ RSpec.describe Gitlab::Database::Reindexing::GrafanaNotifier do end context 'additional tag is provided' do - subject { described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_end(action) } + subject do + described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_end(action) + end let(:payload) do { diff --git a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb index 2ae9037959d..e82a2ab467d 100644 --- a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb +++ b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb @@ -2,14 +2,16 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::IndexSelection do +RSpec.describe Gitlab::Database::Reindexing::IndexSelection, feature_category: :database do include Database::DatabaseHelpers subject { described_class.new(Gitlab::Database::PostgresIndex.all).to_a } + let(:connection) { ApplicationRecord.connection } + before do - swapout_view_for_table(:postgres_index_bloat_estimates) - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_index_bloat_estimates, connection: connection) + swapout_view_for_table(:postgres_indexes, connection: connection) create_list(:postgres_index, 10, ondisk_size_bytes: 10.gigabytes).each_with_index do |index, i| create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 2.gigabyte * (i + 1)) @@ -17,7 +19,7 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection do end def execute(sql) - ActiveRecord::Base.connection.execute(sql) + connection.execute(sql) end it 'orders by highest relative bloat first' do @@ -74,4 +76,30 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection do expect(subject.map(&:name).sort).to eq(not_recently_reindexed.map(&:name).sort) end end + + context 'with restricted tables' do + let!(:ci_builds) do + create( + :postgres_index_bloat_estimate, + index: create(:postgres_index, ondisk_size_bytes: 100.gigabytes, tablename: 'ci_builds'), + bloat_size_bytes: 20.gigabyte + ) + end + + context 'when executed on Fridays', time_travel_to: '2022-12-16T09:44:07Z' do + it { expect(subject).not_to include(ci_builds.index) } + end + + context 'when executed on Saturdays', time_travel_to: '2022-12-17T09:44:07Z' do + it { expect(subject).to include(ci_builds.index) } + end + + context 'when executed on Sundays', time_travel_to: '2022-12-18T09:44:07Z' do + it { expect(subject).not_to include(ci_builds.index) } + end + + context 'when executed on Mondays', time_travel_to: '2022-12-19T09:44:07Z' do + it { expect(subject).not_to include(ci_builds.index) } + end + end end diff --git a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb index 1b409924acc..06b89e08737 100644 --- a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb +++ b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb @@ -2,13 +2,13 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::ReindexAction do +RSpec.describe Gitlab::Database::Reindexing::ReindexAction, feature_category: :database do include Database::DatabaseHelpers let(:index) { create(:postgres_index) } before_all do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end it { is_expected.to be_a Gitlab::Database::SharedModel } diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index fa26aa59329..6575c92e313 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing, feature_category: :database do +RSpec.describe Gitlab::Database::Reindexing, feature_category: :database, time_travel_to: '2023-01-07T09:44:07Z' do include ExclusiveLeaseHelpers include Database::DatabaseHelpers @@ -76,7 +76,7 @@ RSpec.describe Gitlab::Database::Reindexing, feature_category: :database do let(:limit) { 5 } before_all do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end before do @@ -147,7 +147,7 @@ RSpec.describe Gitlab::Database::Reindexing, feature_category: :database do subject { described_class.perform_from_queue(maximum_records: limit) } before_all do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end let(:limit) { 2 } diff --git a/spec/lib/gitlab/database/tables_truncate_spec.rb b/spec/lib/gitlab/database/tables_truncate_spec.rb index 4d04bd67a1e..9af0b964221 100644 --- a/spec/lib/gitlab/database/tables_truncate_spec.rb +++ b/spec/lib/gitlab/database/tables_truncate_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_base, - :suppress_gitlab_schemas_validate_connection do + :suppress_gitlab_schemas_validate_connection, feature_category: :pods do include MigrationsHelpers let(:min_batch_size) { 1 } @@ -18,7 +18,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba let(:main_db_shared_item_model) { table("_test_gitlab_shared_items", database: "main") } let(:main_db_partitioned_item) { table("_test_gitlab_hook_logs", database: "main") } let(:main_db_partitioned_item_detached) do - table("gitlab_partitions_dynamic._test_gitlab_hook_logs_20220101", database: "main") + table("gitlab_partitions_dynamic._test_gitlab_hook_logs_202201", database: "main") end # CI Database @@ -29,7 +29,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba let(:ci_db_shared_item_model) { table("_test_gitlab_shared_items", database: "ci") } let(:ci_db_partitioned_item) { table("_test_gitlab_hook_logs", database: "ci") } let(:ci_db_partitioned_item_detached) do - table("gitlab_partitions_dynamic._test_gitlab_hook_logs_20220101", database: "ci") + table("gitlab_partitions_dynamic._test_gitlab_hook_logs_202201", database: "ci") end shared_examples 'truncating legacy tables on a database' do @@ -64,19 +64,19 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba id bigserial not null, created_at timestamptz not null, item_id BIGINT NOT NULL, - primary key (id, created_at), + PRIMARY KEY (id, created_at), CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_main_items(id) ) PARTITION BY RANGE(created_at); - CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_20220101 + CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_202201 PARTITION OF _test_gitlab_hook_logs FOR VALUES FROM ('20220101') TO ('20220131'); - CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_20220201 + CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_202202 PARTITION OF _test_gitlab_hook_logs FOR VALUES FROM ('20220201') TO ('20220228'); - ALTER TABLE _test_gitlab_hook_logs DETACH PARTITION gitlab_partitions_dynamic._test_gitlab_hook_logs_20220101; + ALTER TABLE _test_gitlab_hook_logs DETACH PARTITION gitlab_partitions_dynamic._test_gitlab_hook_logs_202201; SQL main_connection.execute(main_tables_sql) @@ -124,14 +124,14 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba Gitlab::Database::SharedModel.using_connection(main_connection) do Postgresql::DetachedPartition.create!( - table_name: '_test_gitlab_hook_logs_20220101', + table_name: '_test_gitlab_hook_logs_202201', drop_after: Time.current ) end Gitlab::Database::SharedModel.using_connection(ci_connection) do Postgresql::DetachedPartition.create!( - table_name: '_test_gitlab_hook_logs_20220101', + table_name: '_test_gitlab_hook_logs_202201', drop_after: Time.current ) end @@ -176,7 +176,8 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba Gitlab::Database::LockWritesManager.new( table_name: table, connection: connection, - database_name: connection.pool.db_config.name + database_name: connection.pool.db_config.name, + with_retries: false ).lock_writes end end @@ -236,6 +237,25 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba end end + context 'when one of the attached partitions happened to be locked for writes' do + before do + skip if connection.pool.db_config.name != 'ci' + + Gitlab::Database::LockWritesManager.new( + table_name: "#{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_gitlab_hook_logs_202202", + connection: connection, + database_name: connection.pool.db_config.name, + with_retries: false + ).lock_writes + end + + it 'truncates the locked partition successfully' do + expect do + truncate_legacy_tables + end.to change { ci_db_partitioned_item.count }.from(5).to(0) + end + end + context 'with geo configured' do let(:geo_connection) { Gitlab::Database.database_base_models[:geo].connection } |