diff options
Diffstat (limited to 'spec/lib/gitlab/database')
30 files changed, 1275 insertions, 894 deletions
diff --git a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb index 2de784d3e16..0182e0f7651 100644 --- a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb @@ -124,4 +124,84 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d end end end + + describe '#split_and_retry!' do + let!(:job) { create(:batched_background_migration_job, batch_size: 10, min_value: 6, max_value: 15, status: :failed, attempts: 3) } + + context 'when job can be split' do + 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 'sets the correct attributes' do + expect { job.split_and_retry! }.to change { described_class.count }.by(1) + + expect(job).to have_attributes( + min_value: 6, + max_value: 10, + batch_size: 5, + status: 'failed', + attempts: 0, + started_at: nil, + finished_at: nil, + metrics: {} + ) + + new_job = described_class.last + + expect(new_job).to have_attributes( + batched_background_migration_id: job.batched_background_migration_id, + min_value: 11, + max_value: 15, + batch_size: 5, + status: 'failed', + attempts: 0, + started_at: nil, + finished_at: nil, + metrics: {} + ) + expect(new_job.created_at).not_to eq(job.created_at) + end + + it 'splits the jobs into retriable jobs' do + migration = job.batched_migration + + expect { job.split_and_retry! }.to change { migration.batched_jobs.retriable.count }.from(0).to(2) + end + end + + context 'when job is not failed' do + let!(:job) { create(:batched_background_migration_job, status: :succeeded) } + + it 'raises an exception' do + expect { job.split_and_retry! }.to raise_error 'Only failed jobs can be split' + end + end + + context 'when batch size is already 1' do + let!(:job) { create(:batched_background_migration_job, batch_size: 1, status: :failed) } + + it 'raises an exception' do + expect { job.split_and_retry! }.to raise_error 'Job cannot be split further' + end + end + + context 'when computed midpoint is larger than the max value of the batch' do + 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, 16]) + end + end + + it 'lowers the batch size and resets the number of attempts' do + expect { job.split_and_retry! }.not_to change { described_class.count } + + expect(job.batch_size).to eq(5) + expect(job.attempts).to eq(0) + expect(job.status).to eq('failed') + end + end + end end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb index 9f0493ab0d7..779e8e40c97 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb @@ -281,4 +281,152 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end end end + + describe '#finalize' do + let(:migration_wrapper) { Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper.new } + + let(:migration_helpers) { ActiveRecord::Migration.new } + let(:table_name) { :_batched_migrations_test_table } + let(:column_name) { :some_id } + let(:job_arguments) { [:some_id, :some_id_convert_to_bigint] } + + let(:migration_status) { :active } + + let!(:batched_migration) do + create( + :batched_background_migration, + status: migration_status, + max_value: 8, + batch_size: 2, + sub_batch_size: 1, + interval: 0, + table_name: table_name, + column_name: column_name, + job_arguments: job_arguments, + pause_ms: 0 + ) + end + + before do + migration_helpers.drop_table table_name, if_exists: true + migration_helpers.create_table table_name, id: false do |t| + t.integer :some_id, primary_key: true + t.integer :some_id_convert_to_bigint + end + + migration_helpers.execute("INSERT INTO #{table_name} VALUES (1, 1), (2, 2), (3, NULL), (4, NULL), (5, NULL), (6, NULL), (7, NULL), (8, NULL)") + end + + after do + migration_helpers.drop_table table_name, if_exists: true + end + + context 'when the migration is not yet completed' do + before do + common_attributes = { + batched_migration: batched_migration, + batch_size: 2, + sub_batch_size: 1, + pause_ms: 0 + } + + create(:batched_background_migration_job, common_attributes.merge(status: :succeeded, min_value: 1, max_value: 2)) + create(:batched_background_migration_job, common_attributes.merge(status: :pending, min_value: 3, max_value: 4)) + create(:batched_background_migration_job, common_attributes.merge(status: :failed, min_value: 5, max_value: 6, attempts: 1)) + end + + it 'completes the migration' do + expect(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:find_for_configuration) + .with('CopyColumnUsingBackgroundMigrationJob', table_name, column_name, job_arguments) + .and_return(batched_migration) + + expect(batched_migration).to receive(:finalizing!).and_call_original + + expect do + runner.finalize( + batched_migration.job_class_name, + table_name, + column_name, + job_arguments + ) + end.to change { batched_migration.reload.status }.from('active').to('finished') + + expect(batched_migration.batched_jobs).to all(be_succeeded) + + not_converted = migration_helpers.execute("SELECT * FROM #{table_name} WHERE some_id_convert_to_bigint IS NULL") + expect(not_converted.to_a).to be_empty + end + + context 'when migration fails to complete' do + it 'raises an error' do + batched_migration.batched_jobs.failed.update_all(attempts: Gitlab::Database::BackgroundMigration::BatchedJob::MAX_ATTEMPTS) + + expect do + runner.finalize( + batched_migration.job_class_name, + table_name, + column_name, + job_arguments + ) + end.to raise_error described_class::FailedToFinalize + end + end + end + + context 'when the migration is already finished' do + let(:migration_status) { :finished } + + it 'is a no-op' do + expect(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:find_for_configuration) + .with('CopyColumnUsingBackgroundMigrationJob', table_name, column_name, job_arguments) + .and_return(batched_migration) + + configuration = { + job_class_name: batched_migration.job_class_name, + table_name: table_name.to_sym, + column_name: column_name.to_sym, + job_arguments: job_arguments + } + + expect(Gitlab::AppLogger).to receive(:warn) + .with("Batched background migration for the given configuration is already finished: #{configuration}") + + expect(batched_migration).not_to receive(:finalizing!) + + runner.finalize( + batched_migration.job_class_name, + table_name, + column_name, + job_arguments + ) + end + end + + context 'when the migration does not exist' do + it 'is a no-op' do + expect(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:find_for_configuration) + .with('CopyColumnUsingBackgroundMigrationJob', table_name, column_name, [:some, :other, :arguments]) + .and_return(nil) + + configuration = { + job_class_name: batched_migration.job_class_name, + table_name: table_name.to_sym, + column_name: column_name.to_sym, + job_arguments: [:some, :other, :arguments] + } + + expect(Gitlab::AppLogger).to receive(:warn) + .with("Could not find batched background migration for the given configuration: #{configuration}") + + expect(batched_migration).not_to receive(:finalizing!) + + runner.finalize( + batched_migration.job_class_name, + table_name, + column_name, + [:some, :other, :arguments] + ) + end + end + end end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb index d881390cd52..3207e97a639 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -10,11 +10,11 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m describe '#last_job' do let!(:batched_migration) { create(:batched_background_migration) } - let!(:batched_job1) { create(:batched_background_migration_job, batched_migration: batched_migration) } - let!(:batched_job2) { create(:batched_background_migration_job, batched_migration: batched_migration) } + let!(:batched_job1) { create(:batched_background_migration_job, batched_migration: batched_migration, max_value: 1000) } + let!(:batched_job2) { create(:batched_background_migration_job, batched_migration: batched_migration, max_value: 500) } - it 'returns the most recent (in order of id) batched job' do - expect(batched_migration.last_job).to eq(batched_job2) + it 'returns the batched job with highest max_value' do + expect(batched_migration.last_job).to eq(batched_job1) end end end @@ -387,4 +387,22 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m expect(actual).to contain_exactly(migration) end end + + describe '.find_for_configuration' do + it 'returns nill if such migration does not exists' do + expect(described_class.find_for_configuration('MyJobClass', :projects, :id, [[:id], [:id_convert_to_bigint]])).to be_nil + end + + it 'returns the migration when it exists' do + migration = create( + :batched_background_migration, + job_class_name: 'MyJobClass', + table_name: :projects, + column_name: :id, + job_arguments: [[:id], [:id_convert_to_bigint]] + ) + + expect(described_class.find_for_configuration('MyJobClass', :projects, :id, [[:id], [:id_convert_to_bigint]])).to eq(migration) + end + end end diff --git a/spec/lib/gitlab/database/custom_structure_spec.rb b/spec/lib/gitlab/database/custom_structure_spec.rb deleted file mode 100644 index 04ce1e4ad9a..00000000000 --- a/spec/lib/gitlab/database/custom_structure_spec.rb +++ /dev/null @@ -1,65 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::CustomStructure do - let_it_be(:structure) { described_class.new } - let_it_be(:filepath) { Rails.root.join(described_class::CUSTOM_DUMP_FILE) } - let_it_be(:file_header) do - <<~DATA - -- this file tracks custom GitLab data, such as foreign keys referencing partitioned tables - -- more details can be found in the issue: https://gitlab.com/gitlab-org/gitlab/-/issues/201872 - DATA - end - - let(:io) { StringIO.new } - - before do - allow(File).to receive(:open).with(filepath, anything).and_yield(io) - end - - context 'when there are no partitioned_foreign_keys' do - it 'dumps a valid structure file' do - structure.dump - - expect(io.string).to eq("#{file_header}\n") - end - end - - context 'when there are partitioned_foreign_keys' do - let!(:first_fk) do - Gitlab::Database::PartitioningMigrationHelpers::PartitionedForeignKey.create( - cascade_delete: true, from_table: 'issues', from_column: 'project_id', to_table: 'projects', to_column: 'id') - end - - let!(:second_fk) do - Gitlab::Database::PartitioningMigrationHelpers::PartitionedForeignKey.create( - cascade_delete: false, from_table: 'issues', from_column: 'moved_to_id', to_table: 'issues', to_column: 'id') - end - - it 'dumps a file with the command to restore the current keys' do - structure.dump - - expect(io.string).to eq(<<~DATA) - #{file_header} - COPY partitioned_foreign_keys (id, cascade_delete, from_table, from_column, to_table, to_column) FROM STDIN; - #{first_fk.id}\ttrue\tissues\tproject_id\tprojects\tid - #{second_fk.id}\tfalse\tissues\tmoved_to_id\tissues\tid - \\. - DATA - - first_fk.destroy - io.truncate(0) - io.rewind - - structure.dump - - expect(io.string).to eq(<<~DATA) - #{file_header} - COPY partitioned_foreign_keys (id, cascade_delete, from_table, from_column, to_table, to_column) FROM STDIN; - #{second_fk.id}\tfalse\tissues\tmoved_to_id\tissues\tid - \\. - DATA - end - end -end 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 4705bb23885..b82b8d9a311 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -306,26 +306,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end end - describe '#all_caught_up?' do - it 'returns true if all hosts caught up to the write location' do - expect(lb.host_list.hosts).to all(receive(:caught_up?).with('foo').and_return(true)) - - expect(lb.all_caught_up?('foo')).to eq(true) - end - - it 'returns false if a host has not yet caught up' do - expect(lb.host_list.hosts[0]).to receive(:caught_up?) - .with('foo') - .and_return(true) - - expect(lb.host_list.hosts[1]).to receive(:caught_up?) - .with('foo') - .and_return(false) - - expect(lb.all_caught_up?('foo')).to eq(false) - end - end - describe '#retry_with_backoff' do it 'returns the value returned by the block' do value = lb.retry_with_backoff { 10 } @@ -488,7 +468,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end end - describe '#select_caught_up_hosts' do + describe '#select_up_to_date_host' do let(:location) { 'AB/12345'} let(:hosts) { lb.host_list.hosts } let(:set_host) { RequestStore[described_class::CACHE_KEY] } diff --git a/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb index 01367716518..9381ffa59fe 100644 --- a/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb @@ -71,6 +71,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do expect(app).to receive(:call).with(env).and_return(10) + expect(ActiveSupport::Notifications) + .to receive(:instrument) + .with('web_transaction_completed.load_balancing') + .and_call_original + expect(middleware.call(env)).to eq(10) end end 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 90051172fca..54050a87af0 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 @@ -5,12 +5,27 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do let(:middleware) { described_class.new } + let(:load_balancer) { double.as_null_object } + let(:worker_class) { 'TestDataConsistencyWorker' } + let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e" } } + + before do + skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check + allow(::Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer).and_return(load_balancer) + end + after do Gitlab::Database::LoadBalancing::Session.clear_session end + def run_middleware + middleware.call(worker_class, job, nil, nil) {} + end + describe '#call' do shared_context 'data consistency worker class' do |data_consistency, feature_flag| + let(:expected_consistency) { data_consistency } let(:worker_class) do Class.new do def self.name @@ -31,13 +46,23 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end end + shared_examples_for 'job data consistency' do + it "sets job data consistency" do + run_middleware + + expect(job['worker_data_consistency']).to eq(expected_consistency) + end + end + shared_examples_for 'does not pass database locations' do it 'does not pass database locations', :aggregate_failures do - middleware.call(worker_class, job, double(:queue), redis_pool) { 10 } + run_middleware expect(job['database_replica_location']).to be_nil expect(job['database_write_location']).to be_nil end + + include_examples 'job data consistency' end shared_examples_for 'mark data consistency location' do |data_consistency| @@ -45,7 +70,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do let(:location) { '0/D525E3A8' } - context 'when feature flag load_balancing_for_sidekiq is disabled' do + context 'when feature flag is disabled' do + let(:expected_consistency) { :always } + before do stub_feature_flags(load_balancing_for_test_data_consistency_worker: false) end @@ -59,12 +86,14 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'passes database_replica_location' do - expect(middleware).to receive_message_chain(:load_balancer, :host, "database_replica_location").and_return(location) + expect(load_balancer).to receive_message_chain(:host, "database_replica_location").and_return(location) - middleware.call(worker_class, job, double(:queue), redis_pool) { 10 } + run_middleware expect(job['database_replica_location']).to eq(location) end + + include_examples 'job data consistency' end context 'when write was performed' do @@ -73,12 +102,14 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'passes primary write location', :aggregate_failures do - expect(middleware).to receive_message_chain(:load_balancer, :primary_write_location).and_return(location) + expect(load_balancer).to receive(:primary_write_location).and_return(location) - middleware.call(worker_class, job, double(:queue), redis_pool) { 10 } + run_middleware expect(job['database_write_location']).to eq(location) end + + include_examples 'job data consistency' end end @@ -89,7 +120,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'does not set database locations again' do - middleware.call(worker_class, job, double(:queue), redis_pool) { 10 } + run_middleware expect(job[provided_database_location]).to eq(old_location) expect(job[other_location]).to be_nil @@ -101,8 +132,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", provided_database_location => old_location } } before do - allow(middleware).to receive_message_chain(:load_balancer, :primary_write_location).and_return(new_location) - allow(middleware).to receive_message_chain(:load_balancer, :database_replica_location).and_return(new_location) + 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 context "when write was performed" do @@ -114,24 +145,16 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end end - let(:queue) { 'default' } - let(:redis_pool) { Sidekiq.redis_pool } - let(:worker_class) { 'TestDataConsistencyWorker' } - let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e" } } - - before do - skip_feature_flags_yaml_validation - skip_default_enabled_yaml_check - 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 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 b7cd0caa922..14f240cd159 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 @@ -5,6 +5,19 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do let(:middleware) { described_class.new } + let(:load_balancer) { double.as_null_object } + + let(:worker) { worker_class.new } + let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'database_replica_location' => '0/D525E3A8' } } + + before do + skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check + allow(::Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer).and_return(load_balancer) + + replication_lag!(false) + end + after do Gitlab::Database::LoadBalancing::Session.clear_session end @@ -31,30 +44,34 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do end end - shared_examples_for 'stick to the primary' do + shared_examples_for 'load balancing strategy' do |strategy| + it "sets load balancing strategy to #{strategy}" do + run_middleware do + expect(job['load_balancing_strategy']).to eq(strategy) + end + end + end + + shared_examples_for 'stick to the primary' do |expected_strategy| it 'sticks to the primary' do - middleware.call(worker, job, double(:queue)) do + run_middleware do expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).to be_truthy end end + + include_examples 'load balancing strategy', expected_strategy end - shared_examples_for 'replica is up to date' do |location, data_consistency| + shared_examples_for 'replica is up to date' do |location, expected_strategy| it 'does not stick to the primary', :aggregate_failures do expect(middleware).to receive(:replica_caught_up?).with(location).and_return(true) - middleware.call(worker, job, double(:queue)) do + run_middleware do expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).not_to be_truthy end - - expect(job[:database_chosen]).to eq('replica') end - it "updates job hash with data_consistency :#{data_consistency}" do - middleware.call(worker, job, double(:queue)) do - expect(job).to include(data_consistency: data_consistency.to_s) - end - end + include_examples 'load balancing strategy', expected_strategy end shared_examples_for 'sticks based on data consistency' do |data_consistency| @@ -65,7 +82,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do stub_feature_flags(load_balancing_for_test_data_consistency_worker: false) end - include_examples 'stick to the primary' + include_examples 'stick to the primary', 'primary' end context 'when database replica location is set' do @@ -75,7 +92,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do allow(middleware).to receive(:replica_caught_up?).and_return(true) end - it_behaves_like 'replica is up to date', '0/D525E3A8', data_consistency + it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica' end context 'when database primary location is set' do @@ -85,39 +102,26 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do allow(middleware).to receive(:replica_caught_up?).and_return(true) end - it_behaves_like 'replica is up to date', '0/D525E3A8', data_consistency + it_behaves_like 'replica is up to date', '0/D525E3A8', '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', nil + it_behaves_like 'stick to the primary', 'primary_no_wal' end end - let(:queue) { 'default' } - let(:redis_pool) { Sidekiq.redis_pool } - let(:worker) { worker_class.new } - let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'database_replica_location' => '0/D525E3A8' } } - let(:block) { 10 } - - before do - skip_feature_flags_yaml_validation - skip_default_enabled_yaml_check - allow(middleware).to receive(:clear) - allow(Gitlab::Database::LoadBalancing::Session.current).to receive(:performed_write?).and_return(true) - end - context 'when worker class does not include ApplicationWorker' do let(:worker) { ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper.new } - include_examples 'stick to the primary' + include_examples 'stick to the primary', 'primary' end context 'when worker data consistency is :always' do include_context 'data consistency worker class', :always, :load_balancing_for_test_data_consistency_worker - include_examples 'stick to the primary' + include_examples 'stick to the primary', 'primary' end context 'when worker data consistency is :delayed' do @@ -125,8 +129,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do context 'when replica is not up to date' do before do - allow(::Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :release_host) - allow(::Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :select_up_to_date_host).and_return(false) + replication_lag!(true) end around do |example| @@ -137,38 +140,34 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do end context 'when job is executed first' do - it 'raise an error and retries', :aggregate_failures do + it 'raises an error and retries', :aggregate_failures do expect do process_job(job) end.to raise_error(Sidekiq::JobRetry::Skip) expect(job['error_class']).to eq('Gitlab::Database::LoadBalancing::SidekiqServerMiddleware::JobReplicaNotUpToDate') - expect(job[:database_chosen]).to eq('retry') end + + include_examples 'load balancing strategy', 'retry' end context 'when job is retried' do - it 'stick to the primary', :aggregate_failures do + before do expect do process_job(job) end.to raise_error(Sidekiq::JobRetry::Skip) - - process_job(job) - expect(job[:database_chosen]).to eq('primary') end - end - context 'replica selection mechanism feature flag rollout' do - before do - stub_feature_flags(sidekiq_load_balancing_rotate_up_to_date_replica: false) + context 'and replica still lagging behind' do + include_examples 'stick to the primary', 'primary' end - it 'uses different implmentation' do - expect(::Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :host, :caught_up?).and_return(false) + context 'and replica is now up-to-date' do + before do + replication_lag!(false) + end - expect do - process_job(job) - end.to raise_error(Sidekiq::JobRetry::Skip) + it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica_retried' end end end @@ -182,20 +181,24 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do allow(middleware).to receive(:replica_caught_up?).and_return(false) end - include_examples 'stick to the primary' - - it 'updates job hash with primary database chosen', :aggregate_failures do - expect { |b| middleware.call(worker, job, double(:queue), &b) }.to yield_control - - expect(job[:database_chosen]).to eq('primary') - end + include_examples 'stick to the primary', 'primary' end end end def process_job(job) - Sidekiq::JobRetry.new.local(worker_class, job, queue) do + Sidekiq::JobRetry.new.local(worker_class, job, 'default') do worker_class.process_job(job) end end + + def run_middleware + middleware.call(worker, job, double(:queue)) { yield } + rescue described_class::JobReplicaNotUpToDate + # we silence errors here that cause the job to retry + end + + def replication_lag!(exists) + allow(load_balancer).to receive(:select_up_to_date_host).and_return(!exists) + end end diff --git a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb index bf4e3756e0e..53445d73756 100644 --- a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb @@ -46,41 +46,68 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do describe '.all_caught_up?' do let(:lb) { double(:lb) } + let(:last_write_location) { 'foo' } before do allow(described_class).to receive(:load_balancer).and_return(lb) - end - it 'returns true if no write location could be found' do allow(described_class).to receive(:last_write_location_for) .with(:user, 42) - .and_return(nil) + .and_return(last_write_location) + end + + context 'when no write location could be found' do + let(:last_write_location) { nil } - expect(lb).not_to receive(:all_caught_up?) + it 'returns true' do + allow(described_class).to receive(:last_write_location_for) + .with(:user, 42) + .and_return(nil) + + expect(lb).not_to receive(:select_up_to_date_host) - expect(described_class.all_caught_up?(:user, 42)).to eq(true) + expect(described_class.all_caught_up?(:user, 42)).to eq(true) + end end - it 'returns true, and unsticks if all secondaries have caught up' do - allow(described_class).to receive(:last_write_location_for) - .with(:user, 42) - .and_return('foo') + context 'when all secondaries have caught up' do + before do + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) + end - allow(lb).to receive(:all_caught_up?).with('foo').and_return(true) + it 'returns true, and unsticks' do + expect(described_class).to receive(:unstick).with(:user, 42) - expect(described_class).to receive(:unstick).with(:user, 42) + expect(described_class.all_caught_up?(:user, 42)).to eq(true) + end + + it 'notifies with the proper event payload' do + expect(ActiveSupport::Notifications) + .to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: true }) + .and_call_original - expect(described_class.all_caught_up?(:user, 42)).to eq(true) + described_class.all_caught_up?(:user, 42) + end end - it 'return false if the secondaries have not yet caught up' do - allow(described_class).to receive(:last_write_location_for) - .with(:user, 42) - .and_return('foo') + context 'when the secondaries have not yet caught up' do + before do + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) + end + + it 'returns false' do + expect(described_class.all_caught_up?(:user, 42)).to eq(false) + end - allow(lb).to receive(:all_caught_up?).with('foo').and_return(false) + it 'notifies with the proper event payload' do + expect(ActiveSupport::Notifications) + .to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: false }) + .and_call_original - expect(described_class.all_caught_up?(:user, 42)).to eq(false) + described_class.all_caught_up?(:user, 42) + end end end @@ -96,7 +123,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do .with(:user, 42) .and_return(nil) - expect(lb).not_to receive(:all_caught_up?) + expect(lb).not_to receive(:select_up_to_date_host) described_class.unstick_or_continue_sticking(:user, 42) end @@ -106,7 +133,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do .with(:user, 42) .and_return('foo') - allow(lb).to receive(:all_caught_up?).with('foo').and_return(true) + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) expect(described_class).to receive(:unstick).with(:user, 42) @@ -118,7 +145,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do .with(:user, 42) .and_return('foo') - allow(lb).to receive(:all_caught_up?).with('foo').and_return(false) + allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) expect(Gitlab::Database::LoadBalancing::Session.current) .to receive(:use_primary!) @@ -298,10 +325,22 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do end it 'returns true, selects hosts, and unsticks if any secondary has caught up' do - expect(lb).to receive(:select_caught_up_hosts).and_return(true) + expect(lb).to receive(:select_up_to_date_host).and_return(true) expect(described_class).to receive(:unstick).with(:project, 42) expect(described_class.select_caught_up_replicas(:project, 42)).to be true end + + context 'when :load_balancing_refine_load_balancer_methods FF is disabled' do + before do + stub_feature_flags(load_balancing_refine_load_balancer_methods: false) + end + + it 'returns true, selects hosts, and unsticks if any secondary has caught up' do + expect(lb).to receive(:select_caught_up_hosts).and_return(true) + expect(described_class).to receive(:unstick).with(:project, 42) + expect(described_class.select_caught_up_replicas(:project, 42)).to be true + end + end end end end diff --git a/spec/lib/gitlab/database/load_balancing_spec.rb b/spec/lib/gitlab/database/load_balancing_spec.rb index e7de7f2b43b..94717a10492 100644 --- a/spec/lib/gitlab/database/load_balancing_spec.rb +++ b/spec/lib/gitlab/database/load_balancing_spec.rb @@ -142,10 +142,10 @@ RSpec.describe Gitlab::Database::LoadBalancing do expect(described_class.enable?).to eq(false) end - it 'returns false when Sidekiq is being used' do + it 'returns true when Sidekiq is being used' do allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true) - expect(described_class.enable?).to eq(false) + expect(described_class.enable?).to eq(true) end it 'returns false when running inside a Rake task' do @@ -170,18 +170,6 @@ RSpec.describe Gitlab::Database::LoadBalancing do expect(described_class.enable?).to eq(true) end - - context 'when ENABLE_LOAD_BALANCING_FOR_SIDEKIQ environment variable is set' do - before do - stub_env('ENABLE_LOAD_BALANCING_FOR_SIDEKIQ', 'true') - end - - it 'returns true when Sidekiq is being used' do - allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true) - - expect(described_class.enable?).to eq(true) - end - end end describe '.configured?' do diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index f0ea07646fb..8e25f9249fe 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -379,6 +379,37 @@ RSpec.describe Gitlab::Database::MigrationHelpers do allow(model).to receive(:transaction_open?).and_return(false) end + context 'target column' do + it 'defaults to (id) when no custom target column is provided' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/statement_timeout/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) + + expect(model).to receive(:execute).with(/REFERENCES users \(id\)/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id) + end + + it 'references the custom taget column when provided' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/statement_timeout/) + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) + + expect(model).to receive(:execute).with(/REFERENCES users \(id_convert_to_bigint\)/) + + model.add_concurrent_foreign_key(:projects, :users, + column: :user_id, + target_column: :id_convert_to_bigint) + end + end + context 'ON DELETE statements' do context 'on_delete: :nullify' do it 'appends ON DELETE SET NULL statement' do @@ -450,7 +481,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:foreign_key_exists?).with(:projects, :users, column: :user_id, on_delete: :cascade, - name: name).and_return(true) + name: name, + primary_key: :id).and_return(true) expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/) expect(model).to receive(:execute).with(/VALIDATE CONSTRAINT/) @@ -479,6 +511,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do it 'does not create a new foreign key' do expect(model).to receive(:foreign_key_exists?).with(:projects, :users, name: :foo, + primary_key: :id, on_delete: :cascade, column: :user_id).and_return(true) @@ -583,7 +616,15 @@ RSpec.describe Gitlab::Database::MigrationHelpers do describe '#foreign_key_exists?' do before do - key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(:projects, :users, { column: :non_standard_id, name: :fk_projects_users_non_standard_id, on_delete: :cascade }) + key = ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + :projects, :users, + { + column: :non_standard_id, + name: :fk_projects_users_non_standard_id, + on_delete: :cascade, + primary_key: :id + } + ) allow(model).to receive(:foreign_keys).with(:projects).and_return([key]) end @@ -612,6 +653,11 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(model.foreign_key_exists?(:projects, target_table, column: :user_id)).to be_falsey end + it 'compares by target column name if given' do + expect(model.foreign_key_exists?(:projects, target_table, primary_key: :user_id)).to be_falsey + expect(model.foreign_key_exists?(:projects, target_table, primary_key: :id)).to be_truthy + end + it 'compares by foreign key name if given' do expect(model.foreign_key_exists?(:projects, target_table, name: :non_existent_foreign_key_name)).to be_falsey end @@ -2007,7 +2053,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do job_class_name: 'CopyColumnUsingBackgroundMigrationJob', table_name: :events, column_name: :id, - job_arguments: [[:id], [:id_convert_to_bigint]] + job_arguments: [["id"], ["id_convert_to_bigint"]] } end @@ -2017,7 +2063,16 @@ RSpec.describe Gitlab::Database::MigrationHelpers do create(:batched_background_migration, configuration.merge(status: :active)) expect { ensure_batched_background_migration_is_finished } - .to raise_error "Expected batched background migration for the given configuration to be marked as 'finished', but it is 'active': #{configuration}" + .to raise_error "Expected batched background migration for the given configuration to be marked as 'finished', but it is 'active':" \ + "\t#{configuration}" \ + "\n\n" \ + "Finalize it manualy by running" \ + "\n\n" \ + "\tsudo gitlab-rake gitlab:background_migrations:finalize[CopyColumnUsingBackgroundMigrationJob,events,id,'[[\"id\"]\\, [\"id_convert_to_bigint\"]]']" \ + "\n\n" \ + "For more information, check the documentation" \ + "\n\n" \ + "\thttps://docs.gitlab.com/ee/user/admin_area/monitoring/background_migrations.html#database-migrations-failing-because-of-batched-background-migration-not-finished" end it 'does not raise error when migration exists and is marked as finished' do @@ -2153,21 +2208,41 @@ RSpec.describe Gitlab::Database::MigrationHelpers do buffer.rewind expect(buffer.read).to include("\"class\":\"#{model.class}\"") end + + using RSpec::Parameterized::TableSyntax + + 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 end describe '#backfill_iids' do include MigrationsHelpers - before do - stub_const('Issue', Class.new(ActiveRecord::Base)) - - Issue.class_eval do + let(:issue_class) do + Class.new(ActiveRecord::Base) do include AtomicInternalId self.table_name = 'issues' self.inheritance_column = :_type_disabled - belongs_to :project, class_name: "::Project" + belongs_to :project, class_name: "::Project", inverse_of: nil has_internal_id :iid, scope: :project, @@ -2190,7 +2265,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.backfill_iids('issues') - issue = Issue.create!(project_id: project.id) + issue = issue_class.create!(project_id: project.id) expect(issue.iid).to eq(1) end @@ -2201,7 +2276,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.backfill_iids('issues') - issue_b = Issue.create!(project_id: project.id) + issue_b = issue_class.create!(project_id: project.id) expect(issue_a.reload.iid).to eq(1) expect(issue_b.iid).to eq(2) @@ -2216,8 +2291,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.backfill_iids('issues') - issue_a = Issue.create!(project_id: project_a.id) - issue_b = Issue.create!(project_id: project_b.id) + issue_a = issue_class.create!(project_id: project_a.id) + issue_b = issue_class.create!(project_id: project_b.id) expect(issue_a.iid).to eq(2) expect(issue_b.iid).to eq(3) @@ -2231,7 +2306,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.backfill_iids('issues') - issue_b = Issue.create!(project_id: project_b.id) + issue_b = issue_class.create!(project_id: project_b.id) expect(issue_a.reload.iid).to eq(1) expect(issue_b.reload.iid).to eq(1) @@ -2951,4 +3026,12 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end end + + describe '#rename_constraint' do + it "executes the statement to rename constraint" do + expect(model).to receive(:execute).with /ALTER TABLE "test_table"\nRENAME CONSTRAINT "fk_old_name" TO "fk_new_name"/ + + model.rename_constraint(:test_table, :fk_old_name, :fk_new_name) + 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 885eef5723e..f9dca371398 100644 --- a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb @@ -71,6 +71,18 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do model.create!(created_at: Date.parse('2020-06-15')) end + context 'when pruning partitions before June 2020' do + subject { described_class.new(model, partitioning_key, retain_for: 1.month).missing_partitions } + + it 'does not include the missing partition from May 2020 because it would be dropped' do + expect(subject).not_to include(Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01')) + end + + it 'detects the missing partition for 1 month ago (July 2020)' do + expect(subject).to include(Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-07-01', '2020-08-01')) + end + end + it 'detects the gap and the missing partition in May 2020' do expect(subject).to include(Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01')) end @@ -108,6 +120,19 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do SQL end + context 'when pruning partitions before June 2020' do + subject { described_class.new(model, partitioning_key, retain_for: 1.month).missing_partitions } + + it 'detects exactly the set of partitions from June 2020 to March 2021' do + months = %w[2020-07-01 2020-08-01 2020-09-01 2020-10-01 2020-11-01 2020-12-01 2021-01-01 2021-02-01 2021-03-01] + expected = months[..-2].zip(months.drop(1)).map do |(from, to)| + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, from, to) + end + + expect(subject).to match_array(expected) + end + end + it 'detects the missing catch-all partition at the beginning' do expect(subject).to include(Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-08-01')) end @@ -150,4 +175,100 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do end end end + + describe '#extra_partitions' do + let(:model) do + Class.new(ActiveRecord::Base) do + self.table_name = 'partitioned_test' + self.primary_key = :id + end + end + + let(:partitioning_key) { :created_at } + let(:table_name) { :partitioned_test } + + around do |example| + travel_to(Date.parse('2020-08-22')) { example.run } + end + + describe 'with existing partitions' do + before do + ActiveRecord::Base.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); + + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.partitioned_test_000000 + PARTITION OF #{table_name} + FOR VALUES FROM (MINVALUE) TO ('2020-05-01'); + + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.partitioned_test_202005 + PARTITION OF #{table_name} + FOR VALUES FROM ('2020-05-01') TO ('2020-06-01'); + + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.partitioned_test_202006 + PARTITION OF #{table_name} + FOR VALUES FROM ('2020-06-01') TO ('2020-07-01') + SQL + end + + context 'without a time retention policy' do + subject { described_class.new(model, partitioning_key).extra_partitions } + + it 'has no extra partitions to prune' do + expect(subject).to eq([]) + end + end + + context 'with a time retention policy that excludes no partitions' do + subject { described_class.new(model, partitioning_key, retain_for: 4.months).extra_partitions } + + it 'has no extra partitions to prune' do + expect(subject).to eq([]) + end + end + + context 'with a time retention policy of 3 months' do + subject { described_class.new(model, partitioning_key, retain_for: 3.months).extra_partitions } + + it 'prunes the unbounded partition ending 2020-05-01' do + min_value_to_may = Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', + partition_name: 'partitioned_test_000000') + + expect(subject).to contain_exactly(min_value_to_may) + end + + context 'when the feature flag is toggled off' do + before do + stub_feature_flags(partition_pruning_dry_run: false) + end + + it 'is empty' do + expect(subject).to eq([]) + end + end + end + + context 'with a time retention policy of 2 months' do + subject { described_class.new(model, partitioning_key, retain_for: 2.months).extra_partitions } + + it 'prunes the unbounded partition and the partition for May-June' 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 + + context 'when the feature flag is toggled off' do + before do + stub_feature_flags(partition_pruning_dry_run: false) + end + + it 'is empty' do + expect(subject).to eq([]) + end + end + end + end + end end diff --git a/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb b/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb deleted file mode 100644 index ec89f2ed61c..00000000000 --- a/spec/lib/gitlab/database/partitioning/partition_creator_spec.rb +++ /dev/null @@ -1,96 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::Partitioning::PartitionCreator do - include Database::PartitioningHelpers - include ExclusiveLeaseHelpers - - 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 - - describe '#create_partitions (mocked)' do - subject { described_class.new(models).create_partitions } - - let(:models) { [model] } - let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table) } - let(:partitioning_strategy) { double(missing_partitions: partitions) } - 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 - - stub_exclusive_lease(described_class::LEASE_KEY % table, timeout: described_class::LEASE_TIMEOUT) - end - - let(:partitions) do - [ - instance_double(Gitlab::Database::Partitioning::TimePartition, table: 'bar', partition_name: 'foo', to_sql: "SELECT 1"), - instance_double(Gitlab::Database::Partitioning::TimePartition, table: 'bar', partition_name: 'foo2', to_sql: "SELECT 2") - ] - 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) - - subject - 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) } - let(:strategy2) { double('strategy2', missing_partitions: partitions) } - - 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) - - subject - end - end - end - - describe '#create_partitions' do - subject { described_class.new([my_model]).create_partitions } - - let(:connection) { ActiveRecord::Base.connection } - let(:my_model) do - Class.new(ApplicationRecord) do - include PartitionedTable - - self.table_name = 'my_model_example_table' - - partitioned_by :created_at, strategy: :monthly - end - end - - before do - connection.execute(<<~SQL) - CREATE TABLE my_model_example_table - (id serial not null, created_at timestamptz not null, primary key (id, created_at)) - PARTITION BY RANGE (created_at); - SQL - end - - it 'creates partitions' do - expect { subject }.to change { find_partitions(my_model.table_name, schema: Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA).size }.from(0) - - subject - end - end -end diff --git a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb new file mode 100644 index 00000000000..903a41d6dd2 --- /dev/null +++ b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Partitioning::PartitionManager do + include Database::PartitioningHelpers + include Database::TableSchemaHelpers + include ExclusiveLeaseHelpers + + 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 } + + let(:models) { [model] } + let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table) } + let(:partitioning_strategy) { double(missing_partitions: partitions, extra_partitions: []) } + 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 + + stub_exclusive_lease(described_class::MANAGEMENT_LEASE_KEY % table, timeout: described_class::LEASE_TIMEOUT) + end + + let(:partitions) do + [ + instance_double(Gitlab::Database::Partitioning::TimePartition, table: 'bar', partition_name: 'foo', to_sql: "SELECT 1"), + instance_double(Gitlab::Database::Partitioning::TimePartition, table: 'bar', partition_name: 'foo2', to_sql: "SELECT 2") + ] + 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) + + 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: []) } + + 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 + end + end + end + + context 'creating partitions' do + subject(:sync_partitions) { described_class.new([my_model]).sync_partitions } + + let(:connection) { ActiveRecord::Base.connection } + let(:my_model) do + Class.new(ApplicationRecord) do + include PartitionedTable + + self.table_name = 'my_model_example_table' + + partitioned_by :created_at, strategy: :monthly + end + end + + before do + connection.execute(<<~SQL) + CREATE TABLE my_model_example_table + (id serial not null, created_at timestamptz not null, primary key (id, created_at)) + PARTITION BY RANGE (created_at); + SQL + end + + it 'creates partitions' do + expect { sync_partitions }.to change { find_partitions(my_model.table_name, schema: Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA).size }.from(0) + end + end + + 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(:partitioning_strategy) { double(extra_partitions: extra_partitions, missing_partitions: []) } + 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) + + stub_exclusive_lease(described_class::MANAGEMENT_LEASE_KEY % table, timeout: described_class::LEASE_TIMEOUT) + end + + let(:extra_partitions) do + [ + instance_double(Gitlab::Database::Partitioning::TimePartition, table: table, partition_name: 'foo1'), + instance_double(Gitlab::Database::Partitioning::TimePartition, table: table, partition_name: 'foo2') + ] + end + + context 'with the partition_pruning_dry_run feature flag enabled' do + before do + stub_feature_flags(partition_pruning_dry_run: true) + end + + it 'detaches each extra partition' do + extra_partitions.each { |p| expect(manager).to receive(:detach_one_partition).with(p) } + + 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_dry_run feature flag disabled' do + before do + stub_feature_flags(partition_pruning_dry_run: false) + end + + it 'returns immediately' do + expect(manager).not_to receive(:detach) + + sync_partitions + end + 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 83f2436043c..a524fe681e9 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 @@ -3,192 +3,142 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::ForeignKeyHelpers do - include Database::TriggerHelpers + include Database::TableSchemaHelpers - let(:model) do - ActiveRecord::Migration.new.extend(described_class) + let(:migration) do + ActiveRecord::Migration.new.extend(Gitlab::Database::PartitioningMigrationHelpers) end - let_it_be(:connection) { ActiveRecord::Base.connection } - - let(:referenced_table) { :issues } - let(:function_name) { '_test_partitioned_foreign_keys_function' } - let(:trigger_name) { '_test_partitioned_foreign_keys_trigger' } + let(:source_table_name) { '_test_partitioned_table' } + let(:target_table_name) { '_test_referenced_table' } + let(:column_name) { "#{target_table_name}_id" } + let(:foreign_key_name) { '_test_partitioned_fk' } + let(:partition_schema) { 'gitlab_partitions_dynamic' } + let(:partition1_name) { "#{partition_schema}.#{source_table_name}_202001" } + let(:partition2_name) { "#{partition_schema}.#{source_table_name}_202002" } + let(:options) do + { + column: column_name, + name: foreign_key_name, + on_delete: :cascade, + validate: true + } + end before do - allow(model).to receive(:puts) - allow(model).to receive(:fk_function_name).and_return(function_name) - allow(model).to receive(:fk_trigger_name).and_return(trigger_name) + allow(migration).to receive(:puts) + + connection.execute(<<~SQL) + CREATE TABLE #{target_table_name} ( + id serial NOT NULL, + PRIMARY KEY (id) + ); + + CREATE TABLE #{source_table_name} ( + id serial NOT NULL, + #{column_name} int NOT NULL, + created_at timestamptz NOT NULL, + PRIMARY KEY (id, created_at) + ) PARTITION BY RANGE (created_at); + + CREATE TABLE #{partition1_name} PARTITION OF #{source_table_name} + FOR VALUES FROM ('2020-01-01') TO ('2020-02-01'); + + CREATE TABLE #{partition2_name} PARTITION OF #{source_table_name} + FOR VALUES FROM ('2020-02-01') TO ('2020-03-01'); + SQL end - describe 'adding a foreign key' do + describe '#add_concurrent_partitioned_foreign_key' do before do - allow(model).to receive(:transaction_open?).and_return(false) - end - - context 'when the table has no foreign keys' do - it 'creates a trigger function to handle the single cascade' do - model.add_partitioned_foreign_key :issue_assignees, referenced_table - - expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end - end - - context 'when the table already has foreign keys' do - context 'when the foreign key is from a different table' do - before do - model.add_partitioned_foreign_key :issue_assignees, referenced_table - end - - it 'creates a trigger function to handle the multiple cascades' do - model.add_partitioned_foreign_key :epic_issues, referenced_table - - expect_function_to_contain(function_name, - 'delete from issue_assignees where issue_id = old.id', - 'delete from epic_issues where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end - end - - context 'when the foreign key is from the same table' do - before do - model.add_partitioned_foreign_key :issues, referenced_table, column: :moved_to_id - end - - context 'when the foreign key is from a different column' do - it 'creates a trigger function to handle the multiple cascades' do - model.add_partitioned_foreign_key :issues, referenced_table, column: :duplicated_to_id - - expect_function_to_contain(function_name, - 'delete from issues where moved_to_id = old.id', - 'delete from issues where duplicated_to_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end - end - - context 'when the foreign key is from the same column' do - it 'ignores the duplicate and properly recreates the trigger function' do - model.add_partitioned_foreign_key :issues, referenced_table, column: :moved_to_id - - expect_function_to_contain(function_name, 'delete from issues where moved_to_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end - end - end - end + allow(migration).to receive(:foreign_key_exists?) + .with(source_table_name, target_table_name, anything) + .and_return(false) - context 'when the foreign key is set to nullify' do - it 'creates a trigger function that nullifies the foreign key' do - model.add_partitioned_foreign_key :issue_assignees, referenced_table, on_delete: :nullify - - expect_function_to_contain(function_name, 'update issue_assignees set issue_id = null where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end + allow(migration).to receive(:with_lock_retries).and_yield end - context 'when the referencing column is a custom value' do - it 'creates a trigger function with the correct column name' do - model.add_partitioned_foreign_key :issues, referenced_table, column: :duplicated_to_id + context 'when the foreign key does not exist on the parent table' do + it 'creates the foreign key on each partition, and the parent table' do + expect(migration).to receive(:foreign_key_exists?) + .with(source_table_name, target_table_name, **options) + .and_return(false) - expect_function_to_contain(function_name, 'delete from issues where duplicated_to_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end - end + expect(migration).to receive(:concurrent_partitioned_foreign_key_name).and_return(foreign_key_name) - context 'when the referenced column is a custom value' do - let(:referenced_table) { :user_details } + expect_add_concurrent_fk_and_call_original(partition1_name, target_table_name, **options) + expect_add_concurrent_fk_and_call_original(partition2_name, target_table_name, **options) - it 'creates a trigger function with the correct column name' do - model.add_partitioned_foreign_key :user_preferences, referenced_table, column: :user_id, primary_key: :user_id + expect(migration).to receive(:with_lock_retries).ordered.and_yield + expect(migration).to receive(:add_foreign_key) + .with(source_table_name, target_table_name, **options) + .ordered + .and_call_original - expect_function_to_contain(function_name, 'delete from user_preferences where user_id = old.user_id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - end - end + migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, column: column_name) - context 'when the given key definition is invalid' do - it 'raises an error with the appropriate message' do - expect do - model.add_partitioned_foreign_key :issue_assignees, referenced_table, column: :not_a_real_issue_id - end.to raise_error(/From column must be a valid column/) + expect_foreign_key_to_exist(source_table_name, foreign_key_name) end - end - - context 'when run inside a transaction' do - it 'raises an error' do - expect(model).to receive(:transaction_open?).and_return(true) - expect do - model.add_partitioned_foreign_key :issue_assignees, referenced_table - end.to raise_error(/can not be run inside a transaction/) + def expect_add_concurrent_fk_and_call_original(source_table_name, target_table_name, options) + expect(migration).to receive(:add_concurrent_foreign_key) + .ordered + .with(source_table_name, target_table_name, options) + .and_wrap_original do |_, source_table_name, target_table_name, options| + connection.add_foreign_key(source_table_name, target_table_name, **options) + end end end - end - context 'removing a foreign key' do - before do - allow(model).to receive(:transaction_open?).and_return(false) - end + context 'when the foreign key exists on the parent table' do + it 'does not attempt to create any foreign keys' do + expect(migration).to receive(:concurrent_partitioned_foreign_key_name).and_return(foreign_key_name) - context 'when the table has multiple foreign keys' do - before do - model.add_partitioned_foreign_key :issue_assignees, referenced_table - model.add_partitioned_foreign_key :epic_issues, referenced_table - end + expect(migration).to receive(:foreign_key_exists?) + .with(source_table_name, target_table_name, **options) + .and_return(true) - it 'creates a trigger function without the removed cascade' do - expect_function_to_contain(function_name, - 'delete from issue_assignees where issue_id = old.id', - 'delete from epic_issues where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') + expect(migration).not_to receive(:add_concurrent_foreign_key) + expect(migration).not_to receive(:with_lock_retries) + expect(migration).not_to receive(:add_foreign_key) - model.remove_partitioned_foreign_key :issue_assignees, referenced_table + migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, column: column_name) - expect_function_to_contain(function_name, 'delete from epic_issues where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') + expect_foreign_key_not_to_exist(source_table_name, foreign_key_name) end end - context 'when the table has only one remaining foreign key' do - before do - model.add_partitioned_foreign_key :issue_assignees, referenced_table + context 'when additional foreign key options are given' do + let(:options) do + { + column: column_name, + name: '_my_fk_name', + on_delete: :restrict, + validate: true + } end - it 'removes the trigger function altogether' do - expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') - - model.remove_partitioned_foreign_key :issue_assignees, referenced_table - - expect_function_not_to_exist(function_name) - expect_trigger_not_to_exist(referenced_table, trigger_name) - end - end + it 'forwards them to the foreign key helper methods' do + expect(migration).to receive(:foreign_key_exists?) + .with(source_table_name, target_table_name, **options) + .and_return(false) - context 'when the foreign key does not exist' do - before do - model.add_partitioned_foreign_key :issue_assignees, referenced_table - end + expect(migration).not_to receive(:concurrent_partitioned_foreign_key_name) - it 'ignores the invalid key and properly recreates the trigger function' do - expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') + expect_add_concurrent_fk(partition1_name, target_table_name, **options) + expect_add_concurrent_fk(partition2_name, target_table_name, **options) - model.remove_partitioned_foreign_key :issues, referenced_table, column: :moved_to_id + expect(migration).to receive(:with_lock_retries).ordered.and_yield + expect(migration).to receive(:add_foreign_key).with(source_table_name, target_table_name, **options).ordered - expect_function_to_contain(function_name, 'delete from issue_assignees where issue_id = old.id') - expect_valid_function_trigger(referenced_table, trigger_name, function_name, after: 'delete') + migration.add_concurrent_partitioned_foreign_key(source_table_name, target_table_name, + column: column_name, name: '_my_fk_name', on_delete: :restrict) end - end - - context 'when run outside a transaction' do - it 'raises an error' do - expect(model).to receive(:transaction_open?).and_return(true) - expect do - model.remove_partitioned_foreign_key :issue_assignees, referenced_table - end.to raise_error(/can not be run inside a transaction/) + def expect_add_concurrent_fk(source_table_name, target_table_name, options) + expect(migration).to receive(:add_concurrent_foreign_key) + .ordered + .with(source_table_name, target_table_name, options) end end end diff --git a/spec/lib/gitlab/database/partitioning_migration_helpers/partitioned_foreign_key_spec.rb b/spec/lib/gitlab/database/partitioning_migration_helpers/partitioned_foreign_key_spec.rb deleted file mode 100644 index a58c37f111d..00000000000 --- a/spec/lib/gitlab/database/partitioning_migration_helpers/partitioned_foreign_key_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::PartitioningMigrationHelpers::PartitionedForeignKey do - let(:foreign_key) do - described_class.new( - to_table: 'issues', - from_table: 'issue_assignees', - from_column: 'issue_id', - to_column: 'id', - cascade_delete: true) - end - - describe 'validations' do - it 'allows keys that reference valid tables and columns' do - expect(foreign_key).to be_valid - end - - it 'does not allow keys without a valid to_table' do - foreign_key.to_table = 'this_is_not_a_real_table' - - expect(foreign_key).not_to be_valid - expect(foreign_key.errors[:to_table].first).to eq('must be a valid table') - end - - it 'does not allow keys without a valid from_table' do - foreign_key.from_table = 'this_is_not_a_real_table' - - expect(foreign_key).not_to be_valid - expect(foreign_key.errors[:from_table].first).to eq('must be a valid table') - end - - it 'does not allow keys without a valid to_column' do - foreign_key.to_column = 'this_is_not_a_real_fk' - - expect(foreign_key).not_to be_valid - expect(foreign_key.errors[:to_column].first).to eq('must be a valid column') - end - - it 'does not allow keys without a valid from_column' do - foreign_key.from_column = 'this_is_not_a_real_pk' - - expect(foreign_key).not_to be_valid - expect(foreign_key.errors[:from_column].first).to eq('must be a valid column') - end - end -end diff --git a/spec/lib/gitlab/database/postgres_index_spec.rb b/spec/lib/gitlab/database/postgres_index_spec.rb index 2fda9b85c5a..e1832219ebf 100644 --- a/spec/lib/gitlab/database/postgres_index_spec.rb +++ b/spec/lib/gitlab/database/postgres_index_spec.rb @@ -22,17 +22,23 @@ RSpec.describe Gitlab::Database::PostgresIndex do it_behaves_like 'a postgres model' - describe '.regular' do - it 'only non-unique indexes' do - expect(described_class.regular).to all(have_attributes(unique: false)) - end - + describe '.reindexing_support' do it 'only non partitioned indexes' do - expect(described_class.regular).to all(have_attributes(partitioned: false)) + expect(described_class.reindexing_support).to all(have_attributes(partitioned: false)) end it 'only indexes that dont serve an exclusion constraint' do - expect(described_class.regular).to all(have_attributes(exclusion: false)) + expect(described_class.reindexing_support).to all(have_attributes(exclusion: false)) + end + + it 'only non-expression indexes' do + expect(described_class.reindexing_support).to all(have_attributes(expression: false)) + end + + it 'only btree and gist indexes' do + types = described_class.reindexing_support.map(&:type).uniq + + expect(types & %w(btree gist)).to eq(types) end end @@ -67,6 +73,34 @@ RSpec.describe Gitlab::Database::PostgresIndex do end end + describe '#relative_bloat_level' do + subject { build(:postgres_index, bloat_estimate: bloat_estimate, ondisk_size_bytes: 1024) } + + let(:bloat_estimate) { build(:postgres_index_bloat_estimate, bloat_size: 256) } + + it 'calculates the relative bloat level' do + expect(subject.relative_bloat_level).to eq(0.25) + end + end + + describe '#reset' do + subject { index.reset } + + let(:index) { described_class.by_identifier(identifier) } + + it 'calls #reload' do + expect(index).to receive(:reload).once.and_call_original + + subject + end + + it 'resets the bloat estimation' do + expect(index).to receive(:clear_memoization).with(:bloat_size).and_call_original + + subject + end + end + describe '#unique?' do it 'returns true for a unique index' do expect(find('public.bar_key')).to be_unique 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 ca9f4af9187..40e36bc02e9 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 @@ -3,33 +3,27 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PostgresqlAdapter::DumpSchemaVersionsMixin do - let(:schema_migration) { double('schema_migration', all_versions: versions) } - - let(:instance) do - Object.new.extend(described_class) - end - - before do - allow(instance).to receive(:schema_migration).and_return(schema_migration) - end - - context 'when version files exist' do - let(:versions) { %w(5 2 1000 200 4 93 2) } + let(:instance_class) do + klass = Class.new do + def dump_schema_information + original_dump_schema_information + end + + def original_dump_schema_information + end + end - it 'touches version files' do - expect(Gitlab::Database::SchemaVersionFiles).to receive(:touch_all).with(versions) + klass.prepend(described_class) - instance.dump_schema_information - end + klass end - context 'when version files do not exist' do - let(:versions) { [] } + let(:instance) { instance_class.new } - it 'does not touch version files' do - expect(Gitlab::Database::SchemaVersionFiles).not_to receive(:touch_all) + it 'calls SchemaMigrations touch_all and skips original implementation' do + expect(Gitlab::Database::SchemaMigrations).to receive(:touch_all).with(instance) + expect(instance).not_to receive(:original_dump_schema_information) - instance.dump_schema_information - end + instance.dump_schema_information end end diff --git a/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb b/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb index ea8c9e2cfd7..2a1f91b5b21 100644 --- a/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb +++ b/spec/lib/gitlab/database/postgresql_adapter/force_disconnectable_mixin_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Gitlab::Database::PostgresqlAdapter::ForceDisconnectableMixin do end end - let(:config) { Rails.application.config_for(:database).merge(pool: 1) } + let(:config) { ActiveRecord::Base.configurations.find_db_config(Rails.env).configuration_hash.merge(pool: 1) } let(:pool) { model.establish_connection(config) } it 'calls the force disconnect callback on checkin' do diff --git a/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb b/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb index e9c512f94bb..c6542aa2adb 100644 --- a/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb +++ b/spec/lib/gitlab/database/postgresql_adapter/type_map_cache_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::PostgresqlAdapter::TypeMapCache do - let(:db_config) { ActiveRecord::Base.configurations.configs_for(env_name: 'test', name: 'primary').configuration_hash } + let(:db_config) { ActiveRecord::Base.configurations.find_db_config(Rails.env).configuration_hash } let(:adapter_class) { ActiveRecord::ConnectionAdapters::PostgreSQLAdapter } before do diff --git a/spec/lib/gitlab/database/postgresql_database_tasks/load_schema_versions_mixin_spec.rb b/spec/lib/gitlab/database/postgresql_database_tasks/load_schema_versions_mixin_spec.rb new file mode 100644 index 00000000000..3e675a85999 --- /dev/null +++ b/spec/lib/gitlab/database/postgresql_database_tasks/load_schema_versions_mixin_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::PostgresqlDatabaseTasks::LoadSchemaVersionsMixin do + let(:instance_class) do + klass = Class.new do + def structure_load + original_structure_load + end + + def original_structure_load + end + end + + klass.prepend(described_class) + + klass + end + + let(:instance) { instance_class.new } + + it 'calls SchemaMigrations load_all' do + connection = double('connection') + allow(instance).to receive(:connection).and_return(connection) + + expect(instance).to receive(:original_structure_load).ordered + expect(Gitlab::Database::SchemaMigrations).to receive(:load_all).with(connection).ordered + + instance.structure_load + end +end diff --git a/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb b/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb deleted file mode 100644 index d9077969003..00000000000 --- a/spec/lib/gitlab/database/reindexing/concurrent_reindex_spec.rb +++ /dev/null @@ -1,303 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::Reindexing::ConcurrentReindex, '#perform' do - subject { described_class.new(index, logger: logger) } - - let(:table_name) { '_test_reindex_table' } - let(:column_name) { '_test_column' } - let(:index_name) { '_test_reindex_index' } - let(:index) { instance_double(Gitlab::Database::PostgresIndex, indexrelid: 42, name: index_name, schema: 'public', tablename: table_name, partitioned?: false, unique?: false, exclusion?: false, expression?: false, definition: 'CREATE INDEX _test_reindex_index ON public._test_reindex_table USING btree (_test_column)') } - let(:logger) { double('logger', debug: nil, info: nil, error: nil ) } - let(:connection) { ActiveRecord::Base.connection } - - before do - connection.execute(<<~SQL) - CREATE TABLE #{table_name} ( - id serial NOT NULL PRIMARY KEY, - #{column_name} integer NOT NULL); - - CREATE INDEX #{index.name} ON #{table_name} (#{column_name}); - SQL - end - - context 'when the index is unique' do - before do - allow(index).to receive(:unique?).and_return(true) - end - - it 'raises an error' do - expect do - subject.perform - end.to raise_error(described_class::ReindexError, /UNIQUE indexes are currently not supported/) - end - end - - context 'when the index is partitioned' do - before do - allow(index).to receive(:partitioned?).and_return(true) - end - - it 'raises an error' do - expect do - subject.perform - end.to raise_error(described_class::ReindexError, /partitioned indexes are currently not supported/) - end - end - - context 'when the index serves an exclusion constraint' do - before do - allow(index).to receive(:exclusion?).and_return(true) - end - - it 'raises an error' do - expect do - subject.perform - end.to raise_error(described_class::ReindexError, /indexes serving an exclusion constraint are currently not supported/) - end - end - - context 'when the index is a lingering temporary index from a previous reindexing run' do - context 'with the temporary index prefix' do - let(:index_name) { 'tmp_reindex_something' } - - it 'raises an error' do - expect do - subject.perform - end.to raise_error(described_class::ReindexError, /left-over temporary index/) - end - end - - context 'with the replaced index prefix' do - let(:index_name) { 'old_reindex_something' } - - it 'raises an error' do - expect do - subject.perform - end.to raise_error(described_class::ReindexError, /left-over temporary index/) - end - end - end - - context 'replacing the original index with a rebuilt copy' do - let(:replacement_name) { 'tmp_reindex_42' } - let(:replaced_name) { 'old_reindex_42' } - - let(:create_index) { "CREATE INDEX CONCURRENTLY #{replacement_name} ON public.#{table_name} USING btree (#{column_name})" } - let(:drop_index) do - <<~SQL - DROP INDEX CONCURRENTLY - IF EXISTS "public"."#{replacement_name}" - SQL - end - - let!(:original_index) { find_index_create_statement } - - it 'integration test: executing full index replacement without mocks' do - allow(connection).to receive(:execute).and_wrap_original do |method, sql| - method.call(sql.sub(/CONCURRENTLY/, '')) - end - - subject.perform - - check_index_exists - end - - context 'mocked specs' do - before do - allow(subject).to receive(:connection).and_return(connection) - allow(connection).to receive(:execute).and_call_original - end - - it 'replaces the existing index with an identical index' do - expect(connection).to receive(:execute).with('SET statement_timeout TO \'32400s\'') - - expect_to_execute_concurrently_in_order(create_index) - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield - end - - expect_index_rename(index.name, replaced_name) - expect_index_rename(replacement_name, index.name) - expect_index_rename(replaced_name, replacement_name) - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: false).and_yield - end - - expect_to_execute_concurrently_in_order(drop_index) - - subject.perform - - check_index_exists - end - - context 'for expression indexes' do - before do - allow(index).to receive(:expression?).and_return(true) - end - - it 'rebuilds table statistics before dropping the original index' do - expect(connection).to receive(:execute).with('SET statement_timeout TO \'32400s\'') - - expect_to_execute_concurrently_in_order(create_index) - - expect_to_execute_concurrently_in_order(<<~SQL) - ANALYZE "#{index.schema}"."#{index.tablename}" - SQL - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield - end - - expect_index_rename(index.name, replaced_name) - expect_index_rename(replacement_name, index.name) - expect_index_rename(replaced_name, replacement_name) - - expect_index_drop(drop_index) - - subject.perform - - check_index_exists - end - end - - context 'when a dangling index is left from a previous run' do - before do - connection.execute("CREATE INDEX #{replacement_name} ON #{table_name} (#{column_name})") - end - - it 'replaces the existing index with an identical index' do - expect_index_drop(drop_index) - expect_to_execute_concurrently_in_order(create_index) - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield - end - - expect_index_rename(index.name, replaced_name) - expect_index_rename(replacement_name, index.name) - expect_index_rename(replaced_name, replacement_name) - - expect_index_drop(drop_index) - - subject.perform - - check_index_exists - end - end - - context 'when it fails to create the replacement index' do - it 'safely cleans up and signals the error' do - expect(connection).to receive(:execute).with(create_index).ordered - .and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout') - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: false).and_yield - end - - expect_to_execute_concurrently_in_order(drop_index) - - expect { subject.perform }.to raise_error(ActiveRecord::ConnectionTimeoutError, /connect timeout/) - - check_index_exists - end - end - - context 'when the replacement index is not valid' do - it 'safely cleans up and signals the error' do - replacement_index = double('replacement index', valid_index?: false) - allow(Gitlab::Database::PostgresIndex).to receive(:find_by).with(schema: 'public', name: replacement_name).and_return(nil, replacement_index) - - expect_to_execute_concurrently_in_order(create_index) - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: false).and_yield - end - - expect_to_execute_concurrently_in_order(drop_index) - - expect { subject.perform }.to raise_error(described_class::ReindexError, /replacement index was created as INVALID/) - - check_index_exists - end - end - - context 'when a database error occurs while swapping the indexes' do - it 'safely cleans up and signals the error' do - replacement_index = double('replacement index', valid_index?: true) - allow(Gitlab::Database::PostgresIndex).to receive(:find_by).with(schema: 'public', name: replacement_name).and_return(nil, replacement_index) - - expect_to_execute_concurrently_in_order(create_index) - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: true).and_yield - end - - expect_index_rename(index.name, replaced_name).and_raise(ActiveRecord::ConnectionTimeoutError, 'connect timeout') - - expect_index_drop(drop_index) - - expect { subject.perform }.to raise_error(ActiveRecord::ConnectionTimeoutError, /connect timeout/) - - check_index_exists - end - end - - context 'when with_lock_retries fails to acquire the lock' do - it 'safely cleans up and signals the error' do - expect_to_execute_concurrently_in_order(create_index) - - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: true) - .and_raise(::Gitlab::Database::WithLockRetries::AttemptsExhaustedError, 'exhausted') - end - - expect_index_drop(drop_index) - - expect { subject.perform }.to raise_error(::Gitlab::Database::WithLockRetries::AttemptsExhaustedError, /exhausted/) - - check_index_exists - end - end - end - end - - def expect_to_execute_concurrently_in_order(sql) - # Indexes cannot be created CONCURRENTLY in a transaction. Since the tests are wrapped in transactions, - # verify the original call but pass through the non-concurrent form. - expect(connection).to receive(:execute).with(sql).ordered.and_wrap_original do |method, sql| - method.call(sql.sub(/CONCURRENTLY/, '')) - end - end - - def expect_index_rename(from, to) - expect(connection).to receive(:execute).with(<<~SQL).ordered - ALTER INDEX "public"."#{from}" - RENAME TO "#{to}" - SQL - end - - def expect_index_drop(drop_index) - expect_next_instance_of(::Gitlab::Database::WithLockRetries) do |instance| - expect(instance).to receive(:run).with(raise_on_exhaustion: false).and_yield - end - - expect_to_execute_concurrently_in_order(drop_index) - end - - def find_index_create_statement - ActiveRecord::Base.connection.select_value(<<~SQL) - SELECT indexdef - FROM pg_indexes - WHERE schemaname = 'public' - AND indexname = #{ActiveRecord::Base.connection.quote(index.name)} - SQL - end - - def check_index_exists - expect(find_index_create_statement).to eq(original_index) - end -end diff --git a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb index ae6362ba812..085fd3061ad 100644 --- a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb +++ b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb @@ -9,16 +9,9 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do describe '.perform' do subject { described_class.new(index, notifier).perform } - before do - swapout_view_for_table(:postgres_indexes) - - allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer) - allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) - end - let(:index) { create(:postgres_index) } let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) } - let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ConcurrentReindex, perform: nil) } + let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ReindexConcurrently, perform: nil) } let(:action) { create(:reindex_action, index: index) } let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) } @@ -26,6 +19,13 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do let(:lease_timeout) { 1.day } let(:uuid) { 'uuid' } + before do + swapout_view_for_table(:postgres_indexes) + + allow(Gitlab::Database::Reindexing::ReindexConcurrently).to receive(:new).with(index).and_return(reindexer) + allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) + end + context 'locking' do it 'acquires a lock while reindexing' do expect(lease).to receive(:try_obtain).ordered.and_return(uuid) @@ -39,7 +39,7 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do it 'does not perform reindexing actions if lease is not granted' do expect(lease).to receive(:try_obtain).ordered.and_return(false) - expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new) + expect(Gitlab::Database::Reindexing::ReindexConcurrently).not_to receive(:new) subject end diff --git a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb index 4466679a099..ee3f2b1b415 100644 --- a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb +++ b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb @@ -10,20 +10,50 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection do before do swapout_view_for_table(:postgres_index_bloat_estimates) swapout_view_for_table(:postgres_indexes) + + create_list(:postgres_index, 10, ondisk_size_bytes: 10.gigabytes).each_with_index do |index, i| + create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 2.gigabyte * (i + 1)) + end end def execute(sql) ActiveRecord::Base.connection.execute(sql) end - it 'orders by highest bloat first' do - create_list(:postgres_index, 10).each_with_index do |index, i| - create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 1.megabyte * i) - end + it 'orders by highest relative bloat first' do + expected = Gitlab::Database::PostgresIndex.all.sort_by(&:relative_bloat_level).reverse.map(&:name) + + expect(subject.map(&:name)).to eq(expected) + end + + it 'excludes indexes with a relative bloat level below 20%' do + excluded = create( + :postgres_index_bloat_estimate, + index: create(:postgres_index, ondisk_size_bytes: 10.gigabytes), + bloat_size_bytes: 1.9.gigabyte # 19% relative index bloat + ) - expected = Gitlab::Database::PostgresIndexBloatEstimate.order(bloat_size_bytes: :desc).map(&:index) + expect(subject).not_to include(excluded.index) + end + + it 'excludes indexes smaller than 1 GB ondisk size' do + excluded = create( + :postgres_index_bloat_estimate, + index: create(:postgres_index, ondisk_size_bytes: 0.99.gigabytes), + bloat_size_bytes: 0.8.gigabyte + ) + + expect(subject).not_to include(excluded.index) + end + + it 'excludes indexes larger than 100 GB ondisk size' do + excluded = create( + :postgres_index_bloat_estimate, + index: create(:postgres_index, ondisk_size_bytes: 101.gigabytes), + bloat_size_bytes: 25.gigabyte + ) - expect(subject).to eq(expected) + expect(subject).not_to include(excluded.index) end context 'with time frozen' do @@ -31,20 +61,17 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection do freeze_time { example.run } end - it 'does not return indexes with reindex action in the last 7 days' do - not_recently_reindexed = create_list(:postgres_index, 2).each_with_index do |index, i| - create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 1.megabyte * i) - create(:reindex_action, index: index, action_end: Time.zone.now - 7.days - 1.minute) + it 'does not return indexes with reindex action in the last 10 days' do + not_recently_reindexed = Gitlab::Database::PostgresIndex.all.each do |index| + create(:reindex_action, index: index, action_end: Time.zone.now - 10.days - 1.minute) end - create_list(:postgres_index, 2).each_with_index do |index, i| - create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 1.megabyte * i) + create_list(:postgres_index, 10, ondisk_size_bytes: 10.gigabytes).each_with_index do |index, i| + create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 2.gigabyte * (i + 1)) create(:reindex_action, index: index, action_end: Time.zone.now) end - expected = Gitlab::Database::PostgresIndexBloatEstimate.where(identifier: not_recently_reindexed.map(&:identifier)).map(&:index).map(&:identifier).sort - - expect(subject.map(&:identifier).sort).to eq(expected) + expect(subject.map(&:name).sort).to eq(not_recently_reindexed.map(&:name).sort) end end end diff --git a/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb new file mode 100644 index 00000000000..6f87475fc94 --- /dev/null +++ b/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Reindexing::ReindexConcurrently, '#perform' do + subject { described_class.new(index, logger: logger).perform } + + let(:table_name) { '_test_reindex_table' } + let(:column_name) { '_test_column' } + let(:index_name) { '_test_reindex_index' } + let(:index) { Gitlab::Database::PostgresIndex.by_identifier("public.#{iname(index_name)}") } + let(:logger) { double('logger', debug: nil, info: nil, error: nil ) } + let(:connection) { ActiveRecord::Base.connection } + + before do + connection.execute(<<~SQL) + CREATE TABLE #{table_name} ( + id serial NOT NULL PRIMARY KEY, + #{column_name} integer NOT NULL); + + CREATE INDEX #{index_name} ON #{table_name} (#{column_name}); + SQL + end + + context 'when the index serves an exclusion constraint' do + before do + allow(index).to receive(:exclusion?).and_return(true) + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /indexes serving an exclusion constraint are currently not supported/) + end + end + + context 'when attempting to reindex an expression index' do + before do + allow(index).to receive(:expression?).and_return(true) + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /expression indexes are currently not supported/) + end + end + + context 'when the index is a dangling temporary index from a previous reindexing run' do + context 'with the temporary index prefix' do + let(:index_name) { '_test_reindex_index_ccnew' } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /left-over temporary index/) + end + end + + context 'with the temporary index prefix with a counter' do + let(:index_name) { '_test_reindex_index_ccnew1' } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /left-over temporary index/) + end + end + end + + it 'recreates the index using REINDEX with a long statement timeout' do + expect_to_execute_in_order( + "SET statement_timeout TO '32400s'", + "REINDEX INDEX CONCURRENTLY \"public\".\"#{index.name}\"", + "RESET statement_timeout" + ) + + subject + end + + context 'with dangling indexes matching TEMPORARY_INDEX_PATTERN, i.e. /some\_index\_ccnew(\d)*/' do + before do + # dangling indexes + connection.execute("CREATE INDEX #{iname(index_name, '_ccnew')} ON #{table_name} (#{column_name})") + connection.execute("CREATE INDEX #{iname(index_name, '_ccnew2')} ON #{table_name} (#{column_name})") + + # Unrelated index - don't drop + connection.execute("CREATE INDEX some_other_index_ccnew ON #{table_name} (#{column_name})") + end + + shared_examples_for 'dropping the dangling index' do + it 'drops the dangling indexes while controlling lock_timeout' do + expect_to_execute_in_order( + # Regular index rebuild + "SET statement_timeout TO '32400s'", + "REINDEX INDEX CONCURRENTLY \"public\".\"#{index_name}\"", + "RESET statement_timeout", + # Drop _ccnew index + "SET lock_timeout TO '60000ms'", + "DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{iname(index_name, '_ccnew')}\"", + "RESET idle_in_transaction_session_timeout; RESET lock_timeout", + # Drop _ccnew2 index + "SET lock_timeout TO '60000ms'", + "DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{iname(index_name, '_ccnew2')}\"", + "RESET idle_in_transaction_session_timeout; RESET lock_timeout" + ) + + subject + end + end + + context 'with normal index names' do + it_behaves_like 'dropping the dangling index' + end + + context 'with index name at 63 character limit' do + let(:index_name) { 'a' * 63 } + + before do + # Another unrelated index - don't drop + extra_index = index_name[0...55] + connection.execute("CREATE INDEX #{extra_index}_ccnew ON #{table_name} (#{column_name})") + end + + it_behaves_like 'dropping the dangling index' + end + end + + def iname(name, suffix = '') + "#{name[0...63 - suffix.size]}#{suffix}" + end + + def expect_to_execute_in_order(*queries) + # Indexes cannot be created CONCURRENTLY in a transaction. Since the tests are wrapped in transactions, + # verify the original call but pass through the non-concurrent form. + queries.each do |query| + expect(connection).to receive(:execute).with(query).ordered.and_wrap_original do |method, sql| + method.call(sql.sub(/CONCURRENTLY/, '')) + end + end + end +end diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index b2f038e8b62..8aff99544ca 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -31,7 +31,7 @@ RSpec.describe Gitlab::Database::Reindexing do it 'retrieves regular indexes that are no left-overs from previous runs' do result = double - expect(Gitlab::Database::PostgresIndex).to receive_message_chain('regular.where.not_match.not_match').with(no_args).with('NOT expression').with('^tmp_reindex_').with('^old_reindex_').and_return(result) + expect(Gitlab::Database::PostgresIndex).to receive_message_chain('not_match.reindexing_support').with('\_ccnew[0-9]*$').with(no_args).and_return(result) expect(subject).to eq(result) end diff --git a/spec/lib/gitlab/database/schema_migrations/context_spec.rb b/spec/lib/gitlab/database/schema_migrations/context_spec.rb new file mode 100644 index 00000000000..f3bed9b40d6 --- /dev/null +++ b/spec/lib/gitlab/database/schema_migrations/context_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::SchemaMigrations::Context do + let(:connection) { ActiveRecord::Base.connection } + + let(:context) { described_class.new(connection) } + + describe '#schema_directory' do + it 'returns db/schema_migrations' do + expect(context.schema_directory).to eq(File.join(Rails.root, 'db/schema_migrations')) + end + + context 'multiple databases' do + let(:connection) { Ci::BaseModel.connection } + + 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/ci_schema_migrations')) + end + end + end + + describe '#versions_to_create' do + before do + allow(connection).to receive_message_chain(:schema_migration, :all_versions).and_return(migrated_versions) + + migrations_struct = Struct.new(:version) + migrations = file_versions.map { |version| migrations_struct.new(version) } + allow(connection).to receive_message_chain(:migration_context, :migrations).and_return(migrations) + end + + let(:version1) { '20200123' } + let(:version2) { '20200410' } + let(:version3) { '20200602' } + let(:version4) { '20200809' } + + let(:migrated_versions) { file_versions } + let(:file_versions) { [version1, version2, version3, version4] } + + context 'migrated versions is the same as migration file versions' do + it 'returns migrated versions' do + expect(context.versions_to_create).to eq(migrated_versions) + end + end + + context 'migrated versions is subset of migration file versions' do + let(:migrated_versions) { [version1, version2] } + + it 'returns migrated versions' do + expect(context.versions_to_create).to eq(migrated_versions) + end + end + + context 'migrated versions is superset of migration file versions' do + let(:migrated_versions) { file_versions + ['20210809'] } + + it 'returns file versions' do + expect(context.versions_to_create).to eq(file_versions) + end + end + + context 'migrated versions has slightly different versions to migration file versions' do + let(:migrated_versions) { [version1, version2, version3, version4, '20210101'] } + let(:file_versions) { [version1, version2, version3, version4, '20210102'] } + + it 'returns the common set' do + expect(context.versions_to_create).to eq([version1, version2, version3, version4]) + 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/schema_version_files_spec.rb b/spec/lib/gitlab/database/schema_migrations/migrations_spec.rb index c3b3ae0a07f..8be776fdb88 100644 --- a/spec/lib/gitlab/database/schema_version_files_spec.rb +++ b/spec/lib/gitlab/database/schema_migrations/migrations_spec.rb @@ -2,43 +2,37 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::SchemaVersionFiles do - describe '.touch_all' do +RSpec.describe Gitlab::Database::SchemaMigrations::Migrations do + let(:connection) { ApplicationRecord.connection } + let(:context) { Gitlab::Database::SchemaMigrations::Context.new(connection) } + + let(:migrations) { described_class.new(context) } + + describe '#touch_all' do let(:version1) { '20200123' } let(:version2) { '20200410' } let(:version3) { '20200602' } let(:version4) { '20200809' } + let(:relative_schema_directory) { 'db/schema_migrations' } - let(:relative_migrate_directory) { 'db/migrate' } - let(:relative_post_migrate_directory) { 'db/post_migrate' } it 'creates a file containing a checksum for each version with a matching migration' do Dir.mktmpdir do |tmpdir| schema_directory = Pathname.new(tmpdir).join(relative_schema_directory) - migrate_directory = Pathname.new(tmpdir).join(relative_migrate_directory) - post_migrate_directory = Pathname.new(tmpdir).join(relative_post_migrate_directory) - - FileUtils.mkdir_p(migrate_directory) - FileUtils.mkdir_p(post_migrate_directory) FileUtils.mkdir_p(schema_directory) - migration1_filepath = migrate_directory.join("#{version1}_migration.rb") - FileUtils.touch(migration1_filepath) - - migration2_filepath = post_migrate_directory.join("#{version2}_post_migration.rb") - FileUtils.touch(migration2_filepath) - old_version_filepath = schema_directory.join('20200101') FileUtils.touch(old_version_filepath) expect(File.exist?(old_version_filepath)).to be(true) - allow(described_class).to receive(:schema_directory).and_return(schema_directory) - allow(described_class).to receive(:migration_directories).and_return([migrate_directory, post_migrate_directory]) + allow(context).to receive(:schema_directory).and_return(schema_directory) + allow(context).to receive(:versions_to_create).and_return([version1, version2]) - described_class.touch_all([version1, version2, version3, version4]) + migrations.touch_all expect(File.exist?(old_version_filepath)).to be(false) + [version1, version2].each do |version| version_filepath = schema_directory.join(version) expect(File.exist?(version_filepath)).to be(true) @@ -55,12 +49,9 @@ RSpec.describe Gitlab::Database::SchemaVersionFiles do end end - describe '.load_all' do - let(:connection) { double('connection') } - + describe '#load_all' do before do - allow(described_class).to receive(:connection).and_return(connection) - allow(described_class).to receive(:find_version_filenames).and_return(filenames) + allow(migrations).to receive(:version_filenames).and_return(filenames) end context 'when there are no version files' do @@ -70,7 +61,7 @@ RSpec.describe Gitlab::Database::SchemaVersionFiles do expect(connection).not_to receive(:quote_string) expect(connection).not_to receive(:execute) - described_class.load_all + migrations.load_all end end @@ -88,7 +79,7 @@ RSpec.describe Gitlab::Database::SchemaVersionFiles do ON CONFLICT DO NOTHING SQL - described_class.load_all + migrations.load_all end end end diff --git a/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb b/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb index e93d8ab590d..ff8e76311ae 100644 --- a/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb @@ -37,8 +37,10 @@ RSpec.describe Gitlab::Database::WithLockRetriesOutsideTransaction do context 'when lock retry is enabled' do let(:lock_fiber) do Fiber.new do + configuration = ActiveRecordSecond.configurations.find_db_config(Rails.env).configuration_hash + # Initiating a second DB connection for the lock - conn = ActiveRecordSecond.establish_connection(Rails.configuration.database_configuration[Rails.env]).connection + conn = ActiveRecordSecond.establish_connection(configuration).connection conn.transaction do conn.execute("LOCK TABLE #{Project.table_name} in exclusive mode") diff --git a/spec/lib/gitlab/database/with_lock_retries_spec.rb b/spec/lib/gitlab/database/with_lock_retries_spec.rb index df2c506e163..367f793b117 100644 --- a/spec/lib/gitlab/database/with_lock_retries_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_spec.rb @@ -37,8 +37,10 @@ RSpec.describe Gitlab::Database::WithLockRetries do context 'when lock retry is enabled' do let(:lock_fiber) do Fiber.new do + configuration = ActiveRecordSecond.configurations.find_db_config(Rails.env).configuration_hash + # Initiating a second DB connection for the lock - conn = ActiveRecordSecond.establish_connection(Rails.configuration.database_configuration[Rails.env]).connection + conn = ActiveRecordSecond.establish_connection(configuration).connection conn.transaction do conn.execute("LOCK TABLE #{Project.table_name} in exclusive mode") |