diff options
Diffstat (limited to 'spec/lib/gitlab/database')
31 files changed, 1425 insertions, 436 deletions
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 ed15951dfb0..eb16a8ccfa5 100644 --- a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb @@ -150,6 +150,23 @@ RSpec.describe Gitlab::Database::AsyncIndexes::MigrationHelpers do migration.prepare_async_index(table_name, 'id') end.not_to change { index_model.where(name: index_name).count } end + + it 'updates definition if changed' do + index = create(:postgres_async_index, table_name: table_name, name: index_name, definition: '...') + + expect do + migration.prepare_async_index(table_name, 'id', name: index_name) + end.to change { index.reload.definition } + end + + it 'does not update definition if not changed' do + definition = "CREATE INDEX CONCURRENTLY \"index_#{table_name}_on_id\" ON \"#{table_name}\" (\"id\")" + index = create(:postgres_async_index, table_name: table_name, name: index_name, definition: definition) + + expect do + migration.prepare_async_index(table_name, 'id', name: index_name) + end.not_to change { index.reload.updated_at } + end end context 'when the async index table does not exist' do 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 3207e97a639..a1c2634f59c 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -234,6 +234,42 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end end + describe '#retry_failed_jobs!' do + let(:batched_migration) { create(:batched_background_migration, status: 'failed') } + + subject(:retry_failed_jobs) { batched_migration.retry_failed_jobs! } + + context 'when there are failed migration jobs' do + let!(:batched_background_migration_job) { create(:batched_background_migration_job, batched_migration: batched_migration, batch_size: 10, min_value: 6, max_value: 15, status: :failed, attempts: 3) } + + before do + allow_next_instance_of(Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchingStrategy) do |batch_class| + allow(batch_class).to receive(:next_batch).with(anything, anything, batch_min_value: 6, batch_size: 5).and_return([6, 10]) + end + end + + it 'moves the status of the migration to active' do + retry_failed_jobs + + expect(batched_migration.status).to eql 'active' + end + + it 'changes the number of attempts to 0' do + retry_failed_jobs + + expect(batched_background_migration_job.reload.attempts).to be_zero + end + end + + context 'when there are no failed migration jobs' do + it 'moves the status of the migration to active' do + retry_failed_jobs + + expect(batched_migration.status).to eql 'active' + end + end + end + describe '#job_class_name=' do it_behaves_like 'an attr_writer that demodulizes assigned class names', :job_class_name end diff --git a/spec/lib/gitlab/database/connection_spec.rb b/spec/lib/gitlab/database/connection_spec.rb index 5e0e6039afc..7f94d7af4a9 100644 --- a/spec/lib/gitlab/database/connection_spec.rb +++ b/spec/lib/gitlab/database/connection_spec.rb @@ -5,29 +5,14 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Connection do let(:connection) { described_class.new } - describe '#default_pool_size' do - before do - allow(Gitlab::Runtime).to receive(:max_threads).and_return(7) - end - - it 'returns the max thread size plus a fixed headroom of 10' do - expect(connection.default_pool_size).to eq(17) - end - - it 'returns the max thread size plus a DB_POOL_HEADROOM if this env var is present' do - stub_env('DB_POOL_HEADROOM', '7') - - expect(connection.default_pool_size).to eq(14) - end - end - describe '#config' do it 'returns a HashWithIndifferentAccess' do expect(connection.config).to be_an_instance_of(HashWithIndifferentAccess) end it 'returns a default pool size' do - expect(connection.config).to include(pool: connection.default_pool_size) + expect(connection.config) + .to include(pool: Gitlab::Database.default_pool_size) end it 'does not cache its results' do @@ -43,7 +28,7 @@ RSpec.describe Gitlab::Database::Connection do it 'returns the default pool size' do expect(connection).to receive(:config).and_return({ pool: nil }) - expect(connection.pool_size).to eq(connection.default_pool_size) + expect(connection.pool_size).to eq(Gitlab::Database.default_pool_size) end end @@ -129,7 +114,7 @@ RSpec.describe Gitlab::Database::Connection do describe '#db_config_with_default_pool_size' do it 'returns db_config with our default pool size' do - allow(connection).to receive(:default_pool_size).and_return(9) + allow(Gitlab::Database).to receive(:default_pool_size).and_return(9) expect(connection.db_config_with_default_pool_size.pool).to eq(9) end @@ -143,7 +128,7 @@ RSpec.describe Gitlab::Database::Connection do describe '#disable_prepared_statements' do around do |example| - original_config = ::Gitlab::Database.main.config + original_config = connection.scope.connection.pool.db_config example.run @@ -162,6 +147,12 @@ RSpec.describe Gitlab::Database::Connection do expect(connection.scope.connection.prepared_statements).to eq(false) end + it 'retains the connection name' do + connection.disable_prepared_statements + + expect(connection.scope.connection_db_config.name).to eq('main') + end + context 'with dynamic connection pool size' do before do connection.scope.establish_connection(connection.config.merge(pool: 7)) @@ -393,34 +384,28 @@ RSpec.describe Gitlab::Database::Connection do end describe '#cached_column_exists?' do - it 'only retrieves data once' do - expect(connection.scope.connection) - .to receive(:columns) - .once.and_call_original - - 2.times do - expect(connection.cached_column_exists?(:projects, :id)).to be_truthy - expect(connection.cached_column_exists?(:projects, :bogus_column)).to be_falsey + it 'only retrieves the data from the schema cache' do + queries = ActiveRecord::QueryRecorder.new do + 2.times do + expect(connection.cached_column_exists?(:projects, :id)).to be_truthy + expect(connection.cached_column_exists?(:projects, :bogus_column)).to be_falsey + end end + + expect(queries.count).to eq(0) end end describe '#cached_table_exists?' do - it 'only retrieves data once per table' do - expect(connection.scope.connection) - .to receive(:data_source_exists?) - .with(:projects) - .once.and_call_original - - expect(connection.scope.connection) - .to receive(:data_source_exists?) - .with(:bogus_table_name) - .once.and_call_original - - 2.times do - expect(connection.cached_table_exists?(:projects)).to be_truthy - expect(connection.cached_table_exists?(:bogus_table_name)).to be_falsey + it 'only retrieves the data from the schema cache' do + queries = ActiveRecord::QueryRecorder.new do + 2.times do + expect(connection.cached_table_exists?(:projects)).to be_truthy + expect(connection.cached_table_exists?(:bogus_table_name)).to be_falsey + end end + + expect(queries.count).to eq(0) end it 'returns false when database does not exist' do @@ -433,16 +418,14 @@ RSpec.describe Gitlab::Database::Connection do end describe '#exists?' do - it 'returns true if `ActiveRecord::Base.connection` succeeds' do - expect(connection.scope).to receive(:connection) - + it 'returns true if the database exists' do expect(connection.exists?).to be(true) end - it 'returns false if `ActiveRecord::Base.connection` fails' do - expect(connection.scope).to receive(:connection) do - raise ActiveRecord::NoDatabaseError, 'broken' - end + it "returns false if the database doesn't exist" do + expect(connection.scope.connection.schema_cache) + .to receive(:database_version) + .and_raise(ActiveRecord::NoDatabaseError) expect(connection.exists?).to be(false) end diff --git a/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb b/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb new file mode 100644 index 00000000000..ebbbafb855f --- /dev/null +++ b/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LoadBalancing::ActionCableCallbacks, :request_store do + describe '.wrapper' do + it 'uses primary and then releases the connection and clears the session' do + expect(Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :release_host) + expect(Gitlab::Database::LoadBalancing::Session).to receive(:clear_session) + + described_class.wrapper.call( + nil, + lambda do + expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).to eq(true) + end + ) + end + + context 'with an exception' do + it 'releases the connection and clears the session' do + expect(Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :release_host) + expect(Gitlab::Database::LoadBalancing::Session).to receive(:clear_session) + + expect do + described_class.wrapper.call(nil, lambda { raise 'test_exception' }) + end.to raise_error('test_exception') + end + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/configuration_spec.rb b/spec/lib/gitlab/database/load_balancing/configuration_spec.rb new file mode 100644 index 00000000000..6621e6276a5 --- /dev/null +++ b/spec/lib/gitlab/database/load_balancing/configuration_spec.rb @@ -0,0 +1,175 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LoadBalancing::Configuration do + let(:model) do + config = ActiveRecord::DatabaseConfigurations::HashConfig + .new('main', 'test', configuration_hash) + + double(:model, connection_db_config: config) + end + + describe '.for_model' do + context 'when load balancing is not configured' do + let(:configuration_hash) { {} } + + it 'uses the default settings' do + config = described_class.for_model(model) + + expect(config.hosts).to eq([]) + expect(config.max_replication_difference).to eq(8.megabytes) + expect(config.max_replication_lag_time).to eq(60.0) + expect(config.replica_check_interval).to eq(60.0) + expect(config.service_discovery).to eq( + nameserver: 'localhost', + port: 8600, + record: nil, + record_type: 'A', + interval: 60, + disconnect_timeout: 120, + use_tcp: false + ) + expect(config.pool_size).to eq(Gitlab::Database.default_pool_size) + end + end + + context 'when load balancing is configured' do + let(:configuration_hash) do + { + pool: 4, + load_balancing: { + max_replication_difference: 1, + max_replication_lag_time: 2, + replica_check_interval: 3, + hosts: %w[foo bar], + discover: { + 'record' => 'foo.example.com' + } + } + } + end + + it 'uses the custom configuration settings' do + config = described_class.for_model(model) + + expect(config.hosts).to eq(%w[foo bar]) + expect(config.max_replication_difference).to eq(1) + expect(config.max_replication_lag_time).to eq(2.0) + expect(config.replica_check_interval).to eq(3.0) + expect(config.service_discovery).to eq( + nameserver: 'localhost', + port: 8600, + record: 'foo.example.com', + record_type: 'A', + interval: 60, + disconnect_timeout: 120, + use_tcp: false + ) + expect(config.pool_size).to eq(4) + end + end + + context 'when the load balancing configuration uses strings as the keys' do + let(:configuration_hash) do + { + pool: 4, + load_balancing: { + 'max_replication_difference' => 1, + 'max_replication_lag_time' => 2, + 'replica_check_interval' => 3, + 'hosts' => %w[foo bar], + 'discover' => { + 'record' => 'foo.example.com' + } + } + } + end + + it 'uses the custom configuration settings' do + config = described_class.for_model(model) + + expect(config.hosts).to eq(%w[foo bar]) + expect(config.max_replication_difference).to eq(1) + expect(config.max_replication_lag_time).to eq(2.0) + expect(config.replica_check_interval).to eq(3.0) + expect(config.service_discovery).to eq( + nameserver: 'localhost', + port: 8600, + record: 'foo.example.com', + record_type: 'A', + interval: 60, + disconnect_timeout: 120, + use_tcp: false + ) + expect(config.pool_size).to eq(4) + end + end + end + + describe '#load_balancing_enabled?' do + it 'returns true when hosts are configured' do + config = described_class.new(ActiveRecord::Base, %w[foo bar]) + + expect(config.load_balancing_enabled?).to eq(true) + end + + it 'returns true when a service discovery record is configured' do + config = described_class.new(ActiveRecord::Base) + config.service_discovery[:record] = 'foo' + + expect(config.load_balancing_enabled?).to eq(true) + end + + it 'returns false when no hosts are configured and service discovery is disabled' do + config = described_class.new(ActiveRecord::Base) + + expect(config.load_balancing_enabled?).to eq(false) + end + end + + describe '#service_discovery_enabled?' do + it 'returns true when a record is configured' do + config = described_class.new(ActiveRecord::Base) + config.service_discovery[:record] = 'foo' + + expect(config.service_discovery_enabled?).to eq(true) + end + + it 'returns false when no record is configured' do + config = described_class.new(ActiveRecord::Base) + + expect(config.service_discovery_enabled?).to eq(false) + end + end + + describe '#pool_size' do + context 'when a custom pool size is used' do + let(:configuration_hash) { { pool: 4 } } + + it 'always reads the value from the model configuration' do + config = described_class.new(model) + + expect(config.pool_size).to eq(4) + + # We can't modify `configuration_hash` as it's only used to populate the + # internal hash used by ActiveRecord; instead of it being used as-is. + allow(model.connection_db_config) + .to receive(:configuration_hash) + .and_return({ pool: 42 }) + + expect(config.pool_size).to eq(42) + end + end + + context 'when the pool size is nil' do + let(:configuration_hash) { {} } + + it 'returns the default pool size' do + config = described_class.new(model) + + expect(config.pool_size).to eq(Gitlab::Database.default_pool_size) + end + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb b/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb index 0ca99ec9acf..ba2f9485066 100644 --- a/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb @@ -3,7 +3,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do - let(:proxy) { described_class.new } + let(:proxy) do + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base) + + described_class.new(Gitlab::Database::LoadBalancing::LoadBalancer.new(config)) + end describe '#select' do it 'performs a read' do @@ -35,9 +40,15 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do describe 'using a SELECT FOR UPDATE query' do it 'runs the query on the primary and sticks to it' do arel = double(:arel, locked: true) + session = Gitlab::Database::LoadBalancing::Session.new + + allow(Gitlab::Database::LoadBalancing::Session).to receive(:current) + .and_return(session) + + expect(session).to receive(:write!) expect(proxy).to receive(:write_using_load_balancer) - .with(:select_all, arel, 'foo', [], sticky: true) + .with(:select_all, arel, 'foo', []) proxy.select_all(arel, 'foo') end @@ -58,8 +69,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do Gitlab::Database::LoadBalancing::ConnectionProxy::STICKY_WRITES.each do |name| describe "#{name}" do it 'runs the query on the primary and sticks to it' do - expect(proxy).to receive(:write_using_load_balancer) - .with(name, 'foo', sticky: true) + session = Gitlab::Database::LoadBalancing::Session.new + + allow(Gitlab::Database::LoadBalancing::Session).to receive(:current) + .and_return(session) + + expect(session).to receive(:write!) + expect(proxy).to receive(:write_using_load_balancer).with(name, 'foo') proxy.send(name, 'foo') end @@ -108,7 +124,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do # We have an extra test for #transaction here to make sure that nested queries # are also sent to a primary. describe '#transaction' do - let(:session) { double(:session) } + let(:session) { Gitlab::Database::LoadBalancing::Session.new } before do allow(Gitlab::Database::LoadBalancing::Session).to receive(:current) @@ -192,7 +208,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do proxy.foo('foo') end - it 'properly forwards trailing hash arguments' do + it 'properly forwards keyword arguments' do allow(proxy.load_balancer).to receive(:read_write) expect(proxy).to receive(:write_using_load_balancer).and_call_original @@ -217,7 +233,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do proxy.foo('foo') end - it 'properly forwards trailing hash arguments' do + it 'properly forwards keyword arguments' do allow(proxy.load_balancer).to receive(:read) expect(proxy).to receive(:read_using_load_balancer).and_call_original @@ -297,20 +313,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do .and_return(session) end - it 'uses but does not stick to the primary when sticking is disabled' do + it 'uses but does not stick to the primary' do expect(proxy.load_balancer).to receive(:read_write).and_yield(connection) expect(connection).to receive(:foo).with('foo') expect(session).not_to receive(:write!) proxy.write_using_load_balancer(:foo, 'foo') end - - it 'sticks to the primary when sticking is enabled' do - expect(proxy.load_balancer).to receive(:read_write).and_yield(connection) - expect(connection).to receive(:foo).with('foo') - expect(session).to receive(:write!) - - proxy.write_using_load_balancer(:foo, 'foo', sticky: true) - end end end diff --git a/spec/lib/gitlab/database/load_balancing/host_list_spec.rb b/spec/lib/gitlab/database/load_balancing/host_list_spec.rb index ad4ca18d5e6..9bb8116c434 100644 --- a/spec/lib/gitlab/database/load_balancing/host_list_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_list_spec.rb @@ -4,7 +4,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::HostList do let(:db_host) { ActiveRecord::Base.connection_pool.db_config.host } - let(:load_balancer) { double(:load_balancer) } + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) + ) + end + let(:host_count) { 2 } let(:hosts) { Array.new(host_count) { Gitlab::Database::LoadBalancing::Host.new(db_host, load_balancer, port: 5432) } } let(:host_list) { described_class.new(hosts) } diff --git a/spec/lib/gitlab/database/load_balancing/host_spec.rb b/spec/lib/gitlab/database/load_balancing/host_spec.rb index f42ac8be1bb..e2011692228 100644 --- a/spec/lib/gitlab/database/load_balancing/host_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::Host do - let(:load_balancer) { Gitlab::Database::LoadBalancing::LoadBalancer.new } + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer + .new(Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base)) + end let(:host) do Gitlab::Database::LoadBalancing::Host.new('localhost', load_balancer) @@ -274,7 +277,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do end it 'returns false when the data is not recent enough' do - diff = Gitlab::Database::LoadBalancing.max_replication_difference * 2 + diff = load_balancer.configuration.max_replication_difference * 2 expect(host) .to receive(:query_and_release) diff --git a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb index c647f5a8f5d..86fae14b961 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -5,7 +5,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do let(:conflict_error) { Class.new(RuntimeError) } let(:db_host) { ActiveRecord::Base.connection_pool.db_config.host } - let(:lb) { described_class.new([db_host, db_host]) } + let(:config) do + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [db_host, db_host]) + end + + let(:lb) { described_class.new(config) } let(:request_cache) { lb.send(:request_cache) } before do @@ -41,6 +46,19 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do top_error end + describe '#initialize' do + it 'ignores the hosts when the primary_only option is enabled' do + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [db_host]) + lb = described_class.new(config, primary_only: true) + hosts = lb.host_list.hosts + + expect(hosts.length).to eq(1) + expect(hosts.first) + .to be_instance_of(Gitlab::Database::LoadBalancing::PrimaryHost) + end + end + describe '#read' do it 'yields a connection for a read' do connection = double(:connection) @@ -121,6 +139,19 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do expect { |b| lb.read(&b) } .to yield_with_args(ActiveRecord::Base.retrieve_connection) end + + it 'uses the primary when the primary_only option is enabled' do + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base) + lb = described_class.new(config, primary_only: true) + + # When no hosts are configured, we don't want to produce any warnings, as + # they aren't useful/too noisy. + expect(Gitlab::Database::LoadBalancing::Logger).not_to receive(:warn) + + expect { |b| lb.read(&b) } + .to yield_with_args(ActiveRecord::Base.retrieve_connection) + end end describe '#read_write' do @@ -152,8 +183,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end it 'does not create conflicts with other load balancers when caching hosts' do - lb1 = described_class.new([db_host, db_host], ActiveRecord::Base) - lb2 = described_class.new([db_host, db_host], Ci::CiDatabaseRecord) + ci_config = Gitlab::Database::LoadBalancing::Configuration + .new(Ci::CiDatabaseRecord, [db_host, db_host]) + + lb1 = described_class.new(config) + lb2 = described_class.new(ci_config) host1 = lb1.host host2 = lb2.host @@ -283,6 +317,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do expect(lb.connection_error?(error)).to eq(false) end + + it 'returns false for ActiveRecord errors without a cause' do + error = ActiveRecord::RecordNotUnique.new + + expect(lb.connection_error?(error)).to eq(false) + end end describe '#serialization_failure?' do diff --git a/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb b/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb new file mode 100644 index 00000000000..a0e63a7ee4e --- /dev/null +++ b/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LoadBalancing::PrimaryHost do + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) + ) + end + + let(:host) { Gitlab::Database::LoadBalancing::PrimaryHost.new(load_balancer) } + + describe '#connection' do + it 'returns a connection from the pool' do + expect(load_balancer.pool).to receive(:connection) + + host.connection + end + end + + describe '#release_connection' do + it 'does nothing' do + expect(host.release_connection).to be_nil + end + end + + describe '#enable_query_cache!' do + it 'does nothing' do + expect(host.enable_query_cache!).to be_nil + end + end + + describe '#disable_query_cache!' do + it 'does nothing' do + expect(host.disable_query_cache!).to be_nil + end + end + + describe '#query_cache_enabled' do + it 'delegates to the primary connection pool' do + expect(host.query_cache_enabled) + .to eq(load_balancer.pool.query_cache_enabled) + end + end + + describe '#disconnect!' do + it 'does nothing' do + expect(host.disconnect!).to be_nil + end + end + + describe '#offline!' do + it 'does nothing' do + expect(host.offline!).to be_nil + end + end + + describe '#online?' do + it 'returns true' do + expect(host.online?).to eq(true) + end + end + + describe '#primary_write_location' do + it 'returns the write location of the primary' do + expect(host.primary_write_location).to be_an_instance_of(String) + expect(host.primary_write_location).not_to be_empty + end + end + + describe '#caught_up?' do + it 'returns true' do + expect(host.caught_up?('foo')).to eq(true) + end + end + + describe '#database_replica_location' do + let(:connection) { double(:connection) } + + it 'returns the write ahead location of the replica', :aggregate_failures do + expect(host) + .to receive(:query_and_release) + .and_return({ 'location' => '0/D525E3A8' }) + + expect(host.database_replica_location).to be_an_instance_of(String) + end + + it 'returns nil when the database query returned no rows' do + expect(host).to receive(:query_and_release).and_return({}) + + expect(host.database_replica_location).to be_nil + end + + it 'returns nil when the database connection fails' do + allow(host).to receive(:connection).and_raise(PG::Error) + + expect(host.database_replica_location).to be_nil + end + end + + describe '#query_and_release' do + it 'executes a SQL query' do + results = host.query_and_release('SELECT 10 AS number') + + expect(results).to be_an_instance_of(Hash) + expect(results['number'].to_i).to eq(10) + end + + it 'releases the connection after running the query' do + expect(host) + .to receive(:release_connection) + .once + + host.query_and_release('SELECT 10 AS number') + end + + it 'returns an empty Hash in the event of an error' do + expect(host.connection) + .to receive(:select_all) + .and_raise(RuntimeError, 'kittens') + + expect(host.query_and_release('SELECT 10 AS number')).to eq({}) + end + end +end 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 a27341a3324..e9bc465b1c7 100644 --- a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb @@ -3,13 +3,18 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do - let(:load_balancer) { Gitlab::Database::LoadBalancing::LoadBalancer.new([]) } + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) + ) + end + let(:service) do described_class.new( + load_balancer, nameserver: 'localhost', port: 8600, - record: 'foo', - load_balancer: load_balancer + record: 'foo' ) end @@ -26,11 +31,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do describe ':record_type' do subject do described_class.new( + load_balancer, nameserver: 'localhost', port: 8600, record: 'foo', - record_type: record_type, - load_balancer: load_balancer + record_type: record_type ) end @@ -69,18 +74,69 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do end describe '#perform_service_discovery' do - it 'reports exceptions to Sentry' do - error = StandardError.new + context 'without any failures' do + it 'runs once' do + expect(service) + .to receive(:refresh_if_necessary).once + + expect(service).not_to receive(:sleep) + + expect(Gitlab::ErrorTracking).not_to receive(:track_exception) + + service.perform_service_discovery + end + end + context 'with failures' do + before do + allow(Gitlab::ErrorTracking).to receive(:track_exception) + allow(service).to receive(:sleep) + end + + let(:valid_retry_sleep_duration) { satisfy { |val| described_class::RETRY_DELAY_RANGE.include?(val) } } + + it 'retries service discovery when under the retry limit' do + error = StandardError.new + + expect(service) + .to receive(:refresh_if_necessary) + .and_raise(error).exactly(described_class::MAX_DISCOVERY_RETRIES - 1).times.ordered + + expect(service) + .to receive(:sleep).with(valid_retry_sleep_duration) + .exactly(described_class::MAX_DISCOVERY_RETRIES - 1).times + + expect(service).to receive(:refresh_if_necessary).and_return(45).ordered + + expect(service.perform_service_discovery).to eq(45) + end + + it 'does not retry service discovery after exceeding the limit' do + error = StandardError.new + + expect(service) + .to receive(:refresh_if_necessary) + .and_raise(error).exactly(described_class::MAX_DISCOVERY_RETRIES).times + + expect(service) + .to receive(:sleep).with(valid_retry_sleep_duration) + .exactly(described_class::MAX_DISCOVERY_RETRIES).times + + service.perform_service_discovery + end - expect(service) - .to receive(:refresh_if_necessary) - .and_raise(error) + it 'reports exceptions to Sentry' do + error = StandardError.new + + expect(service) + .to receive(:refresh_if_necessary) + .and_raise(error).exactly(described_class::MAX_DISCOVERY_RETRIES).times - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with(error) + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .with(error).exactly(described_class::MAX_DISCOVERY_RETRIES).times - service.perform_service_discovery + service.perform_service_discovery + end end end @@ -133,7 +189,10 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do let(:address_bar) { described_class::Address.new('bar') } let(:load_balancer) do - Gitlab::Database::LoadBalancing::LoadBalancer.new([address_foo]) + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [address_foo]) + ) end before do @@ -166,11 +225,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do describe '#addresses_from_dns' do let(:service) do described_class.new( + load_balancer, nameserver: 'localhost', port: 8600, record: 'foo', - record_type: record_type, - load_balancer: load_balancer + record_type: record_type ) end @@ -224,6 +283,16 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do expect(service.addresses_from_dns).to eq([90, addresses]) end end + + context 'when the resolver returns an empty response' do + let(:packet) { double(:packet, answer: []) } + + let(:record_type) { 'A' } + + it 'raises EmptyDnsResponse' do + expect { service.addresses_from_dns }.to raise_error(Gitlab::Database::LoadBalancing::ServiceDiscovery::EmptyDnsResponse) + end + end end describe '#new_wait_time_for' do @@ -246,7 +315,10 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do describe '#addresses_from_load_balancer' do let(:load_balancer) do - Gitlab::Database::LoadBalancing::LoadBalancer.new(%w[b a]) + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, %w[b a]) + ) end it 'returns the ordered host names of the load balancer' do diff --git a/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb index 54050a87af0..f683ade978a 100644 --- a/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb @@ -58,8 +58,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do it 'does not pass database locations', :aggregate_failures do run_middleware - expect(job['database_replica_location']).to be_nil - expect(job['database_write_location']).to be_nil + expect(job['wal_locations']).to be_nil end include_examples 'job data consistency' @@ -86,11 +85,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'passes database_replica_location' do + expected_location = { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } + expect(load_balancer).to receive_message_chain(:host, "database_replica_location").and_return(location) run_middleware - expect(job['database_replica_location']).to eq(location) + expect(job['wal_locations']).to eq(expected_location) end include_examples 'job data consistency' @@ -102,40 +103,56 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'passes primary write location', :aggregate_failures do + expected_location = { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } + expect(load_balancer).to receive(:primary_write_location).and_return(location) run_middleware - expect(job['database_write_location']).to eq(location) + expect(job['wal_locations']).to eq(expected_location) end include_examples 'job data consistency' end end - shared_examples_for 'database location was already provided' do |provided_database_location, other_location| - shared_examples_for 'does not set database location again' do |use_primary| - before do - allow(Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary?).and_return(use_primary) - end + context 'when worker cannot be constantized' do + let(:worker_class) { 'ActionMailer::MailDeliveryJob' } + let(:expected_consistency) { :always } - it 'does not set database locations again' do - run_middleware + include_examples 'does not pass database locations' + end - expect(job[provided_database_location]).to eq(old_location) - expect(job[other_location]).to be_nil - end - end + context 'when worker class does not include ApplicationWorker' do + let(:worker_class) { ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper } + let(:expected_consistency) { :always } + + include_examples 'does not pass database locations' + end + context 'database wal location was already provided' do let(:old_location) { '0/D525E3A8' } let(:new_location) { 'AB/12345' } - let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", provided_database_location => old_location } } + let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => old_location } } + let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'wal_locations' => wal_locations } } before do allow(load_balancer).to receive(:primary_write_location).and_return(new_location) allow(load_balancer).to receive(:database_replica_location).and_return(new_location) end + shared_examples_for 'does not set database location again' do |use_primary| + before do + allow(Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary?).and_return(use_primary) + end + + it 'does not set database locations again' do + run_middleware + + expect(job['wal_locations']).to eq(wal_locations) + end + end + context "when write was performed" do include_examples 'does not set database location again', true end @@ -145,28 +162,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end end - context 'when worker cannot be constantized' do - let(:worker_class) { 'ActionMailer::MailDeliveryJob' } - let(:expected_consistency) { :always } - - include_examples 'does not pass database locations' - end - - context 'when worker class does not include ApplicationWorker' do - let(:worker_class) { ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper } - let(:expected_consistency) { :always } - - include_examples 'does not pass database locations' - end - - context 'database write location was already provided' do - include_examples 'database location was already provided', 'database_write_location', 'database_replica_location' - end - - context 'database replica location was already provided' do - include_examples 'database location was already provided', 'database_replica_location', 'database_write_location' - end - context 'when worker data consistency is :always' do include_context 'data consistency worker class', :always, :load_balancing_for_test_data_consistency_worker diff --git a/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb index 14f240cd159..9f23eb0094f 100644 --- a/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb @@ -62,9 +62,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do include_examples 'load balancing strategy', expected_strategy end - shared_examples_for 'replica is up to date' do |location, expected_strategy| + shared_examples_for 'replica is up to date' do |expected_strategy| + let(:location) {'0/D525E3A8' } + let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } } + it 'does not stick to the primary', :aggregate_failures do - expect(middleware).to receive(:replica_caught_up?).with(location).and_return(true) + expect(load_balancer).to receive(:select_up_to_date_host).with(location).and_return(true) run_middleware do expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).not_to be_truthy @@ -85,30 +88,40 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do include_examples 'stick to the primary', 'primary' end - context 'when database replica location is set' do - let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'database_replica_location' => '0/D525E3A8' } } + context 'when database wal location is set' do + let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'wal_locations' => wal_locations } } + + before do + allow(load_balancer).to receive(:select_up_to_date_host).with(location).and_return(true) + end + + it_behaves_like 'replica is up to date', 'replica' + end + + context 'when deduplication wal location is set' do + let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'dedup_wal_locations' => wal_locations } } before do - allow(middleware).to receive(:replica_caught_up?).and_return(true) + allow(load_balancer).to receive(:select_up_to_date_host).with(wal_locations[:main]).and_return(true) end - it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica' + it_behaves_like 'replica is up to date', 'replica' end - context 'when database primary location is set' do + context 'when legacy wal location is set' do let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'database_write_location' => '0/D525E3A8' } } before do - allow(middleware).to receive(:replica_caught_up?).and_return(true) + allow(load_balancer).to receive(:select_up_to_date_host).with('0/D525E3A8').and_return(true) end - it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica' + it_behaves_like 'replica is up to date', 'replica' end context 'when database location is not set' do let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e' } } - it_behaves_like 'stick to the primary', 'primary_no_wal' + include_examples 'stick to the primary', 'primary_no_wal' end end @@ -167,7 +180,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do replication_lag!(false) end - it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica_retried' + include_examples 'replica is up to date', 'replica_retried' end end end @@ -178,7 +191,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do context 'when replica is not up to date' do before do - allow(middleware).to receive(:replica_caught_up?).and_return(false) + allow(load_balancer).to receive(:select_up_to_date_host).and_return(false) end include_examples 'stick to the primary', 'primary' diff --git a/spec/lib/gitlab/database/load_balancing_spec.rb b/spec/lib/gitlab/database/load_balancing_spec.rb index 6ec8e0516f6..f40ad444081 100644 --- a/spec/lib/gitlab/database/load_balancing_spec.rb +++ b/spec/lib/gitlab/database/load_balancing_spec.rb @@ -40,106 +40,25 @@ RSpec.describe Gitlab::Database::LoadBalancing do end describe '.configuration' do - it 'returns a Hash' do - lb_config = { 'hosts' => %w(foo) } + it 'returns the configuration for the load balancer' do + raw = ActiveRecord::Base.connection_db_config.configuration_hash + cfg = described_class.configuration - original_db_config = Gitlab::Database.main.config - modified_db_config = original_db_config.merge(load_balancing: lb_config) - expect(Gitlab::Database.main).to receive(:config).and_return(modified_db_config) - - expect(described_class.configuration).to eq(lb_config) - end - end - - describe '.max_replication_difference' do - context 'without an explicitly configured value' do - it 'returns the default value' do - allow(described_class) - .to receive(:configuration) - .and_return({}) - - expect(described_class.max_replication_difference).to eq(8.megabytes) - end - end - - context 'with an explicitly configured value' do - it 'returns the configured value' do - allow(described_class) - .to receive(:configuration) - .and_return({ 'max_replication_difference' => 4 }) - - expect(described_class.max_replication_difference).to eq(4) - end - end - end - - describe '.max_replication_lag_time' do - context 'without an explicitly configured value' do - it 'returns the default value' do - allow(described_class) - .to receive(:configuration) - .and_return({}) - - expect(described_class.max_replication_lag_time).to eq(60) - end - end - - context 'with an explicitly configured value' do - it 'returns the configured value' do - allow(described_class) - .to receive(:configuration) - .and_return({ 'max_replication_lag_time' => 4 }) - - expect(described_class.max_replication_lag_time).to eq(4) - end - end - end - - describe '.replica_check_interval' do - context 'without an explicitly configured value' do - it 'returns the default value' do - allow(described_class) - .to receive(:configuration) - .and_return({}) - - expect(described_class.replica_check_interval).to eq(60) - end - end - - context 'with an explicitly configured value' do - it 'returns the configured value' do - allow(described_class) - .to receive(:configuration) - .and_return({ 'replica_check_interval' => 4 }) - - expect(described_class.replica_check_interval).to eq(4) - end - end - end - - describe '.hosts' do - it 'returns a list of hosts' do - allow(described_class) - .to receive(:configuration) - .and_return({ 'hosts' => %w(foo bar baz) }) - - expect(described_class.hosts).to eq(%w(foo bar baz)) - end - end - - describe '.pool_size' do - it 'returns a Fixnum' do - expect(described_class.pool_size).to be_a_kind_of(Integer) + # There isn't much to test here as the load balancing settings might not + # (and likely aren't) set when running tests. + expect(cfg.pool_size).to eq(raw[:pool]) end end describe '.enable?' do before do - allow(described_class).to receive(:hosts).and_return(%w(foo)) + allow(described_class.configuration) + .to receive(:hosts) + .and_return(%w(foo)) end it 'returns false when no hosts are specified' do - allow(described_class).to receive(:hosts).and_return([]) + allow(described_class.configuration).to receive(:hosts).and_return([]) expect(described_class.enable?).to eq(false) end @@ -163,10 +82,10 @@ RSpec.describe Gitlab::Database::LoadBalancing do end it 'returns true when service discovery is enabled' do - allow(described_class).to receive(:hosts).and_return([]) + allow(described_class.configuration).to receive(:hosts).and_return([]) allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(false) - allow(described_class) + allow(described_class.configuration) .to receive(:service_discovery_enabled?) .and_return(true) @@ -175,17 +94,17 @@ RSpec.describe Gitlab::Database::LoadBalancing do end describe '.configured?' do - it 'returns true when Sidekiq is being used' do - allow(described_class).to receive(:hosts).and_return(%w(foo)) - allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true) + it 'returns true when hosts are configured' do + allow(described_class.configuration) + .to receive(:hosts) + .and_return(%w[foo]) + expect(described_class.configured?).to eq(true) end - it 'returns true when service discovery is enabled in Sidekiq' do - allow(described_class).to receive(:hosts).and_return([]) - allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true) - - allow(described_class) + it 'returns true when service discovery is enabled' do + allow(described_class.configuration).to receive(:hosts).and_return([]) + allow(described_class.configuration) .to receive(:service_discovery_enabled?) .and_return(true) @@ -193,9 +112,8 @@ RSpec.describe Gitlab::Database::LoadBalancing do end it 'returns false when neither service discovery nor hosts are configured' do - allow(described_class).to receive(:hosts).and_return([]) - - allow(described_class) + allow(described_class.configuration).to receive(:hosts).and_return([]) + allow(described_class.configuration) .to receive(:service_discovery_enabled?) .and_return(false) @@ -204,9 +122,11 @@ RSpec.describe Gitlab::Database::LoadBalancing do end describe '.configure_proxy' do - it 'configures the connection proxy' do + before do allow(ActiveRecord::Base).to receive(:load_balancing_proxy=) + end + it 'configures the connection proxy' do described_class.configure_proxy expect(ActiveRecord::Base).to have_received(:load_balancing_proxy=) @@ -214,71 +134,24 @@ RSpec.describe Gitlab::Database::LoadBalancing do end context 'when service discovery is enabled' do - let(:service_discovery) { double(Gitlab::Database::LoadBalancing::ServiceDiscovery) } - it 'runs initial service discovery when configuring the connection proxy' do - allow(described_class) - .to receive(:configuration) - .and_return('discover' => { 'record' => 'foo' }) - - expect(Gitlab::Database::LoadBalancing::ServiceDiscovery).to receive(:new).and_return(service_discovery) - expect(service_discovery).to receive(:perform_service_discovery) - - described_class.configure_proxy - end - end - end + discover = instance_spy(Gitlab::Database::LoadBalancing::ServiceDiscovery) - describe '.active_record_models' do - it 'returns an Array' do - expect(described_class.active_record_models).to be_an_instance_of(Array) - end - end + allow(described_class.configuration) + .to receive(:service_discovery) + .and_return({ record: 'foo' }) - describe '.service_discovery_enabled?' do - it 'returns true if service discovery is enabled' do - allow(described_class) - .to receive(:configuration) - .and_return('discover' => { 'record' => 'foo' }) - - expect(described_class.service_discovery_enabled?).to eq(true) - end + expect(Gitlab::Database::LoadBalancing::ServiceDiscovery) + .to receive(:new) + .with( + an_instance_of(Gitlab::Database::LoadBalancing::LoadBalancer), + an_instance_of(Hash) + ) + .and_return(discover) - it 'returns false if service discovery is disabled' do - expect(described_class.service_discovery_enabled?).to eq(false) - end - end + expect(discover).to receive(:perform_service_discovery) - describe '.service_discovery_configuration' do - context 'when no configuration is provided' do - it 'returns a default configuration Hash' do - expect(described_class.service_discovery_configuration).to eq( - nameserver: 'localhost', - port: 8600, - record: nil, - record_type: 'A', - interval: 60, - disconnect_timeout: 120, - use_tcp: false - ) - end - end - - context 'when configuration is provided' do - it 'returns a Hash including the custom configuration' do - allow(described_class) - .to receive(:configuration) - .and_return('discover' => { 'record' => 'foo', 'record_type' => 'SRV' }) - - expect(described_class.service_discovery_configuration).to eq( - nameserver: 'localhost', - port: 8600, - record: 'foo', - record_type: 'SRV', - interval: 60, - disconnect_timeout: 120, - use_tcp: false - ) + described_class.configure_proxy end end end @@ -292,15 +165,23 @@ RSpec.describe Gitlab::Database::LoadBalancing do end it 'starts service discovery if enabled' do - allow(described_class) + allow(described_class.configuration) .to receive(:service_discovery_enabled?) .and_return(true) instance = double(:instance) + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base) + lb = Gitlab::Database::LoadBalancing::LoadBalancer.new(config) + proxy = Gitlab::Database::LoadBalancing::ConnectionProxy.new(lb) + + allow(described_class) + .to receive(:proxy) + .and_return(proxy) expect(Gitlab::Database::LoadBalancing::ServiceDiscovery) .to receive(:new) - .with(an_instance_of(Hash)) + .with(lb, an_instance_of(Hash)) .and_return(instance) expect(instance) @@ -330,7 +211,13 @@ RSpec.describe Gitlab::Database::LoadBalancing do context 'when the load balancing is configured' do let(:db_host) { ActiveRecord::Base.connection_pool.db_config.host } - let(:proxy) { described_class::ConnectionProxy.new([db_host]) } + let(:config) do + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [db_host]) + end + + let(:load_balancer) { described_class::LoadBalancer.new(config) } + let(:proxy) { described_class::ConnectionProxy.new(load_balancer) } context 'when a proxy connection is used' do it 'returns :unknown' do @@ -770,6 +657,16 @@ RSpec.describe Gitlab::Database::LoadBalancing do it 'redirects queries to the right roles' do roles = [] + # If we don't run any queries, the pool may be a NullPool. This can + # result in some tests reporting a role as `:unknown`, even though the + # tests themselves are correct. + # + # To prevent this from happening we simply run a simple query to + # ensure the proper pool type is put in place. The exact query doesn't + # matter, provided it actually runs a query and thus creates a proper + # connection pool. + model.count + subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do |event| role = ::Gitlab::Database::LoadBalancing.db_role_for_connection(event.payload[:connection]) roles << role if role.present? diff --git a/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb new file mode 100644 index 00000000000..708d1be6e00 --- /dev/null +++ b/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do + let_it_be(:migration) do + ActiveRecord::Migration.new.extend(described_class) + end + + let(:model) do + Class.new(ApplicationRecord) do + self.table_name = 'loose_fk_test_table' + end + end + + before(:all) do + migration.create_table :loose_fk_test_table do |t| + t.timestamps + end + end + + before do + 3.times { model.create! } + end + + context 'when the record deletion tracker trigger is not installed' do + it 'does store record deletions' do + model.delete_all + + expect(LooseForeignKeys::DeletedRecord.count).to eq(0) + end + end + + context 'when the record deletion tracker trigger is installed' do + before do + migration.track_record_deletions(:loose_fk_test_table) + end + + it 'stores the record deletion' do + records = model.all + record_to_be_deleted = records.last + + record_to_be_deleted.delete + + expect(LooseForeignKeys::DeletedRecord.count).to eq(1) + deleted_record = LooseForeignKeys::DeletedRecord.all.first + + expect(deleted_record.deleted_table_primary_key_value).to eq(record_to_be_deleted.id) + expect(deleted_record.deleted_table_name).to eq('loose_fk_test_table') + end + + it 'stores multiple record deletions' do + model.delete_all + + expect(LooseForeignKeys::DeletedRecord.count).to eq(3) + end + end +end diff --git a/spec/lib/gitlab/database/migration_helpers/v2_spec.rb b/spec/lib/gitlab/database/migration_helpers/v2_spec.rb index f132ecbf13b..854e97ef897 100644 --- a/spec/lib/gitlab/database/migration_helpers/v2_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/v2_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::MigrationHelpers::V2 do include Database::TriggerHelpers + include Database::TableSchemaHelpers let(:migration) do ActiveRecord::Migration.new.extend(described_class) @@ -11,6 +12,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do before do allow(migration).to receive(:puts) + + allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false) end shared_examples_for 'Setting up to rename a column' do @@ -218,4 +221,105 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do let(:added_column) { :original } end end + + describe '#create_table' do + let(:table_name) { :test_table } + let(:column_attributes) do + [ + { name: 'id', sql_type: 'bigint', null: false, default: nil }, + { name: 'created_at', sql_type: 'timestamp with time zone', null: false, default: nil }, + { name: 'updated_at', sql_type: 'timestamp with time zone', null: false, default: nil }, + { name: 'some_id', sql_type: 'integer', null: false, default: nil }, + { name: 'active', sql_type: 'boolean', null: false, default: 'true' }, + { name: 'name', sql_type: 'text', null: true, default: nil } + ] + end + + context 'using a limit: attribute on .text' do + it 'creates the table as expected' do + migration.create_table table_name do |t| + t.timestamps_with_timezone + t.integer :some_id, null: false + t.boolean :active, null: false, default: true + t.text :name, limit: 100 + end + + expect_table_columns_to_match(column_attributes, table_name) + expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 100') + end + end + end + + describe '#with_lock_retries' do + let(:model) do + ActiveRecord::Migration.new.extend(described_class) + end + + let(:buffer) { StringIO.new } + let(:in_memory_logger) { Gitlab::JsonLogger.new(buffer) } + let(:env) { { 'DISABLE_LOCK_RETRIES' => 'true' } } + + it 'sets the migration class name in the logs' do + model.with_lock_retries(env: env, logger: in_memory_logger) { } + + buffer.rewind + expect(buffer.read).to include("\"class\":\"#{model.class}\"") + end + + where(raise_on_exhaustion: [true, false]) + + with_them do + it 'sets raise_on_exhaustion as requested' do + with_lock_retries = double + expect(Gitlab::Database::WithLockRetries).to receive(:new).and_return(with_lock_retries) + expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: raise_on_exhaustion) + + model.with_lock_retries(env: env, logger: in_memory_logger, raise_on_exhaustion: raise_on_exhaustion) { } + end + end + + it 'does not raise on exhaustion by default' do + with_lock_retries = double + expect(Gitlab::Database::WithLockRetries).to receive(:new).and_return(with_lock_retries) + expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) + + model.with_lock_retries(env: env, logger: in_memory_logger) { } + end + + it 'defaults to disallowing subtransactions' do + with_lock_retries = double + expect(Gitlab::Database::WithLockRetries).to receive(:new).with(hash_including(allow_savepoints: false)).and_return(with_lock_retries) + expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) + + model.with_lock_retries(env: env, logger: in_memory_logger) { } + end + + context 'when in transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(true) + end + + context 'when lock retries are enabled' do + before do + allow(model).to receive(:enable_lock_retries?).and_return(true) + end + + it 'does not use Gitlab::Database::WithLockRetries and executes the provided block directly' do + expect(Gitlab::Database::WithLockRetries).not_to receive(:new) + + expect(model.with_lock_retries(env: env, logger: in_memory_logger) { :block_result }).to eq(:block_result) + end + end + + context 'when lock retries are not enabled' do + before do + allow(model).to receive(:enable_lock_retries?).and_return(false) + end + + it 'raises an error' do + expect { model.with_lock_retries(env: env, logger: in_memory_logger) { } }.to raise_error /can not be run inside an already open transaction/ + end + end + end + end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 9f9aef77de7..006f8a39f9c 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -798,13 +798,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers do # This spec runs without an enclosing transaction (:delete truncation method for db_cleaner) context 'when the statement_timeout is already disabled', :delete do before do - ActiveRecord::Base.connection.execute('SET statement_timeout TO 0') + ActiveRecord::Migration.connection.execute('SET statement_timeout TO 0') end after do - # Use ActiveRecord::Base.connection instead of model.execute + # Use ActiveRecord::Migration.connection instead of model.execute # so that this call is not counted below - ActiveRecord::Base.connection.execute('RESET statement_timeout') + ActiveRecord::Migration.connection.execute('RESET statement_timeout') end it 'yields control without disabling the timeout or resetting' do @@ -954,10 +954,11 @@ RSpec.describe Gitlab::Database::MigrationHelpers do let(:trigger_name) { model.rename_trigger_name(:users, :id, :new) } let(:user) { create(:user) } let(:copy_trigger) { double('copy trigger') } + let(:connection) { ActiveRecord::Migration.connection } before do expect(Gitlab::Database::UnidirectionalCopyTrigger).to receive(:on_table) - .with(:users).and_return(copy_trigger) + .with(:users, connection: connection).and_return(copy_trigger) end it 'copies the value to the new column using the type_cast_function', :aggregate_failures do @@ -1300,11 +1301,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe '#install_rename_triggers' do + let(:connection) { ActiveRecord::Migration.connection } + it 'installs the triggers' do copy_trigger = double('copy trigger') expect(Gitlab::Database::UnidirectionalCopyTrigger).to receive(:on_table) - .with(:users).and_return(copy_trigger) + .with(:users, connection: connection).and_return(copy_trigger) expect(copy_trigger).to receive(:create).with(:old, :new, trigger_name: 'foo') @@ -1313,11 +1316,13 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe '#remove_rename_triggers' do + let(:connection) { ActiveRecord::Migration.connection } + it 'removes the function and trigger' do copy_trigger = double('copy trigger') expect(Gitlab::Database::UnidirectionalCopyTrigger).to receive(:on_table) - .with('bar').and_return(copy_trigger) + .with('bar', connection: connection).and_return(copy_trigger) expect(copy_trigger).to receive(:drop).with('foo') @@ -1886,6 +1891,61 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end + describe '#restore_conversion_of_integer_to_bigint' do + let(:table) { :test_table } + let(:column) { :id } + let(:tmp_column) { model.convert_to_bigint_column(column) } + + before do + model.create_table table, id: false do |t| + t.bigint :id, primary_key: true + t.bigint :build_id, null: false + t.timestamps + end + end + + context 'when the target table does not exist' do + it 'raises an error' do + expect { model.restore_conversion_of_integer_to_bigint(:this_table_is_not_real, column) } + .to raise_error('Table this_table_is_not_real does not exist') + end + end + + context 'when the column to migrate does not exist' do + it 'raises an error' do + expect { model.restore_conversion_of_integer_to_bigint(table, :this_column_is_not_real) } + .to raise_error(ArgumentError, "Column this_column_is_not_real does not exist on #{table}") + end + end + + context 'when a single column is given' do + let(:column_to_convert) { 'id' } + let(:temporary_column) { model.convert_to_bigint_column(column_to_convert) } + + it 'creates the correct columns and installs the trigger' do + expect(model).to receive(:add_column).with(table, temporary_column, :int, default: 0, null: false) + + expect(model).to receive(:install_rename_triggers).with(table, [column_to_convert], [temporary_column]) + + model.restore_conversion_of_integer_to_bigint(table, column_to_convert) + end + end + + context 'when multiple columns are given' do + let(:columns_to_convert) { %i[id build_id] } + let(:temporary_columns) { columns_to_convert.map { |column| model.convert_to_bigint_column(column) } } + + it 'creates the correct columns and installs the trigger' do + expect(model).to receive(:add_column).with(table, temporary_columns[0], :int, default: 0, null: false) + expect(model).to receive(:add_column).with(table, temporary_columns[1], :int, default: 0, null: false) + + expect(model).to receive(:install_rename_triggers).with(table, columns_to_convert, temporary_columns) + + model.restore_conversion_of_integer_to_bigint(table, columns_to_convert) + end + end + end + describe '#revert_initialize_conversion_of_integer_to_bigint' do let(:table) { :test_table } @@ -2139,7 +2199,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do describe '#index_exists_by_name?' do it 'returns true if an index exists' do - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE INDEX test_index_for_index_exists ON projects (path);' ) @@ -2154,7 +2214,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do context 'when an index with a function exists' do before do - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE INDEX test_index ON projects (LOWER(path));' ) end @@ -2167,15 +2227,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do context 'when an index exists for a table with the same name in another schema' do before do - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE SCHEMA new_test_schema' ) - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE TABLE new_test_schema.projects (id integer, name character varying)' ) - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE INDEX test_index_on_name ON new_test_schema.projects (LOWER(name));' ) end @@ -2255,8 +2315,6 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(buffer.read).to include("\"class\":\"#{model.class}\"") end - using RSpec::Parameterized::TableSyntax - where(raise_on_exhaustion: [true, false]) with_them do @@ -2276,6 +2334,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.with_lock_retries(env: env, logger: in_memory_logger) { } end + + it 'defaults to allowing subtransactions' do + with_lock_retries = double + + expect(Gitlab::Database::WithLockRetries).to receive(:new).with(hash_including(allow_savepoints: true)).and_return(with_lock_retries) + expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) + + model.with_lock_retries(env: env, logger: in_memory_logger) { } + end end describe '#backfill_iids' do @@ -2401,19 +2468,19 @@ RSpec.describe Gitlab::Database::MigrationHelpers do describe '#check_constraint_exists?' do before do - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'ALTER TABLE projects ADD CONSTRAINT check_1 CHECK (char_length(path) <= 5) NOT VALID' ) - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE SCHEMA new_test_schema' ) - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'CREATE TABLE new_test_schema.projects (id integer, name character varying)' ) - ActiveRecord::Base.connection.execute( + ActiveRecord::Migration.connection.execute( 'ALTER TABLE new_test_schema.projects ADD CONSTRAINT check_2 CHECK (char_length(name) <= 5)' ) end @@ -2628,6 +2695,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe '#remove_check_constraint' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + it 'removes the constraint' do drop_sql = /ALTER TABLE test_table\s+DROP CONSTRAINT IF EXISTS check_name/ diff --git a/spec/lib/gitlab/database/migration_spec.rb b/spec/lib/gitlab/database/migration_spec.rb new file mode 100644 index 00000000000..287e738c24e --- /dev/null +++ b/spec/lib/gitlab/database/migration_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migration do + describe '.[]' do + context 'version: 1.0' do + subject { described_class[1.0] } + + it 'inherits from ActiveRecord::Migration[6.1]' do + expect(subject.superclass).to eq(ActiveRecord::Migration[6.1]) + end + + it 'includes migration helpers version 2' do + expect(subject.included_modules).to include(Gitlab::Database::MigrationHelpers::V2) + end + + it 'includes LockRetriesConcern' do + expect(subject.included_modules).to include(Gitlab::Database::Migration::LockRetriesConcern) + end + end + + context 'unknown version' do + it 'raises an error' do + expect { described_class[0] }.to raise_error(ArgumentError, /Unknown migration version/) + end + end + end + + describe '.current_version' do + it 'includes current ActiveRecord migration class' do + # This breaks upon Rails upgrade. In that case, we'll add a new version in Gitlab::Database::Migration::MIGRATION_CLASSES, + # bump .current_version and leave existing migrations and already defined versions of Gitlab::Database::Migration + # untouched. + expect(described_class[described_class.current_version].superclass).to eq(ActiveRecord::Migration::Current) + end + end + + describe Gitlab::Database::Migration::LockRetriesConcern do + subject { class_def.new } + + context 'when not explicitly called' do + let(:class_def) do + Class.new do + include Gitlab::Database::Migration::LockRetriesConcern + end + end + + it 'does not disable lock retries by default' do + expect(subject.enable_lock_retries?).not_to be_truthy + end + end + + context 'when explicitly disabled' do + let(:class_def) do + Class.new do + include Gitlab::Database::Migration::LockRetriesConcern + + enable_lock_retries! + end + end + + it 'does not disable lock retries by default' do + expect(subject.enable_lock_retries?).to be_truthy + end + end + end +end diff --git a/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb b/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb new file mode 100644 index 00000000000..076fb9e8215 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::LockRetryMixin do + describe Gitlab::Database::Migrations::LockRetryMixin::ActiveRecordMigrationProxyLockRetries do + let(:migration) { double } + let(:return_value) { double } + let(:class_def) do + Class.new do + include Gitlab::Database::Migrations::LockRetryMixin::ActiveRecordMigrationProxyLockRetries + + attr_reader :migration + + def initialize(migration) + @migration = migration + end + end + end + + describe '#enable_lock_retries?' do + subject { class_def.new(migration).enable_lock_retries? } + + it 'delegates to #migration' do + expect(migration).to receive(:enable_lock_retries?).and_return(return_value) + + result = subject + + expect(result).to eq(return_value) + end + end + + describe '#migration_class' do + subject { class_def.new(migration).migration_class } + + it 'retrieves actual migration class from #migration' do + expect(migration).to receive(:class).and_return(return_value) + + result = subject + + expect(result).to eq(return_value) + end + end + end + + describe Gitlab::Database::Migrations::LockRetryMixin::ActiveRecordMigratorLockRetries do + let(:class_def) do + Class.new do + attr_reader :receiver + + def initialize(receiver) + @receiver = receiver + end + + def ddl_transaction(migration, &block) + receiver.ddl_transaction(migration, &block) + end + + def use_transaction?(migration) + receiver.use_transaction?(migration) + end + end.prepend(Gitlab::Database::Migrations::LockRetryMixin::ActiveRecordMigratorLockRetries) + end + + subject { class_def.new(receiver) } + + before do + allow(migration).to receive(:migration_class).and_return('TestClass') + allow(receiver).to receive(:ddl_transaction) + end + + context 'with transactions disabled' do + let(:migration) { double('migration', enable_lock_retries?: false) } + let(:receiver) { double('receiver', use_transaction?: false)} + + it 'calls super method' do + p = proc { } + + expect(receiver).to receive(:ddl_transaction).with(migration, &p) + + subject.ddl_transaction(migration, &p) + end + end + + context 'with transactions enabled, but lock retries disabled' do + let(:receiver) { double('receiver', use_transaction?: true)} + let(:migration) { double('migration', enable_lock_retries?: false) } + + it 'calls super method' do + p = proc { } + + expect(receiver).to receive(:ddl_transaction).with(migration, &p) + + subject.ddl_transaction(migration, &p) + end + end + + context 'with transactions enabled and lock retries enabled' do + let(:receiver) { double('receiver', use_transaction?: true)} + let(:migration) { double('migration', enable_lock_retries?: true) } + + it 'calls super method' do + p = proc { } + + expect(receiver).not_to receive(:ddl_transaction) + expect_next_instance_of(Gitlab::Database::WithLockRetries) do |retries| + expect(retries).to receive(:run).with(raise_on_exhaustion: false, &p) + end + + subject.ddl_transaction(migration, &p) + end + end + end + + describe '.patch!' do + subject { described_class.patch! } + + it 'patches MigrationProxy' do + expect(ActiveRecord::MigrationProxy).to receive(:prepend).with(Gitlab::Database::Migrations::LockRetryMixin::ActiveRecordMigrationProxyLockRetries) + + subject + end + + it 'patches Migrator' do + expect(ActiveRecord::Migrator).to receive(:prepend).with(Gitlab::Database::Migrations::LockRetryMixin::ActiveRecordMigratorLockRetries) + + subject + end + end +end diff --git a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb index c4fbf53d1c2..27ada12b067 100644 --- a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do + let(:connection) { ActiveRecord::Base.connection } + describe '#current_partitions' do subject { described_class.new(model, partitioning_key).current_partitions } @@ -11,7 +13,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do let(:table_name) { :partitioned_test } before do - ActiveRecord::Base.connection.execute(<<~SQL) + connection.execute(<<~SQL) CREATE TABLE #{table_name} (id serial not null, created_at timestamptz not null, PRIMARY KEY (id, created_at)) PARTITION BY RANGE (created_at); @@ -52,7 +54,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do context 'with existing partitions' do before do - ActiveRecord::Base.connection.execute(<<~SQL) + connection.execute(<<~SQL) CREATE TABLE #{model.table_name} (id serial not null, created_at timestamptz not null, PRIMARY KEY (id, created_at)) PARTITION BY RANGE (created_at); @@ -113,7 +115,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do context 'without existing partitions' do before do - ActiveRecord::Base.connection.execute(<<~SQL) + connection.execute(<<~SQL) CREATE TABLE #{model.table_name} (id serial not null, created_at timestamptz not null, PRIMARY KEY (id, created_at)) PARTITION BY RANGE (created_at); @@ -159,7 +161,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do context 'with a regular partition but no catchall (MINVALUE, to) partition' do before do - ActiveRecord::Base.connection.execute(<<~SQL) + connection.execute(<<~SQL) CREATE TABLE #{model.table_name} (id serial not null, created_at timestamptz not null, PRIMARY KEY (id, created_at)) PARTITION BY RANGE (created_at); @@ -248,6 +250,25 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01', partition_name: 'partitioned_test_202005') ) end + + context 'when the retain_non_empty_partitions is true' do + subject { described_class.new(model, partitioning_key, retain_for: 2.months, retain_non_empty_partitions: true).extra_partitions } + + it 'prunes empty partitions' do + expect(subject).to contain_exactly( + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: 'partitioned_test_000000'), + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01', partition_name: 'partitioned_test_202005') + ) + end + + it 'does not prune non-empty partitions' do + connection.execute("INSERT INTO #{table_name} (created_at) VALUES (('2020-05-15'))") # inserting one record into partitioned_test_202005 + + expect(subject).to contain_exactly( + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: 'partitioned_test_000000') + ) + end + end end end end diff --git a/spec/lib/gitlab/database/partitioning/multi_database_partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/multi_database_partition_manager_spec.rb new file mode 100644 index 00000000000..3c94c1bf4ea --- /dev/null +++ b/spec/lib/gitlab/database/partitioning/multi_database_partition_manager_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Partitioning::MultiDatabasePartitionManager, '#sync_partitions' do + subject(:sync_partitions) { manager.sync_partitions } + + let(:manager) { described_class.new(models) } + let(:models) { [model1, model2] } + + let(:model1) { double('model1', connection: connection1, table_name: 'table1') } + let(:model2) { double('model2', connection: connection1, table_name: 'table2') } + + let(:connection1) { double('connection1') } + let(:connection2) { double('connection2') } + + let(:target_manager_class) { Gitlab::Database::Partitioning::PartitionManager } + let(:target_manager1) { double('partition manager') } + let(:target_manager2) { double('partition manager') } + + before do + allow(manager).to receive(:connection_name).and_return('name') + end + + it 'syncs model partitions, setting up the appropriate connection for each', :aggregate_failures do + expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(model1.connection).and_yield.ordered + expect(target_manager_class).to receive(:new).with(model1).and_return(target_manager1).ordered + expect(target_manager1).to receive(:sync_partitions) + + expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(model2.connection).and_yield.ordered + expect(target_manager_class).to receive(:new).with(model2).and_return(target_manager2).ordered + expect(target_manager2).to receive(:sync_partitions) + + sync_partitions + end +end diff --git a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb index 3d60457c3a9..8f1f5b5ba1b 100644 --- a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb @@ -12,26 +12,18 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do end end - describe '.register' do - let(:model) { double(partitioning_strategy: nil) } - - it 'remembers registered models' do - expect { described_class.register(model) }.to change { described_class.models }.to include(model) - end - end - context 'creating partitions (mocked)' do - subject(:sync_partitions) { described_class.new(models).sync_partitions } + subject(:sync_partitions) { described_class.new(model).sync_partitions } - let(:models) { [model] } - let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table) } + let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table, connection: connection) } let(:partitioning_strategy) { double(missing_partitions: partitions, extra_partitions: []) } + let(:connection) { ActiveRecord::Base.connection } let(:table) { "some_table" } before do - allow(ActiveRecord::Base.connection).to receive(:table_exists?).and_call_original - allow(ActiveRecord::Base.connection).to receive(:table_exists?).with(table).and_return(true) - allow(ActiveRecord::Base.connection).to receive(:execute).and_call_original + allow(connection).to receive(:table_exists?).and_call_original + allow(connection).to receive(:table_exists?).with(table).and_return(true) + allow(connection).to receive(:execute).and_call_original stub_exclusive_lease(described_class::MANAGEMENT_LEASE_KEY % table, timeout: described_class::LEASE_TIMEOUT) end @@ -44,35 +36,23 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do end it 'creates the partition' do - expect(ActiveRecord::Base.connection).to receive(:execute).with(partitions.first.to_sql) - expect(ActiveRecord::Base.connection).to receive(:execute).with(partitions.second.to_sql) + expect(connection).to receive(:execute).with(partitions.first.to_sql) + expect(connection).to receive(:execute).with(partitions.second.to_sql) sync_partitions end - context 'error handling with 2 models' do - let(:models) do - [ - double(partitioning_strategy: strategy1, table_name: table), - double(partitioning_strategy: strategy2, table_name: table) - ] - end - - let(:strategy1) { double('strategy1', missing_partitions: nil, extra_partitions: []) } - let(:strategy2) { double('strategy2', missing_partitions: partitions, extra_partitions: []) } + context 'when an error occurs during partition management' do + it 'does not raise an error' do + expect(partitioning_strategy).to receive(:missing_partitions).and_raise('this should never happen (tm)') - it 'still creates partitions for the second table' do - expect(strategy1).to receive(:missing_partitions).and_raise('this should never happen (tm)') - expect(ActiveRecord::Base.connection).to receive(:execute).with(partitions.first.to_sql) - expect(ActiveRecord::Base.connection).to receive(:execute).with(partitions.second.to_sql) - - sync_partitions + expect { sync_partitions }.not_to raise_error end end end context 'creating partitions' do - subject(:sync_partitions) { described_class.new([my_model]).sync_partitions } + subject(:sync_partitions) { described_class.new(my_model).sync_partitions } let(:connection) { ActiveRecord::Base.connection } let(:my_model) do @@ -101,15 +81,15 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do context 'detaching partitions (mocked)' do subject(:sync_partitions) { manager.sync_partitions } - let(:manager) { described_class.new(models) } - let(:models) { [model] } - let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table)} + let(:manager) { described_class.new(model) } + let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table, connection: connection) } let(:partitioning_strategy) { double(extra_partitions: extra_partitions, missing_partitions: []) } + let(:connection) { ActiveRecord::Base.connection } let(:table) { "foo" } before do - allow(ActiveRecord::Base.connection).to receive(:table_exists?).and_call_original - allow(ActiveRecord::Base.connection).to receive(:table_exists?).with(table).and_return(true) + allow(connection).to receive(:table_exists?).and_call_original + allow(connection).to receive(:table_exists?).with(table).and_return(true) stub_exclusive_lease(described_class::MANAGEMENT_LEASE_KEY % table, timeout: described_class::LEASE_TIMEOUT) end @@ -131,24 +111,6 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do sync_partitions end - - context 'error handling' do - let(:models) do - [ - double(partitioning_strategy: error_strategy, table_name: table), - model - ] - end - - let(:error_strategy) { double(extra_partitions: nil, missing_partitions: []) } - - it 'still drops partitions for the other model' do - expect(error_strategy).to receive(:extra_partitions).and_raise('injected error!') - extra_partitions.each { |p| expect(manager).to receive(:detach_one_partition).with(p) } - - sync_partitions - end - end end context 'with the partition_pruning feature flag disabled' do @@ -171,7 +133,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do end end - subject { described_class.new([my_model]).sync_partitions } + subject { described_class.new(my_model).sync_partitions } let(:connection) { ActiveRecord::Base.connection } let(:my_model) do @@ -280,11 +242,11 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do it 'creates partitions for the future then drops the oldest one after a month' do # 1 month for the current month, 1 month for the old month that we're retaining data for, headroom expected_num_partitions = (Gitlab::Database::Partitioning::MonthlyStrategy::HEADROOM + 2.months) / 1.month - expect { described_class.new([my_model]).sync_partitions }.to change { num_partitions(my_model) }.from(0).to(expected_num_partitions) + expect { described_class.new(my_model).sync_partitions }.to change { num_partitions(my_model) }.from(0).to(expected_num_partitions) travel 1.month - expect { described_class.new([my_model]).sync_partitions }.to change { has_partition(my_model, 2.months.ago.beginning_of_month) }.from(true).to(false).and(change { num_partitions(my_model) }.by(0)) + expect { described_class.new(my_model).sync_partitions }.to change { has_partition(my_model, 2.months.ago.beginning_of_month) }.from(true).to(false).and(change { num_partitions(my_model) }.by(0)) end end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb index a524fe681e9..f0e34476cf2 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers_spec.rb @@ -27,6 +27,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers before do allow(migration).to receive(:puts) + allow(migration).to receive(:transaction_open?).and_return(false) connection.execute(<<~SQL) CREATE TABLE #{target_table_name} ( @@ -141,5 +142,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers .with(source_table_name, target_table_name, options) end end + + context 'when run inside a transaction block' do + it 'raises an error' do + expect(migration).to receive(:transaction_open?).and_return(true) + + expect do + migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, column: column_name) + end.to raise_error(/can not be run inside a transaction/) + end + end end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb index c3edc3a0c87..8ab3816529b 100644 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb +++ b/spec/lib/gitlab/database/partitioning_migration_helpers/index_helpers_spec.rb @@ -20,6 +20,7 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do before do allow(migration).to receive(:puts) + allow(migration).to receive(:transaction_open?).and_return(false) connection.execute(<<~SQL) CREATE TABLE #{table_name} ( @@ -127,6 +128,16 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do end.to raise_error(ArgumentError, /#{table_name} is not a partitioned table/) end end + + context 'when run inside a transaction block' do + it 'raises an error' do + expect(migration).to receive(:transaction_open?).and_return(true) + + expect do + migration.add_concurrent_partitioned_index(table_name, column_name) + end.to raise_error(/can not be run inside a transaction/) + end + end end describe '#remove_concurrent_partitioned_index_by_name' do @@ -182,5 +193,15 @@ RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::IndexHelpers do end.to raise_error(ArgumentError, /#{table_name} is not a partitioned table/) end end + + context 'when run inside a transaction block' do + it 'raises an error' do + expect(migration).to receive(:transaction_open?).and_return(true) + + expect do + migration.remove_concurrent_partitioned_index_by_name(table_name, index_name) + end.to raise_error(/can not be run inside a transaction/) + end + end end end diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb new file mode 100644 index 00000000000..f163b45e01e --- /dev/null +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Partitioning do + describe '.sync_partitions' do + let(:partition_manager_class) { described_class::MultiDatabasePartitionManager } + let(:partition_manager) { double('partition manager') } + + context 'when no partitioned models are given' do + it 'calls the partition manager with the registered models' do + expect(partition_manager_class).to receive(:new) + .with(described_class.registered_models) + .and_return(partition_manager) + + expect(partition_manager).to receive(:sync_partitions) + + described_class.sync_partitions + end + end + + context 'when partitioned models are given' do + it 'calls the partition manager with the given models' do + models = ['my special model'] + + expect(partition_manager_class).to receive(:new) + .with(models) + .and_return(partition_manager) + + expect(partition_manager).to receive(:sync_partitions) + + described_class.sync_partitions(models) + end + end + end +end diff --git a/spec/lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin_spec.rb b/spec/lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin_spec.rb index 40e36bc02e9..8b06f068503 100644 --- a/spec/lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin_spec.rb +++ b/spec/lib/gitlab/database/postgresql_adapter/dump_schema_versions_mixin_spec.rb @@ -26,4 +26,12 @@ RSpec.describe Gitlab::Database::PostgresqlAdapter::DumpSchemaVersionsMixin do instance.dump_schema_information end + + it 'does not call touch_all in production' do + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('production')) + + expect(Gitlab::Database::SchemaMigrations).not_to receive(:touch_all) + + instance.dump_schema_information + end end diff --git a/spec/lib/gitlab/database/schema_migrations/context_spec.rb b/spec/lib/gitlab/database/schema_migrations/context_spec.rb index 1f1943d00a3..a79e6706149 100644 --- a/spec/lib/gitlab/database/schema_migrations/context_spec.rb +++ b/spec/lib/gitlab/database/schema_migrations/context_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do describe '#schema_directory' do it 'returns db/schema_migrations' do - expect(context.schema_directory).to eq(File.join(Rails.root, 'db/schema_migrations')) + expect(context.schema_directory).to eq(File.join(Rails.root, described_class.default_schema_migrations_path)) end context 'CI database' do @@ -19,7 +19,7 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do it 'returns a directory path that is database specific' do skip_if_multiple_databases_not_setup - expect(context.schema_directory).to eq(File.join(Rails.root, 'db/schema_migrations')) + expect(context.schema_directory).to eq(File.join(Rails.root, described_class.default_schema_migrations_path)) end end @@ -124,8 +124,4 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do end end end - - def skip_if_multiple_databases_not_setup - skip 'Skipping because multiple databases not set up' unless Gitlab::Database.has_config?(:ci) - end end diff --git a/spec/lib/gitlab/database/shared_model_spec.rb b/spec/lib/gitlab/database/shared_model_spec.rb new file mode 100644 index 00000000000..5d616aeb05f --- /dev/null +++ b/spec/lib/gitlab/database/shared_model_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SharedModel do + describe 'using an external connection' do + let!(:original_connection) { described_class.connection } + let(:new_connection) { double('connection') } + + it 'overrides the connection for the duration of the block', :aggregate_failures do + expect_original_connection_around do + described_class.using_connection(new_connection) do + expect(described_class.connection).to be(new_connection) + end + end + end + + it 'does not affect connections in other threads', :aggregate_failures do + expect_original_connection_around do + described_class.using_connection(new_connection) do + expect(described_class.connection).to be(new_connection) + + Thread.new do + expect(described_class.connection).not_to be(new_connection) + end.join + end + end + end + + context 'when the block raises an error', :aggregate_failures do + it 're-raises the error, removing the overridden connection' do + expect_original_connection_around do + expect do + described_class.using_connection(new_connection) do + expect(described_class.connection).to be(new_connection) + + raise 'here comes an error!' + end + end.to raise_error(RuntimeError, 'here comes an error!') + 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 +end diff --git a/spec/lib/gitlab/database/transaction/context_spec.rb b/spec/lib/gitlab/database/transaction/context_spec.rb index 65d52b4d099..37cfc841d48 100644 --- a/spec/lib/gitlab/database/transaction/context_spec.rb +++ b/spec/lib/gitlab/database/transaction/context_spec.rb @@ -62,30 +62,32 @@ RSpec.describe Gitlab::Database::Transaction::Context do it { expect(data[:queries]).to eq(['SELECT 1', 'SELECT * FROM users']) } end - describe '#duration' do + describe '#track_backtrace' do before do - subject.set_start_time + subject.track_backtrace(caller) end - it { expect(subject.duration).to be >= 0 } - end + it { expect(data[:backtraces]).to be_a(Array) } + it { expect(data[:backtraces]).to all(be_a(Array)) } + it { expect(data[:backtraces].length).to eq(1) } + it { expect(data[:backtraces][0][0]).to be_a(String) } - context 'when depth is low' do - it 'does not log data upon COMMIT' do - expect(subject).not_to receive(:application_info) + it 'appends the backtrace' do + subject.track_backtrace(caller) - subject.commit + expect(data[:backtraces].length).to eq(2) + expect(subject.backtraces).to be_a(Array) + expect(subject.backtraces).to all(be_a(Array)) + expect(subject.backtraces[1][0]).to be_a(String) end + end - it 'does not log data upon ROLLBACK' do - expect(subject).not_to receive(:application_info) - - subject.rollback + describe '#duration' do + before do + subject.set_start_time end - it '#should_log? returns false' do - expect(subject.should_log?).to be false - end + it { expect(subject.duration).to be >= 0 } end shared_examples 'logs transaction data' do @@ -116,17 +118,9 @@ RSpec.describe Gitlab::Database::Transaction::Context do end end - context 'when depth exceeds threshold' do - before do - subject.set_depth(described_class::LOG_DEPTH_THRESHOLD + 1) - end - - it_behaves_like 'logs transaction data' - end - context 'when savepoints count exceeds threshold' do before do - data[:savepoints] = described_class::LOG_SAVEPOINTS_THRESHOLD + 1 + data[:savepoints] = 1 end it_behaves_like 'logs transaction data' diff --git a/spec/lib/gitlab/database/transaction/observer_spec.rb b/spec/lib/gitlab/database/transaction/observer_spec.rb index 7aa24217dc3..e5cc0106c9b 100644 --- a/spec/lib/gitlab/database/transaction/observer_spec.rb +++ b/spec/lib/gitlab/database/transaction/observer_spec.rb @@ -25,7 +25,7 @@ RSpec.describe Gitlab::Database::Transaction::Observer do User.first expect(transaction_context).to be_a(::Gitlab::Database::Transaction::Context) - expect(context.keys).to match_array(%i(start_time depth savepoints queries)) + expect(context.keys).to match_array(%i(start_time depth savepoints queries backtraces)) expect(context[:depth]).to eq(2) expect(context[:savepoints]).to eq(1) expect(context[:queries].length).to eq(1) @@ -35,6 +35,7 @@ RSpec.describe Gitlab::Database::Transaction::Observer do expect(context[:depth]).to eq(2) expect(context[:savepoints]).to eq(1) expect(context[:releases]).to eq(1) + expect(context[:backtraces].length).to eq(1) end describe '.extract_sql_command' do diff --git a/spec/lib/gitlab/database/with_lock_retries_spec.rb b/spec/lib/gitlab/database/with_lock_retries_spec.rb index 72074f06210..0b960830d89 100644 --- a/spec/lib/gitlab/database/with_lock_retries_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_spec.rb @@ -5,7 +5,9 @@ require 'spec_helper' RSpec.describe Gitlab::Database::WithLockRetries do let(:env) { {} } let(:logger) { Gitlab::Database::WithLockRetries::NULL_LOGGER } - let(:subject) { described_class.new(env: env, logger: logger, timing_configuration: timing_configuration) } + let(:subject) { described_class.new(env: env, logger: logger, allow_savepoints: allow_savepoints, timing_configuration: timing_configuration) } + let(:allow_savepoints) { true } + let(:connection) { ActiveRecord::Base.connection } let(:timing_configuration) do [ @@ -66,7 +68,7 @@ RSpec.describe Gitlab::Database::WithLockRetries do WHERE t.relkind = 'r' AND l.mode = 'ExclusiveLock' AND t.relname = '#{Project.table_name}' """ - expect(ActiveRecord::Base.connection.execute(check_exclusive_lock_query).to_a).to be_present + expect(connection.execute(check_exclusive_lock_query).to_a).to be_present end end @@ -95,8 +97,8 @@ RSpec.describe Gitlab::Database::WithLockRetries do lock_fiber.resume end - ActiveRecord::Base.transaction do - ActiveRecord::Base.connection.execute("LOCK TABLE #{Project.table_name} in exclusive mode") + connection.transaction do + connection.execute("LOCK TABLE #{Project.table_name} in exclusive mode") lock_acquired = true end end @@ -114,7 +116,7 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'setting the idle transaction timeout' do context 'when there is no outer transaction: disable_ddl_transaction! is set in the migration' do it 'does not disable the idle transaction timeout' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false) + allow(connection).to receive(:transaction_open?).and_return(false) allow(subject).to receive(:run_block_with_lock_timeout).once.and_raise(ActiveRecord::LockWaitTimeout) allow(subject).to receive(:run_block_with_lock_timeout).once @@ -126,7 +128,7 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'when there is outer transaction: disable_ddl_transaction! is not set in the migration' do it 'disables the idle transaction timeout so the code can sleep and retry' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(true) + allow(connection).to receive(:transaction_open?).and_return(true) n = 0 allow(subject).to receive(:run_block_with_lock_timeout).twice do @@ -151,7 +153,7 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'when there is no outer transaction: disable_ddl_transaction! is set in the migration' do it 'does not disable the lock_timeout' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false) + allow(connection).to receive(:transaction_open?).and_return(false) allow(subject).to receive(:run_block_with_lock_timeout).once.and_raise(ActiveRecord::LockWaitTimeout) expect(subject).not_to receive(:disable_lock_timeout) @@ -162,7 +164,7 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'when there is outer transaction: disable_ddl_transaction! is not set in the migration' do it 'disables the lock_timeout' do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(true) + allow(connection).to receive(:transaction_open?).and_return(true) allow(subject).to receive(:run_block_with_lock_timeout).once.and_raise(ActiveRecord::LockWaitTimeout) expect(subject).to receive(:disable_lock_timeout) @@ -197,8 +199,8 @@ RSpec.describe Gitlab::Database::WithLockRetries do subject.run(raise_on_exhaustion: true) do lock_attempts += 1 - ActiveRecord::Base.transaction do - ActiveRecord::Base.connection.execute("LOCK TABLE #{Project.table_name} in exclusive mode") + connection.transaction do + connection.execute("LOCK TABLE #{Project.table_name} in exclusive mode") lock_acquired = true end end @@ -212,11 +214,11 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'when statement timeout is reached' do it 'raises QueryCanceled error' do lock_acquired = false - ActiveRecord::Base.connection.execute("SET LOCAL statement_timeout='100ms'") + connection.execute("SET LOCAL statement_timeout='100ms'") expect do subject.run do - ActiveRecord::Base.connection.execute("SELECT 1 FROM pg_sleep(0.11)") # 110ms + connection.execute("SELECT 1 FROM pg_sleep(0.11)") # 110ms lock_acquired = true end end.to raise_error(ActiveRecord::QueryCanceled) @@ -229,11 +231,11 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'restore local database variables' do it do - expect { subject.run {} }.not_to change { ActiveRecord::Base.connection.execute("SHOW lock_timeout").to_a } + expect { subject.run {} }.not_to change { connection.execute("SHOW lock_timeout").to_a } end it do - expect { subject.run {} }.not_to change { ActiveRecord::Base.connection.execute("SHOW idle_in_transaction_session_timeout").to_a } + expect { subject.run {} }.not_to change { connection.execute("SHOW idle_in_transaction_session_timeout").to_a } end end @@ -241,10 +243,10 @@ RSpec.describe Gitlab::Database::WithLockRetries do let(:timing_configuration) { [[0.015.seconds, 0.025.seconds], [0.015.seconds, 0.025.seconds]] } # 15ms, 25ms it 'executes `SET LOCAL lock_timeout` using the configured timeout value in milliseconds' do - expect(ActiveRecord::Base.connection).to receive(:execute).with("RESET idle_in_transaction_session_timeout; RESET lock_timeout").and_call_original - expect(ActiveRecord::Base.connection).to receive(:execute).with("SAVEPOINT active_record_1", "TRANSACTION").and_call_original - expect(ActiveRecord::Base.connection).to receive(:execute).with("SET LOCAL lock_timeout TO '15ms'").and_call_original - expect(ActiveRecord::Base.connection).to receive(:execute).with("RELEASE SAVEPOINT active_record_1", "TRANSACTION").and_call_original + expect(connection).to receive(:execute).with("RESET idle_in_transaction_session_timeout; RESET lock_timeout").and_call_original + expect(connection).to receive(:execute).with("SAVEPOINT active_record_1", "TRANSACTION").and_call_original + expect(connection).to receive(:execute).with("SET LOCAL lock_timeout TO '15ms'").and_call_original + expect(connection).to receive(:execute).with("RELEASE SAVEPOINT active_record_1", "TRANSACTION").and_call_original subject.run { } end @@ -256,4 +258,20 @@ RSpec.describe Gitlab::Database::WithLockRetries do subject.run { } end end + + context 'Stop using subtransactions - allow_savepoints: false' do + let(:allow_savepoints) { false } + + it 'prevents running inside already open transaction' do + allow(connection).to receive(:transaction_open?).and_return(true) + + expect { subject.run { } }.to raise_error(/should not run inside already open transaction/) + end + + it 'does not raise the error if not inside open transaction' do + allow(connection).to receive(:transaction_open?).and_return(false) + + expect { subject.run { } }.not_to raise_error + end + end end |