diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-18 13:16:36 +0000 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-18 13:16:36 +0000 |
| commit | 311b0269b4eb9839fa63f80c8d7a58f32b8138a0 (patch) | |
| tree | 07e7870bca8aed6d61fdcc810731c50d2c40af47 /spec/lib/gitlab/database | |
| parent | 27909cef6c4170ed9205afa7426b8d3de47cbb0c (diff) | |
| download | gitlab-ce-311b0269b4eb9839fa63f80c8d7a58f32b8138a0.tar.gz | |
Add latest changes from gitlab-org/gitlab@14-5-stable-eev14.5.0-rc42
Diffstat (limited to 'spec/lib/gitlab/database')
47 files changed, 1883 insertions, 759 deletions
diff --git a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb index 434cba4edde..223730f87c0 100644 --- a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model do + it { is_expected.to be_a Gitlab::Database::SharedModel } + describe 'validations' do let(:identifier_limit) { described_class::MAX_IDENTIFIER_LENGTH } let(:definition_limit) { described_class::MAX_DEFINITION_LENGTH } 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 779e8e40c97..04c18a98ee6 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 @@ -286,7 +286,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do let(:migration_wrapper) { Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper.new } let(:migration_helpers) { ActiveRecord::Migration.new } - let(:table_name) { :_batched_migrations_test_table } + let(:table_name) { :_test_batched_migrations_test_table } let(:column_name) { :some_id } let(:job_arguments) { [:some_id, :some_id_convert_to_bigint] } diff --git a/spec/lib/gitlab/database/batch_count_spec.rb b/spec/lib/gitlab/database/batch_count_spec.rb index da13bc425d1..9831510f014 100644 --- a/spec/lib/gitlab/database/batch_count_spec.rb +++ b/spec/lib/gitlab/database/batch_count_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Gitlab::Database::BatchCount do end before do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(in_transaction) + allow(model.connection).to receive(:transaction_open?).and_return(in_transaction) end def calculate_batch_size(batch_size) diff --git a/spec/lib/gitlab/database/connection_spec.rb b/spec/lib/gitlab/database/connection_spec.rb deleted file mode 100644 index ee1df141cd6..00000000000 --- a/spec/lib/gitlab/database/connection_spec.rb +++ /dev/null @@ -1,442 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::Connection do - let(:connection) { described_class.new } - - describe '#config' do - it 'returns a HashWithIndifferentAccess' do - expect(connection.config).to be_an_instance_of(HashWithIndifferentAccess) - end - - it 'returns a default pool size' do - expect(connection.config) - .to include(pool: Gitlab::Database.default_pool_size) - end - - it 'does not cache its results' do - a = connection.config - b = connection.config - - expect(a).not_to equal(b) - end - end - - describe '#pool_size' do - context 'when no explicit size is configured' do - it 'returns the default pool size' do - expect(connection).to receive(:config).and_return({ pool: nil }) - - expect(connection.pool_size).to eq(Gitlab::Database.default_pool_size) - end - end - - context 'when an explicit pool size is set' do - it 'returns the pool size' do - expect(connection).to receive(:config).and_return({ pool: 4 }) - - expect(connection.pool_size).to eq(4) - end - end - end - - describe '#username' do - context 'when a username is set' do - it 'returns the username' do - allow(connection).to receive(:config).and_return(username: 'bob') - - expect(connection.username).to eq('bob') - end - end - - context 'when a username is not set' do - it 'returns the value of the USER environment variable' do - allow(connection).to receive(:config).and_return(username: nil) - allow(ENV).to receive(:[]).with('USER').and_return('bob') - - expect(connection.username).to eq('bob') - end - end - end - - describe '#database_name' do - it 'returns the name of the database' do - allow(connection).to receive(:config).and_return(database: 'test') - - expect(connection.database_name).to eq('test') - end - end - - describe '#adapter_name' do - it 'returns the database adapter name' do - allow(connection).to receive(:config).and_return(adapter: 'test') - - expect(connection.adapter_name).to eq('test') - end - end - - describe '#human_adapter_name' do - context 'when the adapter is PostgreSQL' do - it 'returns PostgreSQL' do - allow(connection).to receive(:config).and_return(adapter: 'postgresql') - - expect(connection.human_adapter_name).to eq('PostgreSQL') - end - end - - context 'when the adapter is not PostgreSQL' do - it 'returns Unknown' do - allow(connection).to receive(:config).and_return(adapter: 'kittens') - - expect(connection.human_adapter_name).to eq('Unknown') - end - end - end - - describe '#postgresql?' do - context 'when using PostgreSQL' do - it 'returns true' do - allow(connection).to receive(:adapter_name).and_return('PostgreSQL') - - expect(connection.postgresql?).to eq(true) - end - end - - context 'when not using PostgreSQL' do - it 'returns false' do - allow(connection).to receive(:adapter_name).and_return('MySQL') - - expect(connection.postgresql?).to eq(false) - end - end - end - - describe '#db_config_with_default_pool_size' do - it 'returns db_config with our default pool size' do - allow(Gitlab::Database).to receive(:default_pool_size).and_return(9) - - expect(connection.db_config_with_default_pool_size.pool).to eq(9) - end - - it 'returns db_config with the correct database name' do - db_name = connection.scope.connection.pool.db_config.name - - expect(connection.db_config_with_default_pool_size.name).to eq(db_name) - end - end - - describe '#disable_prepared_statements', :reestablished_active_record_base do - it 'disables prepared statements' do - connection.scope.establish_connection( - ::Gitlab::Database.main.config.merge(prepared_statements: true) - ) - - expect(connection.scope.connection.prepared_statements).to eq(true) - - connection.disable_prepared_statements - - expect(connection.scope.connection.prepared_statements).to eq(false) - end - - it 'retains the connection name' do - connection.disable_prepared_statements - - expect(connection.scope.connection_db_config.name).to eq('main') - end - - context 'with dynamic connection pool size' do - before do - connection.scope.establish_connection(connection.config.merge(pool: 7)) - end - - it 'retains the set pool size' do - connection.disable_prepared_statements - - expect(connection.scope.connection.prepared_statements).to eq(false) - expect(connection.scope.connection.pool.size).to eq(7) - end - end - end - - describe '#db_read_only?' do - it 'detects a read-only database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => "t" }]) - - expect(connection.db_read_only?).to be_truthy - end - - it 'detects a read-only database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => true }]) - - expect(connection.db_read_only?).to be_truthy - end - - it 'detects a read-write database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => "f" }]) - - expect(connection.db_read_only?).to be_falsey - end - - it 'detects a read-write database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => false }]) - - expect(connection.db_read_only?).to be_falsey - end - end - - describe '#db_read_write?' do - it 'detects a read-only database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => "t" }]) - - expect(connection.db_read_write?).to eq(false) - end - - it 'detects a read-only database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => true }]) - - expect(connection.db_read_write?).to eq(false) - end - - it 'detects a read-write database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => "f" }]) - - expect(connection.db_read_write?).to eq(true) - end - - it 'detects a read-write database' do - allow(connection.scope.connection) - .to receive(:execute) - .with('SELECT pg_is_in_recovery()') - .and_return([{ "pg_is_in_recovery" => false }]) - - expect(connection.db_read_write?).to eq(true) - end - end - - describe '#version' do - around do |example| - connection.instance_variable_set(:@version, nil) - example.run - connection.instance_variable_set(:@version, nil) - end - - context "on postgresql" do - it "extracts the version number" do - allow(connection) - .to receive(:database_version) - .and_return("PostgreSQL 9.4.4 on x86_64-apple-darwin14.3.0") - - expect(connection.version).to eq '9.4.4' - end - end - - it 'memoizes the result' do - count = ActiveRecord::QueryRecorder - .new { 2.times { connection.version } } - .count - - expect(count).to eq(1) - end - end - - describe '#postgresql_minimum_supported_version?' do - it 'returns false when using PostgreSQL 10' do - allow(connection).to receive(:version).and_return('10') - - expect(connection.postgresql_minimum_supported_version?).to eq(false) - end - - it 'returns false when using PostgreSQL 11' do - allow(connection).to receive(:version).and_return('11') - - expect(connection.postgresql_minimum_supported_version?).to eq(false) - end - - it 'returns true when using PostgreSQL 12' do - allow(connection).to receive(:version).and_return('12') - - expect(connection.postgresql_minimum_supported_version?).to eq(true) - end - end - - describe '#bulk_insert' do - before do - allow(connection).to receive(:connection).and_return(dummy_connection) - allow(dummy_connection).to receive(:quote_column_name, &:itself) - allow(dummy_connection).to receive(:quote, &:itself) - allow(dummy_connection).to receive(:execute) - end - - let(:dummy_connection) { double(:connection) } - - let(:rows) do - [ - { a: 1, b: 2, c: 3 }, - { c: 6, a: 4, b: 5 } - ] - end - - it 'does nothing with empty rows' do - expect(dummy_connection).not_to receive(:execute) - - connection.bulk_insert('test', []) - end - - it 'uses the ordering from the first row' do - expect(dummy_connection).to receive(:execute) do |sql| - expect(sql).to include('(1, 2, 3)') - expect(sql).to include('(4, 5, 6)') - end - - connection.bulk_insert('test', rows) - end - - it 'quotes column names' do - expect(dummy_connection).to receive(:quote_column_name).with(:a) - expect(dummy_connection).to receive(:quote_column_name).with(:b) - expect(dummy_connection).to receive(:quote_column_name).with(:c) - - connection.bulk_insert('test', rows) - end - - it 'quotes values' do - 1.upto(6) do |i| - expect(dummy_connection).to receive(:quote).with(i) - end - - connection.bulk_insert('test', rows) - end - - it 'does not quote values of a column in the disable_quote option' do - [1, 2, 4, 5].each do |i| - expect(dummy_connection).to receive(:quote).with(i) - end - - connection.bulk_insert('test', rows, disable_quote: :c) - end - - it 'does not quote values of columns in the disable_quote option' do - [2, 5].each do |i| - expect(dummy_connection).to receive(:quote).with(i) - end - - connection.bulk_insert('test', rows, disable_quote: [:a, :c]) - end - - it 'handles non-UTF-8 data' do - expect { connection.bulk_insert('test', [{ a: "\255" }]) }.not_to raise_error - end - - context 'when using PostgreSQL' do - it 'allows the returning of the IDs of the inserted rows' do - result = double(:result, values: [['10']]) - - expect(dummy_connection) - .to receive(:execute) - .with(/RETURNING id/) - .and_return(result) - - ids = connection - .bulk_insert('test', [{ number: 10 }], return_ids: true) - - expect(ids).to eq([10]) - end - - it 'allows setting the upsert to do nothing' do - expect(dummy_connection) - .to receive(:execute) - .with(/ON CONFLICT DO NOTHING/) - - connection - .bulk_insert('test', [{ number: 10 }], on_conflict: :do_nothing) - end - end - end - - describe '#cached_column_exists?' do - it 'only retrieves the data from the schema cache' do - queries = ActiveRecord::QueryRecorder.new do - 2.times do - expect(connection.cached_column_exists?(:projects, :id)).to be_truthy - expect(connection.cached_column_exists?(:projects, :bogus_column)).to be_falsey - end - end - - expect(queries.count).to eq(0) - end - end - - describe '#cached_table_exists?' do - it 'only retrieves the data from the schema cache' do - queries = ActiveRecord::QueryRecorder.new do - 2.times do - expect(connection.cached_table_exists?(:projects)).to be_truthy - expect(connection.cached_table_exists?(:bogus_table_name)).to be_falsey - end - end - - expect(queries.count).to eq(0) - end - - it 'returns false when database does not exist' do - expect(connection.scope).to receive(:connection) do - raise ActiveRecord::NoDatabaseError, 'broken' - end - - expect(connection.cached_table_exists?(:projects)).to be(false) - end - end - - describe '#exists?' do - it 'returns true if the database exists' do - expect(connection.exists?).to be(true) - end - - it "returns false if the database doesn't exist" do - expect(connection.scope.connection.schema_cache) - .to receive(:database_version) - .and_raise(ActiveRecord::NoDatabaseError) - - expect(connection.exists?).to be(false) - end - end - - describe '#system_id' do - it 'returns the PostgreSQL system identifier' do - expect(connection.system_id).to be_an_instance_of(Integer) - end - end - - describe '#get_write_location' do - it 'returns a string' do - expect(connection.get_write_location(connection.scope.connection)) - .to be_a(String) - end - - it 'returns nil if there are no results' do - expect(connection.get_write_location(double(select_all: []))).to be_nil - end - end -end diff --git a/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb b/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb index cdcc862c376..9d49db1f018 100644 --- a/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb +++ b/spec/lib/gitlab/database/count/reltuples_count_strategy_spec.rb @@ -38,7 +38,8 @@ RSpec.describe Gitlab::Database::Count::ReltuplesCountStrategy do it 'returns nil counts for inherited tables' do models.each { |model| expect(model).not_to receive(:count) } - expect(subject).to eq({ Namespace => 3 }) + # 3 Namespaces as parents for each Project and 3 ProjectNamespaces(for each Project) + expect(subject).to eq({ Namespace => 6 }) end end diff --git a/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb b/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb index c2028f8c238..2f261aebf02 100644 --- a/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb +++ b/spec/lib/gitlab/database/count/tablesample_count_strategy_spec.rb @@ -47,7 +47,8 @@ RSpec.describe Gitlab::Database::Count::TablesampleCountStrategy do result = subject expect(result[Project]).to eq(3) expect(result[Group]).to eq(1) - expect(result[Namespace]).to eq(4) + # 1-Group, 3 namespaces for each project and 3 project namespaces for each project + expect(result[Namespace]).to eq(7) end end diff --git a/spec/lib/gitlab/database/each_database_spec.rb b/spec/lib/gitlab/database/each_database_spec.rb new file mode 100644 index 00000000000..9327fc4ff78 --- /dev/null +++ b/spec/lib/gitlab/database/each_database_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::EachDatabase do + describe '.each_database_connection' do + let(:expected_connections) do + Gitlab::Database.database_base_models.map { |name, model| [model.connection, name] } + end + + it 'yields each connection after connecting SharedModel' do + expected_connections.each do |connection, _| + expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(connection).and_yield + end + + yielded_connections = [] + + described_class.each_database_connection do |connection, name| + yielded_connections << [connection, name] + end + + expect(yielded_connections).to match_array(expected_connections) + end + end + + describe '.each_model_connection' do + let(:model1) { double(connection: double, table_name: 'table1') } + let(:model2) { double(connection: double, table_name: 'table2') } + + before do + allow(model1.connection).to receive_message_chain('pool.db_config.name').and_return('name1') + allow(model2.connection).to receive_message_chain('pool.db_config.name').and_return('name2') + end + + it 'yields each model after connecting SharedModel' do + expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(model1.connection).and_yield + expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(model2.connection).and_yield + + yielded_models = [] + + described_class.each_model_connection([model1, model2]) do |model, name| + yielded_models << [model, name] + end + + expect(yielded_models).to match_array([[model1, 'name1'], [model2, 'name2']]) + end + end +end diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb new file mode 100644 index 00000000000..255efc99ff6 --- /dev/null +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Database::GitlabSchema do + describe '.tables_to_schema' do + subject { described_class.tables_to_schema } + + it 'all tables have assigned a known gitlab_schema' do + is_expected.to all( + match([be_a(String), be_in([:gitlab_shared, :gitlab_main, :gitlab_ci])]) + ) + end + + # This being run across different databases indirectly also tests + # a general consistency of structure across databases + Gitlab::Database.database_base_models.each do |db_config_name, db_class| + let(:db_data_sources) { db_class.connection.data_sources } + + context "for #{db_config_name} using #{db_class}" do + it 'new data sources are added' do + missing_tables = db_data_sources.to_set - subject.keys + + expect(missing_tables).to be_empty, \ + "Missing table(s) #{missing_tables.to_a} not found in #{described_class}.tables_to_schema. " \ + "Any new tables must be added to lib/gitlab/database/gitlab_schemas.yml." + end + + it 'non-existing data sources are removed' do + extra_tables = subject.keys.to_set - db_data_sources + + expect(extra_tables).to be_empty, \ + "Extra table(s) #{extra_tables.to_a} found in #{described_class}.tables_to_schema. " \ + "Any removed or renamed tables must be removed from lib/gitlab/database/gitlab_schemas.yml." + end + end + end + end + + describe '.table_schema' do + using RSpec::Parameterized::TableSyntax + + where(:name, :classification) do + 'ci_builds' | :gitlab_ci + 'my_schema.ci_builds' | :gitlab_ci + 'information_schema.columns' | :gitlab_shared + 'audit_events_part_5fc467ac26' | :gitlab_main + '_test_my_table' | :gitlab_shared + 'pg_attribute' | :gitlab_shared + 'my_other_table' | :undefined_my_other_table + end + + with_them do + subject { described_class.table_schema(name) } + + it { is_expected.to eq(classification) } + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/configuration_spec.rb b/spec/lib/gitlab/database/load_balancing/configuration_spec.rb index 3e5249a3dea..eef248afdf2 100644 --- a/spec/lib/gitlab/database/load_balancing/configuration_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/configuration_spec.rb @@ -3,17 +3,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::Configuration do - let(:model) do - config = ActiveRecord::DatabaseConfigurations::HashConfig - .new('main', 'test', configuration_hash) - - double(:model, connection_db_config: config) - end + let(:configuration_hash) { {} } + let(:db_config) { ActiveRecord::DatabaseConfigurations::HashConfig.new('test', 'ci', configuration_hash) } + let(:model) { double(:model, connection_db_config: db_config) } describe '.for_model' do context 'when load balancing is not configured' do - let(:configuration_hash) { {} } - it 'uses the default settings' do config = described_class.for_model(model) @@ -105,6 +100,14 @@ RSpec.describe Gitlab::Database::LoadBalancing::Configuration do expect(config.pool_size).to eq(4) end end + + it 'calls reuse_primary_connection!' do + expect_next_instance_of(described_class) do |subject| + expect(subject).to receive(:reuse_primary_connection!).and_call_original + end + + described_class.for_model(model) + end end describe '#load_balancing_enabled?' do @@ -180,4 +183,60 @@ RSpec.describe Gitlab::Database::LoadBalancing::Configuration do end end end + + describe '#db_config_name' do + let(:config) { described_class.new(model) } + + subject { config.db_config_name } + + it 'returns connection name as symbol' do + is_expected.to eq(:ci) + end + end + + describe '#replica_db_config' do + let(:model) { double(:model, connection_db_config: db_config, connection_specification_name: 'Ci::ApplicationRecord') } + let(:config) { described_class.for_model(model) } + + it 'returns exactly db_config' do + expect(config.replica_db_config).to eq(db_config) + end + + context 'when GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci=main' do + it 'does not change replica_db_config' do + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', 'main') + + expect(config.replica_db_config).to eq(db_config) + end + end + end + + describe 'reuse_primary_connection!' do + let(:model) { double(:model, connection_db_config: db_config, connection_specification_name: 'Ci::ApplicationRecord') } + let(:config) { described_class.for_model(model) } + + context 'when GITLAB_LOAD_BALANCING_REUSE_PRIMARY_* not configured' do + it 'the primary connection uses default specification' do + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', nil) + + expect(config.primary_connection_specification_name).to eq('Ci::ApplicationRecord') + end + end + + context 'when GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci=main' do + it 'the primary connection uses main connection' do + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', 'main') + + expect(config.primary_connection_specification_name).to eq('ActiveRecord::Base') + end + end + + context 'when GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci=unknown' do + it 'raises exception' do + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', 'unknown') + + expect { config.reuse_primary_connection! }.to raise_error /Invalid value for/ + end + end + end end diff --git a/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb b/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb index ba2f9485066..ee2718171c0 100644 --- a/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb @@ -3,12 +3,9 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do - let(:proxy) do - config = Gitlab::Database::LoadBalancing::Configuration - .new(ActiveRecord::Base) - - described_class.new(Gitlab::Database::LoadBalancing::LoadBalancer.new(config)) - end + let(:config) { Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) } + let(:load_balancer) { Gitlab::Database::LoadBalancing::LoadBalancer.new(config) } + let(:proxy) { described_class.new(load_balancer) } describe '#select' do it 'performs a read' do @@ -85,7 +82,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do describe '.insert_all!' do before do ActiveRecord::Schema.define do - create_table :connection_proxy_bulk_insert, force: true do |t| + create_table :_test_connection_proxy_bulk_insert, force: true do |t| t.string :name, null: true end end @@ -93,13 +90,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do after do ActiveRecord::Schema.define do - drop_table :connection_proxy_bulk_insert, force: true + drop_table :_test_connection_proxy_bulk_insert, force: true end end let(:model_class) do Class.new(ApplicationRecord) do - self.table_name = "connection_proxy_bulk_insert" + self.table_name = "_test_connection_proxy_bulk_insert" end end @@ -143,9 +140,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do context 'with a read query' do it 'runs the transaction and any nested queries on the replica' do - expect(proxy.load_balancer).to receive(:read) + expect(load_balancer).to receive(:read) .twice.and_yield(replica) - expect(proxy.load_balancer).not_to receive(:read_write) + expect(load_balancer).not_to receive(:read_write) expect(session).not_to receive(:write!) proxy.transaction { proxy.select('true') } @@ -154,8 +151,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do context 'with a write query' do it 'raises an exception' do - allow(proxy.load_balancer).to receive(:read).and_yield(replica) - allow(proxy.load_balancer).to receive(:read_write).and_yield(replica) + allow(load_balancer).to receive(:read).and_yield(replica) + allow(load_balancer).to receive(:read_write).and_yield(replica) expect do proxy.transaction { proxy.insert('something') } @@ -178,9 +175,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do context 'with a read query' do it 'runs the transaction and any nested queries on the primary and stick to it' do - expect(proxy.load_balancer).to receive(:read_write) + expect(load_balancer).to receive(:read_write) .twice.and_yield(primary) - expect(proxy.load_balancer).not_to receive(:read) + expect(load_balancer).not_to receive(:read) expect(session).to receive(:write!) proxy.transaction { proxy.select('true') } @@ -189,9 +186,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do context 'with a write query' do it 'runs the transaction and any nested queries on the primary and stick to it' do - expect(proxy.load_balancer).to receive(:read_write) + expect(load_balancer).to receive(:read_write) .twice.and_yield(primary) - expect(proxy.load_balancer).not_to receive(:read) + expect(load_balancer).not_to receive(:read) expect(session).to receive(:write!).twice proxy.transaction { proxy.insert('something') } @@ -209,7 +206,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do end it 'properly forwards keyword arguments' do - allow(proxy.load_balancer).to receive(:read_write) + allow(load_balancer).to receive(:read_write) expect(proxy).to receive(:write_using_load_balancer).and_call_original @@ -234,7 +231,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do end it 'properly forwards keyword arguments' do - allow(proxy.load_balancer).to receive(:read) + allow(load_balancer).to receive(:read) expect(proxy).to receive(:read_using_load_balancer).and_call_original @@ -259,7 +256,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do allow(session).to receive(:use_replicas_for_read_queries?).and_return(false) expect(connection).to receive(:foo).with('foo') - expect(proxy.load_balancer).to receive(:read).and_yield(connection) + expect(load_balancer).to receive(:read).and_yield(connection) proxy.read_using_load_balancer(:foo, 'foo') end @@ -271,7 +268,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do allow(session).to receive(:use_replicas_for_read_queries?).and_return(true) expect(connection).to receive(:foo).with('foo') - expect(proxy.load_balancer).to receive(:read).and_yield(connection) + expect(load_balancer).to receive(:read).and_yield(connection) proxy.read_using_load_balancer(:foo, 'foo') end @@ -283,7 +280,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do allow(session).to receive(:use_replicas_for_read_queries?).and_return(true) expect(connection).to receive(:foo).with('foo') - expect(proxy.load_balancer).to receive(:read).and_yield(connection) + expect(load_balancer).to receive(:read).and_yield(connection) proxy.read_using_load_balancer(:foo, 'foo') end @@ -296,7 +293,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do expect(connection).to receive(:foo).with('foo') - expect(proxy.load_balancer).to receive(:read_write) + expect(load_balancer).to receive(:read_write) .and_yield(connection) proxy.read_using_load_balancer(:foo, 'foo') @@ -314,7 +311,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do end it 'uses but does not stick to the primary' do - expect(proxy.load_balancer).to receive(:read_write).and_yield(connection) + expect(load_balancer).to receive(:read_write).and_yield(connection) expect(connection).to receive(:foo).with('foo') expect(session).not_to receive(:write!) 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 f824d4cefdf..37b83729125 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -4,10 +4,11 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do let(:conflict_error) { Class.new(RuntimeError) } - let(:db_host) { ActiveRecord::Base.connection_pool.db_config.host } + let(:model) { ActiveRecord::Base } + let(:db_host) { model.connection_pool.db_config.host } let(:config) do Gitlab::Database::LoadBalancing::Configuration - .new(ActiveRecord::Base, [db_host, db_host]) + .new(model, [db_host, db_host]) end let(:lb) { described_class.new(config) } @@ -88,6 +89,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do host = double(:host) allow(lb).to receive(:host).and_return(host) + allow(Rails.application.executor).to receive(:active?).and_return(true) allow(host).to receive(:query_cache_enabled).and_return(false) allow(host).to receive(:connection).and_return(connection) @@ -96,6 +98,20 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do lb.read { 10 } end + it 'does not enable query cache when outside Rails executor context' do + connection = double(:connection) + host = double(:host) + + allow(lb).to receive(:host).and_return(host) + allow(Rails.application.executor).to receive(:active?).and_return(false) + allow(host).to receive(:query_cache_enabled).and_return(false) + allow(host).to receive(:connection).and_return(connection) + + expect(host).not_to receive(:enable_query_cache!) + + lb.read { 10 } + end + it 'marks hosts that are offline' do allow(lb).to receive(:connection_error?).and_return(true) @@ -216,7 +232,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do it 'does not create conflicts with other load balancers when caching hosts' do ci_config = Gitlab::Database::LoadBalancing::Configuration - .new(Ci::CiDatabaseRecord, [db_host, db_host]) + .new(Ci::ApplicationRecord, [db_host, db_host]) lb1 = described_class.new(config) lb2 = described_class.new(ci_config) @@ -459,4 +475,84 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do lb.disconnect!(timeout: 30) end end + + describe '#get_write_location' do + it 'returns a string' do + expect(lb.send(:get_write_location, lb.pool.connection)) + .to be_a(String) + end + + it 'returns nil if there are no results' do + expect(lb.send(:get_write_location, double(select_all: []))).to be_nil + end + end + + describe 'primary connection re-use', :reestablished_active_record_base do + let(:model) { Ci::ApplicationRecord } + + around do |example| + if Gitlab::Database.has_config?(:ci) + example.run + else + # fake additional Database + model.establish_connection( + ActiveRecord::DatabaseConfigurations::HashConfig.new(Rails.env, 'ci', ActiveRecord::Base.connection_db_config.configuration_hash) + ) + + example.run + + # Cleanup connection_specification_name for Ci::ApplicationRecord + model.remove_connection + end + end + + describe '#read' do + it 'returns ci replica connection' do + expect { |b| lb.read(&b) }.to yield_with_args do |args| + expect(args.pool.db_config.name).to eq('ci_replica') + end + end + + context 'when GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci=main' do + it 'returns ci replica connection' do + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', 'main') + + expect { |b| lb.read(&b) }.to yield_with_args do |args| + expect(args.pool.db_config.name).to eq('ci_replica') + end + end + end + end + + describe '#read_write' do + it 'returns Ci::ApplicationRecord connection' do + expect { |b| lb.read_write(&b) }.to yield_with_args do |args| + expect(args.pool.db_config.name).to eq('ci') + end + end + + context 'when GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci=main' do + it 'returns ActiveRecord::Base connection' do + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', 'main') + + expect { |b| lb.read_write(&b) }.to yield_with_args do |args| + expect(args.pool.db_config.name).to eq('main') + end + end + end + end + end + + describe '#wal_diff' do + it 'returns the diff between two write locations' do + loc1 = lb.send(:get_write_location, lb.pool.connection) + + create(:user) # This ensures we get a new WAL location + + loc2 = lb.send(:get_write_location, lb.pool.connection) + diff = lb.wal_diff(loc2, loc1) + + expect(diff).to be_positive + end + end end diff --git a/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb b/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb index 45d81808971..02c9499bedb 100644 --- a/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb @@ -51,7 +51,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::PrimaryHost do end describe '#offline!' do - it 'does nothing' do + it 'logs the event but does nothing else' do + expect(Gitlab::Database::LoadBalancing::Logger).to receive(:warn) + .with(hash_including(event: :host_offline)) + .and_call_original + expect(host.offline!).to be_nil end end 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 af7e2a4b167..b768d4ecea3 100644 --- a/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb @@ -6,12 +6,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do let(:app) { double(:app) } let(:middleware) { described_class.new(app) } let(:warden_user) { double(:warden, user: double(:user, id: 42)) } - let(:single_sticking_object) { Set.new([[ActiveRecord::Base, :user, 42]]) } + let(:single_sticking_object) { Set.new([[ActiveRecord::Base.sticking, :user, 42]]) } let(:multiple_sticking_objects) do Set.new([ - [ActiveRecord::Base, :user, 42], - [ActiveRecord::Base, :runner, '123456789'], - [ActiveRecord::Base, :runner, '1234'] + [ActiveRecord::Base.sticking, :user, 42], + [ActiveRecord::Base.sticking, :runner, '123456789'], + [ActiveRecord::Base.sticking, :runner, '1234'] ]) end @@ -162,7 +162,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do it 'returns the warden user if present' do env = { 'warden' => warden_user } ids = Gitlab::Database::LoadBalancing.base_models.map do |model| - [model, :user, 42] + [model.sticking, :user, 42] end expect(middleware.sticking_namespaces(env)).to eq(ids) @@ -181,9 +181,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do env = { described_class::STICK_OBJECT => multiple_sticking_objects } expect(middleware.sticking_namespaces(env)).to eq([ - [ActiveRecord::Base, :user, 42], - [ActiveRecord::Base, :runner, '123456789'], - [ActiveRecord::Base, :runner, '1234'] + [ActiveRecord::Base.sticking, :user, 42], + [ActiveRecord::Base.sticking, :runner, '123456789'], + [ActiveRecord::Base.sticking, :runner, '1234'] ]) end end diff --git a/spec/lib/gitlab/database/load_balancing/setup_spec.rb b/spec/lib/gitlab/database/load_balancing/setup_spec.rb index 01646bc76ef..953d83d3b48 100644 --- a/spec/lib/gitlab/database/load_balancing/setup_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/setup_spec.rb @@ -7,19 +7,20 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do it 'sets up the load balancer' do setup = described_class.new(ActiveRecord::Base) - expect(setup).to receive(:disable_prepared_statements) - expect(setup).to receive(:setup_load_balancer) + expect(setup).to receive(:configure_connection) + expect(setup).to receive(:setup_connection_proxy) expect(setup).to receive(:setup_service_discovery) + expect(setup).to receive(:setup_feature_flag_to_model_load_balancing) setup.setup end end - describe '#disable_prepared_statements' do - it 'disables prepared statements and reconnects to the database' do + describe '#configure_connection' do + it 'configures pool, prepared statements and reconnects to the database' do config = double( :config, - configuration_hash: { host: 'localhost' }, + configuration_hash: { host: 'localhost', pool: 2, prepared_statements: true }, env_name: 'test', name: 'main' ) @@ -27,7 +28,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do expect(ActiveRecord::DatabaseConfigurations::HashConfig) .to receive(:new) - .with('test', 'main', { host: 'localhost', prepared_statements: false }) + .with('test', 'main', { + host: 'localhost', + prepared_statements: false, + pool: Gitlab::Database.default_pool_size + }) .and_call_original # HashConfig doesn't implement its own #==, so we can't directly compare @@ -36,11 +41,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do .to receive(:establish_connection) .with(an_instance_of(ActiveRecord::DatabaseConfigurations::HashConfig)) - described_class.new(model).disable_prepared_statements + described_class.new(model).configure_connection end end - describe '#setup_load_balancer' do + describe '#setup_connection_proxy' do it 'sets up the load balancer' do model = Class.new(ActiveRecord::Base) setup = described_class.new(model) @@ -54,9 +59,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do .with(setup.configuration) .and_return(lb) - setup.setup_load_balancer + setup.setup_connection_proxy - expect(model.connection.load_balancer).to eq(lb) + expect(model.load_balancer).to eq(lb) expect(model.sticking) .to be_an_instance_of(Gitlab::Database::LoadBalancing::Sticking) end @@ -77,7 +82,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do model = ActiveRecord::Base setup = described_class.new(model) sv = instance_spy(Gitlab::Database::LoadBalancing::ServiceDiscovery) - lb = model.connection.load_balancer allow(setup.configuration) .to receive(:service_discovery_enabled?) @@ -85,7 +89,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do allow(Gitlab::Database::LoadBalancing::ServiceDiscovery) .to receive(:new) - .with(lb, setup.configuration.service_discovery) + .with(setup.load_balancer, setup.configuration.service_discovery) .and_return(sv) expect(sv).to receive(:perform_service_discovery) @@ -98,7 +102,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do model = ActiveRecord::Base setup = described_class.new(model, start_service_discovery: true) sv = instance_spy(Gitlab::Database::LoadBalancing::ServiceDiscovery) - lb = model.connection.load_balancer allow(setup.configuration) .to receive(:service_discovery_enabled?) @@ -106,7 +109,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do allow(Gitlab::Database::LoadBalancing::ServiceDiscovery) .to receive(:new) - .with(lb, setup.configuration.service_discovery) + .with(setup.load_balancer, setup.configuration.service_discovery) .and_return(sv) expect(sv).to receive(:perform_service_discovery) @@ -116,4 +119,181 @@ RSpec.describe Gitlab::Database::LoadBalancing::Setup do end end end + + describe '#setup_feature_flag_to_model_load_balancing', :reestablished_active_record_base do + using RSpec::Parameterized::TableSyntax + + where do + { + "with model LB enabled it picks a dedicated CI connection" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: 'true', + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil, + request_store_active: false, + ff_use_model_load_balancing: nil, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'ci_replica', write: 'ci' } + } + }, + "with model LB enabled and re-use of primary connection it uses CI connection for reads" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: 'true', + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main', + request_store_active: false, + ff_use_model_load_balancing: nil, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'ci_replica', write: 'main' } + } + }, + "with model LB disabled it fallbacks to use main" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: 'false', + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil, + request_store_active: false, + ff_use_model_load_balancing: nil, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'main_replica', write: 'main' } + } + }, + "with model LB disabled, but re-use configured it fallbacks to use main" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: 'false', + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main', + request_store_active: false, + ff_use_model_load_balancing: nil, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'main_replica', write: 'main' } + } + }, + "with FF disabled without RequestStore it uses main" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: nil, + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil, + request_store_active: false, + ff_use_model_load_balancing: false, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'main_replica', write: 'main' } + } + }, + "with FF enabled without RequestStore sticking of FF does not work, so it fallbacks to use main" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: nil, + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil, + request_store_active: false, + ff_use_model_load_balancing: true, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'main_replica', write: 'main' } + } + }, + "with FF disabled with RequestStore it uses main" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: nil, + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil, + request_store_active: true, + ff_use_model_load_balancing: false, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'main_replica', write: 'main' } + } + }, + "with FF enabled with RequestStore it sticks FF and uses CI connection" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: nil, + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: nil, + request_store_active: true, + ff_use_model_load_balancing: true, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'ci_replica', write: 'ci' } + } + }, + "with re-use and FF enabled with RequestStore it sticks FF and uses CI connection for reads" => { + env_GITLAB_USE_MODEL_LOAD_BALANCING: nil, + env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci: 'main', + request_store_active: true, + ff_use_model_load_balancing: true, + expectations: { + main: { read: 'main_replica', write: 'main' }, + ci: { read: 'ci_replica', write: 'main' } + } + } + } + end + + with_them do + let(:ci_class) do + Class.new(ActiveRecord::Base) do + def self.name + 'Ci::ApplicationRecordTemporary' + end + + establish_connection ActiveRecord::DatabaseConfigurations::HashConfig.new( + Rails.env, + 'ci', + ActiveRecord::Base.connection_db_config.configuration_hash + ) + end + end + + let(:models) do + { + main: ActiveRecord::Base, + ci: ci_class + } + end + + around do |example| + if request_store_active + Gitlab::WithRequestStore.with_request_store do + RequestStore.clear! + + example.run + end + else + example.run + end + end + + before do + # Rewrite `class_attribute` to use rspec mocking and prevent modifying the objects + allow_next_instance_of(described_class) do |setup| + allow(setup).to receive(:configure_connection) + + allow(setup).to receive(:setup_class_attribute) do |attribute, value| + allow(setup.model).to receive(attribute) { value } + end + end + + stub_env('GITLAB_USE_MODEL_LOAD_BALANCING', env_GITLAB_USE_MODEL_LOAD_BALANCING) + stub_env('GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci', env_GITLAB_LOAD_BALANCING_REUSE_PRIMARY_ci) + stub_feature_flags(use_model_load_balancing: ff_use_model_load_balancing) + + # Make load balancer to force init with a dedicated replicas connections + models.each do |_, model| + described_class.new(model).tap do |subject| + subject.configuration.hosts = [subject.configuration.replica_db_config.host] + subject.setup + end + end + end + + it 'results match expectations' do + result = models.transform_values do |model| + load_balancer = model.connection.instance_variable_get(:@load_balancer) + + { + read: load_balancer.read { |connection| connection.pool.db_config.name }, + write: load_balancer.read_write { |connection| connection.pool.db_config.name } + } + end + + expect(result).to eq(expectations) + end + + it 'does return load_balancer assigned to a given connection' do + models.each do |name, model| + expect(model.load_balancer.name).to eq(name) + expect(model.sticking.instance_variable_get(:@load_balancer)).to eq(model.load_balancer) + end + end + end + 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 08dd6a0a788..9acf80e684f 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 @@ -181,11 +181,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end context 'when worker data consistency is :delayed' do - include_examples 'mark data consistency location', :delayed + include_examples 'mark data consistency location', :delayed end context 'when worker data consistency is :sticky' do - include_examples 'mark data consistency location', :sticky + include_examples 'mark data consistency location', :sticky end end 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 06efdcd8f99..de2ad662d16 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 @@ -64,7 +64,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } } it 'does not stick to the primary', :aggregate_failures do - expect(ActiveRecord::Base.connection.load_balancer) + expect(ActiveRecord::Base.load_balancer) .to receive(:select_up_to_date_host) .with(location) .and_return(true) @@ -107,7 +107,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'dedup_wal_locations' => wal_locations } } before do - allow(ActiveRecord::Base.connection.load_balancer) + allow(ActiveRecord::Base.load_balancer) .to receive(:select_up_to_date_host) .with(wal_locations[:main]) .and_return(true) @@ -120,7 +120,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'database_write_location' => '0/D525E3A8' } } before do - allow(ActiveRecord::Base.connection.load_balancer) + allow(ActiveRecord::Base.load_balancer) .to receive(:select_up_to_date_host) .with('0/D525E3A8') .and_return(true) diff --git a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb index 8ceda52ee85..d88554614cf 100644 --- a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do let(:sticking) do - described_class.new(ActiveRecord::Base.connection.load_balancer) + described_class.new(ActiveRecord::Base.load_balancer) end after do @@ -22,7 +22,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do sticking.stick_or_unstick_request(env, :user, 42) expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a) - .to eq([[ActiveRecord::Base, :user, 42]]) + .to eq([[sticking, :user, 42]]) end it 'sticks or unsticks multiple objects and updates the Rack environment' do @@ -42,8 +42,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do sticking.stick_or_unstick_request(env, :runner, '123456789') expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a).to eq([ - [ActiveRecord::Base, :user, 42], - [ActiveRecord::Base, :runner, '123456789'] + [sticking, :user, 42], + [sticking, :runner, '123456789'] ]) end end @@ -73,7 +73,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do end describe '#all_caught_up?' do - let(:lb) { ActiveRecord::Base.connection.load_balancer } + let(:lb) { ActiveRecord::Base.load_balancer } let(:last_write_location) { 'foo' } before do @@ -137,7 +137,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do end describe '#unstick_or_continue_sticking' do - let(:lb) { ActiveRecord::Base.connection.load_balancer } + let(:lb) { ActiveRecord::Base.load_balancer } it 'simply returns if no write location could be found' do allow(sticking) @@ -182,13 +182,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do RSpec.shared_examples 'sticking' do before do - allow(ActiveRecord::Base.connection.load_balancer) + allow(ActiveRecord::Base.load_balancer) .to receive(:primary_write_location) .and_return('foo') end it 'sticks an entity to the primary', :aggregate_failures do - allow(ActiveRecord::Base.connection.load_balancer) + allow(ActiveRecord::Base.load_balancer) .to receive(:primary_only?) .and_return(false) @@ -227,11 +227,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do describe '#mark_primary_write_location' do it 'updates the write location with the load balancer' do - allow(ActiveRecord::Base.connection.load_balancer) + allow(ActiveRecord::Base.load_balancer) .to receive(:primary_write_location) .and_return('foo') - allow(ActiveRecord::Base.connection.load_balancer) + allow(ActiveRecord::Base.load_balancer) .to receive(:primary_only?) .and_return(false) @@ -291,7 +291,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do end describe '#select_caught_up_replicas' do - let(:lb) { ActiveRecord::Base.connection.load_balancer } + let(:lb) { ActiveRecord::Base.load_balancer } context 'with no write location' do before do diff --git a/spec/lib/gitlab/database/load_balancing_spec.rb b/spec/lib/gitlab/database/load_balancing_spec.rb index bf5314e2c34..65ffe539910 100644 --- a/spec/lib/gitlab/database/load_balancing_spec.rb +++ b/spec/lib/gitlab/database/load_balancing_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Gitlab::Database::LoadBalancing do expect(models).to include(ActiveRecord::Base) if Gitlab::Database.has_config?(:ci) - expect(models).to include(Ci::CiDatabaseRecord) + expect(models).to include(Ci::ApplicationRecord) end end @@ -76,7 +76,7 @@ RSpec.describe Gitlab::Database::LoadBalancing do context 'when a read connection is used' do it 'returns :replica' do - proxy.load_balancer.read do |connection| + load_balancer.read do |connection| expect(described_class.db_role_for_connection(connection)).to eq(:replica) end end @@ -84,7 +84,7 @@ RSpec.describe Gitlab::Database::LoadBalancing do context 'when a read_write connection is used' do it 'returns :primary' do - proxy.load_balancer.read_write do |connection| + load_balancer.read_write do |connection| expect(described_class.db_role_for_connection(connection)).to eq(:primary) end end @@ -105,7 +105,7 @@ RSpec.describe Gitlab::Database::LoadBalancing do describe 'LoadBalancing integration tests', :database_replica, :delete do before(:all) do ActiveRecord::Schema.define do - create_table :load_balancing_test, force: true do |t| + create_table :_test_load_balancing_test, force: true do |t| t.string :name, null: true end end @@ -113,13 +113,13 @@ RSpec.describe Gitlab::Database::LoadBalancing do after(:all) do ActiveRecord::Schema.define do - drop_table :load_balancing_test, force: true + drop_table :_test_load_balancing_test, force: true end end let(:model) do Class.new(ApplicationRecord) do - self.table_name = "load_balancing_test" + self.table_name = "_test_load_balancing_test" end end @@ -443,7 +443,7 @@ RSpec.describe Gitlab::Database::LoadBalancing do elsif payload[:name] == 'SQL' # Custom query true else - keywords = %w[load_balancing_test] + keywords = %w[_test_load_balancing_test] keywords += %w[begin commit] if include_transaction keywords.any? { |keyword| payload[:sql].downcase.include?(keyword) } end diff --git a/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb index 54b3ad22faf..f1dbfbbff18 100644 --- a/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/loose_foreign_key_helpers_spec.rb @@ -9,18 +9,18 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do let(:model) do Class.new(ApplicationRecord) do - self.table_name = 'loose_fk_test_table' + self.table_name = '_test_loose_fk_test_table' end end before(:all) do - migration.create_table :loose_fk_test_table do |t| + migration.create_table :_test_loose_fk_test_table do |t| t.timestamps end end after(:all) do - migration.drop_table :loose_fk_test_table + migration.drop_table :_test_loose_fk_test_table end before do @@ -37,7 +37,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do context 'when the record deletion tracker trigger is installed' do before do - migration.track_record_deletions(:loose_fk_test_table) + migration.track_record_deletions(:_test_loose_fk_test_table) end it 'stores the record deletion' do @@ -50,7 +50,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers do deleted_record = LooseForeignKeys::DeletedRecord.all.first expect(deleted_record.primary_key_value).to eq(record_to_be_deleted.id) - expect(deleted_record.fully_qualified_table_name).to eq('public.loose_fk_test_table') + expect(deleted_record.fully_qualified_table_name).to eq('public._test_loose_fk_test_table') expect(deleted_record.partition).to eq(1) end diff --git a/spec/lib/gitlab/database/migration_helpers/v2_spec.rb b/spec/lib/gitlab/database/migration_helpers/v2_spec.rb index 854e97ef897..acf775b3538 100644 --- a/spec/lib/gitlab/database/migration_helpers/v2_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/v2_spec.rb @@ -20,7 +20,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do let(:model) { Class.new(ActiveRecord::Base) } before do - model.table_name = :test_table + model.table_name = :_test_table end context 'when called inside a transaction block' do @@ -30,19 +30,19 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do it 'raises an error' do expect do - migration.public_send(operation, :test_table, :original, :renamed) + migration.public_send(operation, :_test_table, :original, :renamed) end.to raise_error("#{operation} can not be run inside a transaction") end end context 'when the existing column has a default value' do before do - migration.change_column_default :test_table, existing_column, 'default value' + migration.change_column_default :_test_table, existing_column, 'default value' end it 'raises an error' do expect do - migration.public_send(operation, :test_table, :original, :renamed) + migration.public_send(operation, :_test_table, :original, :renamed) end.to raise_error("#{operation} does not currently support columns with default values") end end @@ -51,18 +51,18 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do context 'when the batch column does not exist' do it 'raises an error' do expect do - migration.public_send(operation, :test_table, :original, :renamed, batch_column_name: :missing) - end.to raise_error('Column missing does not exist on test_table') + migration.public_send(operation, :_test_table, :original, :renamed, batch_column_name: :missing) + end.to raise_error('Column missing does not exist on _test_table') end end context 'when the batch column does exist' do it 'passes it when creating the column' do expect(migration).to receive(:create_column_from) - .with(:test_table, existing_column, added_column, type: nil, batch_column_name: :status) + .with(:_test_table, existing_column, added_column, type: nil, batch_column_name: :status) .and_call_original - migration.public_send(operation, :test_table, :original, :renamed, batch_column_name: :status) + migration.public_send(operation, :_test_table, :original, :renamed, batch_column_name: :status) end end end @@ -71,17 +71,17 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do existing_record_1 = model.create!(status: 0, existing_column => 'existing') existing_record_2 = model.create!(status: 0, existing_column => nil) - migration.send(operation, :test_table, :original, :renamed) + migration.send(operation, :_test_table, :original, :renamed) model.reset_column_information - expect(migration.column_exists?(:test_table, added_column)).to eq(true) + expect(migration.column_exists?(:_test_table, added_column)).to eq(true) expect(existing_record_1.reload).to have_attributes(status: 0, original: 'existing', renamed: 'existing') expect(existing_record_2.reload).to have_attributes(status: 0, original: nil, renamed: nil) end it 'installs triggers to sync new data' do - migration.public_send(operation, :test_table, :original, :renamed) + migration.public_send(operation, :_test_table, :original, :renamed) model.reset_column_information new_record_1 = model.create!(status: 1, original: 'first') @@ -102,7 +102,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do before do allow(migration).to receive(:transaction_open?).and_return(false) - migration.create_table :test_table do |t| + migration.create_table :_test_table do |t| t.integer :status, null: false t.text :original t.text :other_column @@ -118,8 +118,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do context 'when the column to rename does not exist' do it 'raises an error' do expect do - migration.rename_column_concurrently :test_table, :missing_column, :renamed - end.to raise_error('Column missing_column does not exist on test_table') + migration.rename_column_concurrently :_test_table, :missing_column, :renamed + end.to raise_error('Column missing_column does not exist on _test_table') end end end @@ -128,7 +128,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do before do allow(migration).to receive(:transaction_open?).and_return(false) - migration.create_table :test_table do |t| + migration.create_table :_test_table do |t| t.integer :status, null: false t.text :other_column t.text :renamed @@ -144,8 +144,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do context 'when the renamed column does not exist' do it 'raises an error' do expect do - migration.undo_cleanup_concurrent_column_rename :test_table, :original, :missing_column - end.to raise_error('Column missing_column does not exist on test_table') + migration.undo_cleanup_concurrent_column_rename :_test_table, :original, :missing_column + end.to raise_error('Column missing_column does not exist on _test_table') end end end @@ -156,25 +156,25 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do before do allow(migration).to receive(:transaction_open?).and_return(false) - migration.create_table :test_table do |t| + migration.create_table :_test_table do |t| t.integer :status, null: false t.text :original t.text :other_column end - migration.rename_column_concurrently :test_table, :original, :renamed + migration.rename_column_concurrently :_test_table, :original, :renamed end context 'when the helper is called repeatedly' do before do - migration.public_send(operation, :test_table, :original, :renamed) + migration.public_send(operation, :_test_table, :original, :renamed) end it 'does not make repeated attempts to cleanup' do expect(migration).not_to receive(:remove_column) expect do - migration.public_send(operation, :test_table, :original, :renamed) + migration.public_send(operation, :_test_table, :original, :renamed) end.not_to raise_error end end @@ -182,26 +182,26 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do context 'when the renamed column exists' do let(:triggers) do [ - ['trigger_7cc71f92fd63', 'function_for_trigger_7cc71f92fd63', before: 'insert'], - ['trigger_f1a1f619636a', 'function_for_trigger_f1a1f619636a', before: 'update'], - ['trigger_769a49938884', 'function_for_trigger_769a49938884', before: 'update'] + ['trigger_020dbcb8cdd0', 'function_for_trigger_020dbcb8cdd0', before: 'insert'], + ['trigger_6edaca641d03', 'function_for_trigger_6edaca641d03', before: 'update'], + ['trigger_a3fb9f3add34', 'function_for_trigger_a3fb9f3add34', before: 'update'] ] end it 'removes the sync triggers and renamed columns' do triggers.each do |(trigger_name, function_name, event)| expect_function_to_exist(function_name) - expect_valid_function_trigger(:test_table, trigger_name, function_name, event) + expect_valid_function_trigger(:_test_table, trigger_name, function_name, event) end - expect(migration.column_exists?(:test_table, added_column)).to eq(true) + expect(migration.column_exists?(:_test_table, added_column)).to eq(true) - migration.public_send(operation, :test_table, :original, :renamed) + migration.public_send(operation, :_test_table, :original, :renamed) - expect(migration.column_exists?(:test_table, added_column)).to eq(false) + expect(migration.column_exists?(:_test_table, added_column)).to eq(false) triggers.each do |(trigger_name, function_name, _)| - expect_trigger_not_to_exist(:test_table, trigger_name) + expect_trigger_not_to_exist(:_test_table, trigger_name) expect_function_not_to_exist(function_name) end end @@ -223,7 +223,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do end describe '#create_table' do - let(:table_name) { :test_table } + let(:table_name) { :_test_table } let(:column_attributes) do [ { name: 'id', sql_type: 'bigint', null: false, default: nil }, @@ -245,7 +245,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do end expect_table_columns_to_match(column_attributes, table_name) - expect_check_constraint(table_name, 'check_cda6f69506', 'char_length(name) <= 100') + expect_check_constraint(table_name, 'check_e9982cf9da', 'char_length(name) <= 100') end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index d89af1521a2..ea755f5a368 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -31,16 +31,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe '#add_timestamps_with_timezone' do - let(:in_transaction) { false } - - before do - allow(model).to receive(:transaction_open?).and_return(in_transaction) - allow(model).to receive(:disable_statement_timeout) - end - it 'adds "created_at" and "updated_at" fields with the "datetime_with_timezone" data type' do Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name| - expect(model).to receive(:add_column).with(:foo, column_name, :datetime_with_timezone, { null: false }) + expect(model).to receive(:add_column) + .with(:foo, column_name, :datetime_with_timezone, { default: nil, null: false }) end model.add_timestamps_with_timezone(:foo) @@ -48,7 +42,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do it 'can disable the NOT NULL constraint' do Gitlab::Database::MigrationHelpers::DEFAULT_TIMESTAMP_COLUMNS.each do |column_name| - expect(model).to receive(:add_column).with(:foo, column_name, :datetime_with_timezone, { null: true }) + expect(model).to receive(:add_column) + .with(:foo, column_name, :datetime_with_timezone, { default: nil, null: true }) end model.add_timestamps_with_timezone(:foo, null: true) @@ -64,9 +59,10 @@ RSpec.describe Gitlab::Database::MigrationHelpers do it 'can add choice of acceptable columns' do expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, anything) expect(model).to receive(:add_column).with(:foo, :deleted_at, :datetime_with_timezone, anything) + expect(model).to receive(:add_column).with(:foo, :processed_at, :datetime_with_timezone, anything) expect(model).not_to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, anything) - model.add_timestamps_with_timezone(:foo, columns: [:created_at, :deleted_at]) + model.add_timestamps_with_timezone(:foo, columns: [:created_at, :deleted_at, :processed_at]) end it 'cannot add unacceptable column names' do @@ -74,29 +70,6 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.add_timestamps_with_timezone(:foo, columns: [:bar]) end.to raise_error %r/Illegal timestamp column name/ end - - context 'in a transaction' do - let(:in_transaction) { true } - - before do - allow(model).to receive(:add_column).with(any_args).and_call_original - allow(model).to receive(:add_column) - .with(:foo, anything, :datetime_with_timezone, anything) - .and_return(nil) - end - - it 'cannot add a default value' do - expect do - model.add_timestamps_with_timezone(:foo, default: :i_cause_an_error) - end.to raise_error %r/add_timestamps_with_timezone/ - end - - it 'can add columns without defaults' do - expect do - model.add_timestamps_with_timezone(:foo) - end.not_to raise_error - end - end end describe '#create_table_with_constraints' do @@ -271,12 +244,92 @@ RSpec.describe Gitlab::Database::MigrationHelpers do model.add_concurrent_index(:users, :foo, unique: true) end - it 'does nothing if the index exists already' do - expect(model).to receive(:index_exists?) - .with(:users, :foo, { algorithm: :concurrently, unique: true }).and_return(true) - expect(model).not_to receive(:add_index) + context 'when the index exists and is valid' do + before do + model.add_index :users, :id, unique: true + end - model.add_concurrent_index(:users, :foo, unique: true) + it 'does leaves the existing index' do + expect(model).to receive(:index_exists?) + .with(:users, :id, { algorithm: :concurrently, unique: true }).and_call_original + + expect(model).not_to receive(:remove_index) + expect(model).not_to receive(:add_index) + + model.add_concurrent_index(:users, :id, unique: true) + end + end + + context 'when an invalid copy of the index exists' do + before do + model.add_index :users, :id, unique: true, name: index_name + + model.connection.execute(<<~SQL) + UPDATE pg_index + SET indisvalid = false + WHERE indexrelid = '#{index_name}'::regclass + SQL + end + + context 'when the default name is used' do + let(:index_name) { model.index_name(:users, :id) } + + it 'drops and recreates the index' do + expect(model).to receive(:index_exists?) + .with(:users, :id, { algorithm: :concurrently, unique: true }).and_call_original + expect(model).to receive(:index_invalid?).with(index_name, schema: nil).and_call_original + + expect(model).to receive(:remove_concurrent_index_by_name).with(:users, index_name) + + expect(model).to receive(:add_index) + .with(:users, :id, { algorithm: :concurrently, unique: true }) + + model.add_concurrent_index(:users, :id, unique: true) + end + end + + context 'when a custom name is used' do + let(:index_name) { 'my_test_index' } + + it 'drops and recreates the index' do + expect(model).to receive(:index_exists?) + .with(:users, :id, { algorithm: :concurrently, unique: true, name: index_name }).and_call_original + expect(model).to receive(:index_invalid?).with(index_name, schema: nil).and_call_original + + expect(model).to receive(:remove_concurrent_index_by_name).with(:users, index_name) + + expect(model).to receive(:add_index) + .with(:users, :id, { algorithm: :concurrently, unique: true, name: index_name }) + + model.add_concurrent_index(:users, :id, unique: true, name: index_name) + end + end + + context 'when a qualified table name is used' do + let(:other_schema) { 'foo_schema' } + let(:index_name) { 'my_test_index' } + let(:table_name) { "#{other_schema}.users" } + + before do + model.connection.execute(<<~SQL) + CREATE SCHEMA #{other_schema}; + ALTER TABLE users SET SCHEMA #{other_schema}; + SQL + end + + it 'drops and recreates the index' do + expect(model).to receive(:index_exists?) + .with(table_name, :id, { algorithm: :concurrently, unique: true, name: index_name }).and_call_original + expect(model).to receive(:index_invalid?).with(index_name, schema: other_schema).and_call_original + + expect(model).to receive(:remove_concurrent_index_by_name).with(table_name, index_name) + + expect(model).to receive(:add_index) + .with(table_name, :id, { algorithm: :concurrently, unique: true, name: index_name }) + + model.add_concurrent_index(table_name, :id, unique: true, name: index_name) + end + end end it 'unprepares the async index creation' do diff --git a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb index 1a7116e75e5..e42a6c970ea 100644 --- a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb @@ -583,12 +583,33 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do end describe '#finalized_background_migration' do - include_context 'background migration job class' + let(:job_coordinator) { Gitlab::BackgroundMigration::JobCoordinator.new(:main, BackgroundMigrationWorker) } + + let!(:job_class_name) { 'TestJob' } + let!(:job_class) { Class.new } + let!(:job_perform_method) do + ->(*arguments) do + Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded( + # Value is 'TestJob' defined by :job_class_name in the let! above. + # Scoping prohibits us from directly referencing job_class_name. + RSpec.current_example.example_group_instance.job_class_name, + arguments + ) + end + end let!(:tracked_pending_job) { create(:background_migration_job, class_name: job_class_name, status: :pending, arguments: [1]) } let!(:tracked_successful_job) { create(:background_migration_job, class_name: job_class_name, status: :succeeded, arguments: [2]) } before do + job_class.define_method(:perform, job_perform_method) + + allow(Gitlab::BackgroundMigration).to receive(:coordinator_for_database) + .with(:main).and_return(job_coordinator) + + expect(job_coordinator).to receive(:migration_class_for) + .with(job_class_name).at_least(:once) { job_class } + Sidekiq::Testing.disable! do BackgroundMigrationWorker.perform_async(job_class_name, [1, 2]) BackgroundMigrationWorker.perform_async(job_class_name, [3, 4]) diff --git a/spec/lib/gitlab/database/migrations/observers/transaction_duration_spec.rb b/spec/lib/gitlab/database/migrations/observers/transaction_duration_spec.rb new file mode 100644 index 00000000000..e65f89747c4 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/observers/transaction_duration_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::Observers::TransactionDuration do + subject(:transaction_duration_observer) { described_class.new(observation, directory_path) } + + let(:observation) { Gitlab::Database::Migrations::Observation.new(migration_version, migration_name) } + let(:directory_path) { Dir.mktmpdir } + let(:log_file) { "#{directory_path}/#{migration_version}_#{migration_name}-transaction-duration.json" } + let(:transaction_duration) { Gitlab::Json.parse(File.read(log_file)) } + let(:migration_version) { 20210422152437 } + let(:migration_name) { 'test' } + + after do + FileUtils.remove_entry(directory_path) + end + + it 'records real and sub transactions duration', :delete do + observe + + entry = transaction_duration[0] + start_time, end_time, transaction_type = entry.values_at('start_time', 'end_time', 'transaction_type') + start_time = DateTime.parse(start_time) + end_time = DateTime.parse(end_time) + + aggregate_failures do + expect(transaction_duration.size).to eq(3) + expect(start_time).to be_before(end_time) + expect(transaction_type).not_to be_nil + end + end + + context 'when there are sub-transactions' do + it 'records transaction duration' do + observe_sub_transaction + + expect(transaction_duration.size).to eq(1) + + entry = transaction_duration[0]['transaction_type'] + + expect(entry).to eql 'sub_transaction' + end + end + + context 'when there are real-transactions' do + it 'records transaction duration', :delete do + observe_real_transaction + + expect(transaction_duration.size).to eq(1) + + entry = transaction_duration[0]['transaction_type'] + + expect(entry).to eql 'real_transaction' + end + end + + private + + def observe + transaction_duration_observer.before + run_transaction + transaction_duration_observer.after + transaction_duration_observer.record + end + + def observe_sub_transaction + transaction_duration_observer.before + run_sub_transactions + transaction_duration_observer.after + transaction_duration_observer.record + end + + def observe_real_transaction + transaction_duration_observer.before + run_real_transactions + transaction_duration_observer.after + transaction_duration_observer.record + end + + def run_real_transactions + ActiveRecord::Base.transaction do + end + end + + def run_sub_transactions + ActiveRecord::Base.transaction(requires_new: true) do + end + end + + def run_transaction + ActiveRecord::Base.connection_pool.with_connection do |connection| + Gitlab::Database::SharedModel.using_connection(connection) do + Gitlab::Database::SharedModel.transaction do + Gitlab::Database::SharedModel.transaction(requires_new: true) do + Gitlab::Database::SharedModel.transaction do + Gitlab::Database::SharedModel.transaction do + Gitlab::Database::SharedModel.transaction(requires_new: true) do + end + end + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/database/partitioning/detached_partition_dropper_spec.rb b/spec/lib/gitlab/database/partitioning/detached_partition_dropper_spec.rb index 8c406c90e36..b2c4e4b54a4 100644 --- a/spec/lib/gitlab/database/partitioning/detached_partition_dropper_spec.rb +++ b/spec/lib/gitlab/database/partitioning/detached_partition_dropper_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do include Database::TableSchemaHelpers + subject(:dropper) { described_class.new } + let(:connection) { ActiveRecord::Base.connection } def expect_partition_present(name) @@ -23,10 +25,18 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do before do connection.execute(<<~SQL) + CREATE TABLE referenced_table ( + id bigserial primary key not null + ) + SQL + connection.execute(<<~SQL) + CREATE TABLE parent_table ( id bigserial not null, + referenced_id bigint not null, created_at timestamptz not null, - primary key (id, created_at) + primary key (id, created_at), + constraint fk_referenced foreign key (referenced_id) references referenced_table(id) ) PARTITION BY RANGE(created_at) SQL end @@ -59,7 +69,7 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do attached: false, drop_after: 1.day.from_now) - subject.perform + dropper.perform expect_partition_present('test_partition') end @@ -75,7 +85,7 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do end it 'drops the partition' do - subject.perform + dropper.perform expect(table_oid('test_partition')).to be_nil end @@ -86,16 +96,62 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do end it 'does not drop the partition' do - subject.perform + dropper.perform expect(table_oid('test_partition')).not_to be_nil end end + context 'removing foreign keys' do + it 'removes foreign keys from the table before dropping it' do + expect(dropper).to receive(:drop_detached_partition).and_wrap_original do |drop_method, partition_name| + expect(partition_name).to eq('test_partition') + expect(foreign_key_exists_by_name(partition_name, 'fk_referenced', schema: Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA)).to be_falsey + + drop_method.call(partition_name) + end + + expect(foreign_key_exists_by_name('test_partition', 'fk_referenced', schema: Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA)).to be_truthy + + dropper.perform + end + + it 'does not remove foreign keys from the parent table' do + expect { dropper.perform }.not_to change { foreign_key_exists_by_name('parent_table', 'fk_referenced') }.from(true) + end + + context 'when another process drops the foreign key' do + it 'skips dropping that foreign key' do + expect(dropper).to receive(:drop_foreign_key_if_present).and_wrap_original do |drop_meth, *args| + connection.execute('alter table gitlab_partitions_dynamic.test_partition drop constraint fk_referenced;') + drop_meth.call(*args) + end + + dropper.perform + + expect_partition_removed('test_partition') + end + end + + context 'when another process drops the partition' do + it 'skips dropping the foreign key' do + expect(dropper).to receive(:drop_foreign_key_if_present).and_wrap_original do |drop_meth, *args| + connection.execute('drop table gitlab_partitions_dynamic.test_partition') + Postgresql::DetachedPartition.where(table_name: 'test_partition').delete_all + end + + expect(Gitlab::AppLogger).not_to receive(:error) + dropper.perform + end + end + end + context 'when another process drops the table while the first waits for a lock' do it 'skips the table' do + # First call to .lock is for removing foreign keys + expect(Postgresql::DetachedPartition).to receive(:lock).once.ordered.and_call_original # Rspec's receive_method_chain does not support .and_wrap_original, so we need to nest here. - expect(Postgresql::DetachedPartition).to receive(:lock).and_wrap_original do |lock_meth| + expect(Postgresql::DetachedPartition).to receive(:lock).once.ordered.and_wrap_original do |lock_meth| locked = lock_meth.call expect(locked).to receive(:find_by).and_wrap_original do |find_meth, *find_args| # Another process drops the table then deletes this entry @@ -106,9 +162,9 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do locked end - expect(subject).not_to receive(:drop_one) + expect(dropper).not_to receive(:drop_one) - subject.perform + dropper.perform end end end @@ -123,19 +179,26 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do end it 'does not drop the partition, but does remove the DetachedPartition entry' do - subject.perform + dropper.perform aggregate_failures do expect(table_oid('test_partition')).not_to be_nil expect(Postgresql::DetachedPartition.find_by(table_name: 'test_partition')).to be_nil end end - it 'removes the detached_partition entry' do - detached_partition = Postgresql::DetachedPartition.find_by!(table_name: 'test_partition') + context 'when another process removes the entry before this process' do + it 'does nothing' do + expect(Postgresql::DetachedPartition).to receive(:lock).and_wrap_original do |lock_meth| + Postgresql::DetachedPartition.delete_all + lock_meth.call + end - subject.perform + expect(Gitlab::AppLogger).not_to receive(:error) - expect(Postgresql::DetachedPartition.exists?(id: detached_partition.id)).to be_falsey + dropper.perform + + expect(table_oid('test_partition')).not_to be_nil + end end end @@ -155,7 +218,7 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do end it 'drops both partitions' do - subject.perform + dropper.perform expect_partition_removed('partition_1') expect_partition_removed('partition_2') @@ -163,10 +226,10 @@ RSpec.describe Gitlab::Database::Partitioning::DetachedPartitionDropper do context 'when the first drop returns an error' do it 'still drops the second partition' do - expect(subject).to receive(:drop_detached_partition).ordered.and_raise('injected error') - expect(subject).to receive(:drop_detached_partition).ordered.and_call_original + expect(dropper).to receive(:drop_detached_partition).ordered.and_raise('injected error') + expect(dropper).to receive(:drop_detached_partition).ordered.and_call_original - subject.perform + dropper.perform # We don't know which partition we tried to drop first, so the tests here have to work with either one expect(Postgresql::DetachedPartition.count).to eq(1) diff --git a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb index 27ada12b067..67d80d71e2a 100644 --- a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do let(:model) { double('model', table_name: table_name) } let(:partitioning_key) { double } - let(:table_name) { :partitioned_test } + let(:table_name) { :_test_partitioned_test } before do connection.execute(<<~SQL) @@ -18,11 +18,11 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do (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 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_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 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_partitioned_test_202005 PARTITION OF #{table_name} FOR VALUES FROM ('2020-05-01') TO ('2020-06-01'); SQL @@ -30,8 +30,8 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do it 'detects both partitions' do expect(subject).to eq([ - Gitlab::Database::Partitioning::TimePartition.new(table_name, nil, '2020-05-01', partition_name: 'partitioned_test_000000'), - Gitlab::Database::Partitioning::TimePartition.new(table_name, '2020-05-01', '2020-06-01', partition_name: 'partitioned_test_202005') + Gitlab::Database::Partitioning::TimePartition.new(table_name, nil, '2020-05-01', partition_name: '_test_partitioned_test_000000'), + Gitlab::Database::Partitioning::TimePartition.new(table_name, '2020-05-01', '2020-06-01', partition_name: '_test_partitioned_test_202005') ]) end end @@ -41,7 +41,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do let(:model) do Class.new(ActiveRecord::Base) do - self.table_name = 'partitioned_test' + self.table_name = '_test_partitioned_test' self.primary_key = :id end end @@ -59,11 +59,11 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do (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 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_partitioned_test_000000 PARTITION OF #{model.table_name} FOR VALUES FROM (MINVALUE) TO ('2020-05-01'); - CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.partitioned_test_202006 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_partitioned_test_202006 PARTITION OF #{model.table_name} FOR VALUES FROM ('2020-06-01') TO ('2020-07-01'); SQL @@ -166,7 +166,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do (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_202006 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_partitioned_test_202006 PARTITION OF #{model.table_name} FOR VALUES FROM ('2020-06-01') TO ('2020-07-01'); SQL @@ -181,13 +181,13 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do describe '#extra_partitions' do let(:model) do Class.new(ActiveRecord::Base) do - self.table_name = 'partitioned_test' + self.table_name = '_test_partitioned_test' self.primary_key = :id end end let(:partitioning_key) { :created_at } - let(:table_name) { :partitioned_test } + let(:table_name) { :_test_partitioned_test } around do |example| travel_to(Date.parse('2020-08-22')) { example.run } @@ -200,15 +200,15 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do (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 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_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 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_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 + CREATE TABLE #{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}._test_partitioned_test_202006 PARTITION OF #{table_name} FOR VALUES FROM ('2020-06-01') TO ('2020-07-01') SQL @@ -235,7 +235,7 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do 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') + partition_name: '_test_partitioned_test_000000') expect(subject).to contain_exactly(min_value_to_may) end @@ -246,8 +246,8 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do 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') + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: '_test_partitioned_test_000000'), + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01', partition_name: '_test_partitioned_test_202005') ) end @@ -256,16 +256,16 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do it 'prunes empty partitions' do expect(subject).to contain_exactly( - Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: 'partitioned_test_000000'), - Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01', partition_name: 'partitioned_test_202005') + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: '_test_partitioned_test_000000'), + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, '2020-05-01', '2020-06-01', partition_name: '_test_partitioned_test_202005') ) end it 'does not prune non-empty partitions' do - connection.execute("INSERT INTO #{table_name} (created_at) VALUES (('2020-05-15'))") # inserting one record into partitioned_test_202005 + connection.execute("INSERT INTO #{table_name} (created_at) VALUES (('2020-05-15'))") # inserting one record into _test_partitioned_test_202005 expect(subject).to contain_exactly( - Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: 'partitioned_test_000000') + Gitlab::Database::Partitioning::TimePartition.new(model.table_name, nil, '2020-05-01', partition_name: '_test_partitioned_test_000000') ) end end diff --git a/spec/lib/gitlab/database/partitioning/multi_database_partition_dropper_spec.rb b/spec/lib/gitlab/database/partitioning/multi_database_partition_dropper_spec.rb deleted file mode 100644 index 56d6ebb7aff..00000000000 --- a/spec/lib/gitlab/database/partitioning/multi_database_partition_dropper_spec.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::Partitioning::MultiDatabasePartitionDropper, '#drop_detached_partitions' do - subject(:drop_detached_partitions) { multi_db_dropper.drop_detached_partitions } - - let(:multi_db_dropper) { described_class.new } - - let(:connection_wrapper1) { double(scope: scope1) } - let(:connection_wrapper2) { double(scope: scope2) } - - let(:scope1) { double(connection: connection1) } - let(:scope2) { double(connection: connection2) } - - let(:connection1) { double('connection') } - let(:connection2) { double('connection') } - - let(:dropper_class) { Gitlab::Database::Partitioning::DetachedPartitionDropper } - let(:dropper1) { double('partition dropper') } - let(:dropper2) { double('partition dropper') } - - before do - allow(multi_db_dropper).to receive(:databases).and_return({ db1: connection_wrapper1, db2: connection_wrapper2 }) - end - - it 'drops detached partitions for each database' do - expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(connection1).and_yield.ordered - expect(dropper_class).to receive(:new).and_return(dropper1).ordered - expect(dropper1).to receive(:perform) - - expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(connection2).and_yield.ordered - expect(dropper_class).to receive(:new).and_return(dropper2).ordered - expect(dropper2).to receive(:perform) - - drop_detached_partitions - end -end diff --git a/spec/lib/gitlab/database/partitioning/multi_database_partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/multi_database_partition_manager_spec.rb deleted file mode 100644 index 3c94c1bf4ea..00000000000 --- a/spec/lib/gitlab/database/partitioning/multi_database_partition_manager_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Database::Partitioning::MultiDatabasePartitionManager, '#sync_partitions' do - subject(:sync_partitions) { manager.sync_partitions } - - let(:manager) { described_class.new(models) } - let(:models) { [model1, model2] } - - let(:model1) { double('model1', connection: connection1, table_name: 'table1') } - let(:model2) { double('model2', connection: connection1, table_name: 'table2') } - - let(:connection1) { double('connection1') } - let(:connection2) { double('connection2') } - - let(:target_manager_class) { Gitlab::Database::Partitioning::PartitionManager } - let(:target_manager1) { double('partition manager') } - let(:target_manager2) { double('partition manager') } - - before do - allow(manager).to receive(:connection_name).and_return('name') - end - - it 'syncs model partitions, setting up the appropriate connection for each', :aggregate_failures do - expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(model1.connection).and_yield.ordered - expect(target_manager_class).to receive(:new).with(model1).and_return(target_manager1).ordered - expect(target_manager1).to receive(:sync_partitions) - - expect(Gitlab::Database::SharedModel).to receive(:using_connection).with(model2.connection).and_yield.ordered - expect(target_manager_class).to receive(:new).with(model2).and_return(target_manager2).ordered - expect(target_manager2).to receive(:sync_partitions) - - sync_partitions - end -end diff --git a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb index 7c4cfcfb3a9..1c6f5c5c694 100644 --- a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb @@ -195,7 +195,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do end # Postgres 11 does not support foreign keys to partitioned tables - if Gitlab::Database.main.version.to_f >= 12 + if ApplicationRecord.database.version.to_f >= 12 context 'when the model is the target of a foreign key' do before do connection.execute(<<~SQL) diff --git a/spec/lib/gitlab/database/partitioning/partition_monitoring_spec.rb b/spec/lib/gitlab/database/partitioning/partition_monitoring_spec.rb index 7024cbd55ff..006ce8a7f48 100644 --- a/spec/lib/gitlab/database/partitioning/partition_monitoring_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_monitoring_spec.rb @@ -4,9 +4,8 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning::PartitionMonitoring do describe '#report_metrics' do - subject { described_class.new(models).report_metrics } + subject { described_class.new.report_metrics_for_model(model) } - let(:models) { [model] } let(:model) { double(partitioning_strategy: partitioning_strategy, table_name: table) } let(:partitioning_strategy) { double(missing_partitions: missing_partitions, current_partitions: current_partitions, extra_partitions: extra_partitions) } let(:table) { "some_table" } diff --git a/spec/lib/gitlab/database/partitioning/replace_table_spec.rb b/spec/lib/gitlab/database/partitioning/replace_table_spec.rb index 8e27797208c..fdf514b519f 100644 --- a/spec/lib/gitlab/database/partitioning/replace_table_spec.rb +++ b/spec/lib/gitlab/database/partitioning/replace_table_spec.rb @@ -5,7 +5,9 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning::ReplaceTable, '#perform' do include Database::TableSchemaHelpers - subject(:replace_table) { described_class.new(original_table, replacement_table, archived_table, 'id').perform } + subject(:replace_table) do + described_class.new(connection, original_table, replacement_table, archived_table, 'id').perform + end let(:original_table) { '_test_original_table' } let(:replacement_table) { '_test_replacement_table' } diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index 486af9413e8..154cc2b7972 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -3,52 +3,175 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Partitioning do + include Database::PartitioningHelpers + include Database::TableSchemaHelpers + + let(:connection) { ApplicationRecord.connection } + + around do |example| + previously_registered_models = described_class.registered_models.dup + described_class.instance_variable_set('@registered_models', Set.new) + + previously_registered_tables = described_class.registered_tables.dup + described_class.instance_variable_set('@registered_tables', Set.new) + + example.run + + described_class.instance_variable_set('@registered_models', previously_registered_models) + described_class.instance_variable_set('@registered_tables', previously_registered_tables) + end + + describe '.register_models' do + context 'ensure that the registered models have partitioning strategy' do + it 'fails when partitioning_strategy is not specified for the model' do + model = Class.new(ApplicationRecord) + expect { described_class.register_models([model]) }.to raise_error /should have partitioning strategy defined/ + end + end + end + + describe '.sync_partitions_ignore_db_error' do + it 'calls sync_partitions' do + expect(described_class).to receive(:sync_partitions) + + described_class.sync_partitions_ignore_db_error + end + + [ActiveRecord::ActiveRecordError, PG::Error].each do |error| + context "when #{error} is raised" do + before do + expect(described_class).to receive(:sync_partitions) + .and_raise(error) + end + + it 'ignores it' do + described_class.sync_partitions_ignore_db_error + end + end + end + + context 'when DISABLE_POSTGRES_PARTITION_CREATION_ON_STARTUP is set' do + before do + stub_env('DISABLE_POSTGRES_PARTITION_CREATION_ON_STARTUP', '1') + end + + it 'does not call sync_partitions' do + expect(described_class).to receive(:sync_partitions).never + + described_class.sync_partitions_ignore_db_error + end + end + end + describe '.sync_partitions' do - let(:partition_manager_class) { described_class::MultiDatabasePartitionManager } - let(:partition_manager) { double('partition manager') } + let(:table_names) { %w[partitioning_test1 partitioning_test2] } + let(:models) do + table_names.map do |table_name| + Class.new(ApplicationRecord) do + include PartitionedTable + + self.table_name = table_name + partitioned_by :created_at, strategy: :monthly + end + end + end + + before do + table_names.each do |table_name| + 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); + SQL + end + end + + it 'manages partitions for each given model' do + expect { described_class.sync_partitions(models)} + .to change { find_partitions(table_names.first).size }.from(0) + .and change { find_partitions(table_names.last).size }.from(0) + end context 'when no partitioned models are given' do - it 'calls the partition manager with the registered models' do - expect(partition_manager_class).to receive(:new) - .with(described_class.registered_models) - .and_return(partition_manager) + it 'manages partitions for each registered model' do + described_class.register_models([models.first]) + described_class.register_tables([ + { + table_name: table_names.last, + partitioned_column: :created_at, strategy: :monthly + } + ]) - expect(partition_manager).to receive(:sync_partitions) + expect { described_class.sync_partitions } + .to change { find_partitions(table_names.first).size }.from(0) + .and change { find_partitions(table_names.last).size }.from(0) + end + end + end + + describe '.report_metrics' do + let(:model1) { double('model') } + let(:model2) { double('model') } + + let(:partition_monitoring_class) { described_class::PartitionMonitoring } + + context 'when no partitioned models are given' do + it 'reports metrics for each registered model' do + expect_next_instance_of(partition_monitoring_class) do |partition_monitor| + expect(partition_monitor).to receive(:report_metrics_for_model).with(model1) + expect(partition_monitor).to receive(:report_metrics_for_model).with(model2) + end + + expect(Gitlab::Database::EachDatabase).to receive(:each_model_connection) + .with(described_class.__send__(:registered_models)) + .and_yield(model1) + .and_yield(model2) - described_class.sync_partitions + described_class.report_metrics end end context 'when partitioned models are given' do - it 'calls the partition manager with the given models' do - models = ['my special model'] + it 'reports metrics for each given model' do + expect_next_instance_of(partition_monitoring_class) do |partition_monitor| + expect(partition_monitor).to receive(:report_metrics_for_model).with(model1) + expect(partition_monitor).to receive(:report_metrics_for_model).with(model2) + end - expect(partition_manager_class).to receive(:new) - .with(models) - .and_return(partition_manager) + expect(Gitlab::Database::EachDatabase).to receive(:each_model_connection) + .with([model1, model2]) + .and_yield(model1) + .and_yield(model2) - expect(partition_manager).to receive(:sync_partitions) - - described_class.sync_partitions(models) + described_class.report_metrics([model1, model2]) end end end describe '.drop_detached_partitions' do - let(:partition_dropper_class) { described_class::MultiDatabasePartitionDropper } + let(:table_names) { %w[detached_test_partition1 detached_test_partition2] } + + before do + table_names.each do |table_name| + connection.create_table("#{Gitlab::Database::DYNAMIC_PARTITIONS_SCHEMA}.#{table_name}") - it 'delegates to the partition dropper' do - expect_next_instance_of(partition_dropper_class) do |partition_dropper| - expect(partition_dropper).to receive(:drop_detached_partitions) + Postgresql::DetachedPartition.create!(table_name: table_name, drop_after: 1.year.ago) end + end - described_class.drop_detached_partitions + it 'drops detached partitions for each database' do + expect(Gitlab::Database::EachDatabase).to receive(:each_database_connection).and_yield + + expect { described_class.drop_detached_partitions } + .to change { Postgresql::DetachedPartition.count }.from(2).to(0) + .and change { table_exists?(table_names.first) }.from(true).to(false) + .and change { table_exists?(table_names.last) }.from(true).to(false) end - end - context 'ensure that the registered models have partitioning strategy' do - it 'fails when partitioning_strategy is not specified for the model' do - expect(described_class.registered_models).to all(respond_to(:partitioning_strategy)) + def table_exists?(table_name) + table_oid(table_name).present? end end end diff --git a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb index ec39e5bfee7..b0e08ca1e67 100644 --- a/spec/lib/gitlab/database/postgres_foreign_key_spec.rb +++ b/spec/lib/gitlab/database/postgres_foreign_key_spec.rb @@ -38,4 +38,16 @@ RSpec.describe Gitlab::Database::PostgresForeignKey, type: :model do expect(described_class.by_referenced_table_identifier('public.referenced_table')).to contain_exactly(expected) end end + + describe '#by_constrained_table_identifier' do + it 'throws an error when the identifier name is not fully qualified' do + expect { described_class.by_constrained_table_identifier('constrained_table') }.to raise_error(ArgumentError, /not fully qualified/) + end + + it 'finds the foreign keys for the constrained table' do + expected = described_class.where(name: %w[fk_constrained_to_referenced fk_constrained_to_other_referenced]).to_a + + expect(described_class.by_constrained_table_identifier('public.constrained_table')).to match_array(expected) + end + end end diff --git a/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb b/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb index 2c550f14a08..c9bbc32e059 100644 --- a/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb +++ b/spec/lib/gitlab/database/postgres_hll/batch_distinct_counter_spec.rb @@ -21,7 +21,7 @@ RSpec.describe Gitlab::Database::PostgresHll::BatchDistinctCounter do end before do - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(in_transaction) + allow(model.connection).to receive(:transaction_open?).and_return(in_transaction) end context 'unit test for different counting parameters' do diff --git a/spec/lib/gitlab/database/postgres_index_bloat_estimate_spec.rb b/spec/lib/gitlab/database/postgres_index_bloat_estimate_spec.rb index da4422bd442..13ac9190ab7 100644 --- a/spec/lib/gitlab/database/postgres_index_bloat_estimate_spec.rb +++ b/spec/lib/gitlab/database/postgres_index_bloat_estimate_spec.rb @@ -13,6 +13,8 @@ RSpec.describe Gitlab::Database::PostgresIndexBloatEstimate do let(:identifier) { 'public.schema_migrations_pkey' } + it { is_expected.to be_a Gitlab::Database::SharedModel } + describe '#bloat_size' do it 'returns the bloat size in bytes' do # We cannot reach much more about the bloat size estimate here diff --git a/spec/lib/gitlab/database/postgres_index_spec.rb b/spec/lib/gitlab/database/postgres_index_spec.rb index 9088719d5a4..db66736676b 100644 --- a/spec/lib/gitlab/database/postgres_index_spec.rb +++ b/spec/lib/gitlab/database/postgres_index_spec.rb @@ -22,6 +22,8 @@ RSpec.describe Gitlab::Database::PostgresIndex do it_behaves_like 'a postgres model' + it { is_expected.to be_a Gitlab::Database::SharedModel } + describe '.reindexing_support' do it 'only non partitioned indexes' do expect(described_class.reindexing_support).to all(have_attributes(partitioned: false)) diff --git a/spec/lib/gitlab/database/query_analyzer_spec.rb b/spec/lib/gitlab/database/query_analyzer_spec.rb new file mode 100644 index 00000000000..82a1c7143d5 --- /dev/null +++ b/spec/lib/gitlab/database/query_analyzer_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do + let(:analyzer) { double(:query_analyzer) } + let(:disabled_analyzer) { double(:disabled_query_analyzer) } + + before do + allow(described_class.instance).to receive(:all_analyzers).and_return([analyzer, disabled_analyzer]) + allow(analyzer).to receive(:enabled?).and_return(true) + allow(analyzer).to receive(:suppressed?).and_return(false) + allow(analyzer).to receive(:begin!) + allow(analyzer).to receive(:end!) + allow(disabled_analyzer).to receive(:enabled?).and_return(false) + end + + context 'the hook is enabled by default in specs' do + it 'does process queries and gets normalized SQL' do + expect(analyzer).to receive(:enabled?).and_return(true) + expect(analyzer).to receive(:analyze) do |parsed| + expect(parsed.sql).to include("SELECT $1 FROM projects") + expect(parsed.pg.tables).to eq(%w[projects]) + end + + described_class.instance.within do + Project.connection.execute("SELECT 1 FROM projects") + end + end + + it 'does prevent recursive execution' do + expect(analyzer).to receive(:enabled?).and_return(true) + expect(analyzer).to receive(:analyze) do + Project.connection.execute("SELECT 1 FROM projects") + end + + described_class.instance.within do + Project.connection.execute("SELECT 1 FROM projects") + end + end + end + + describe '#within' do + context 'when it is already initialized' do + around do |example| + described_class.instance.within do + example.run + end + end + + it 'does not evaluate enabled? again do yield block' do + expect(analyzer).not_to receive(:enabled?) + + expect { |b| described_class.instance.within(&b) }.to yield_control + end + end + + context 'when initializer is enabled' do + before do + expect(analyzer).to receive(:enabled?).and_return(true) + end + + it 'calls begin! and end!' do + expect(analyzer).to receive(:begin!) + expect(analyzer).to receive(:end!) + + expect { |b| described_class.instance.within(&b) }.to yield_control + end + + it 'when begin! raises the end! is not called' do + expect(analyzer).to receive(:begin!).and_raise('exception') + expect(analyzer).not_to receive(:end!) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + + expect { |b| described_class.instance.within(&b) }.to yield_control + end + end + end + + describe '#process_sql' do + it 'does not analyze query if not enabled' do + expect(analyzer).to receive(:enabled?).and_return(false) + expect(analyzer).not_to receive(:analyze) + + process_sql("SELECT 1 FROM projects") + end + + it 'does analyze query if enabled' do + expect(analyzer).to receive(:enabled?).and_return(true) + expect(analyzer).to receive(:analyze) do |parsed| + expect(parsed.sql).to eq("SELECT $1 FROM projects") + expect(parsed.pg.tables).to eq(%w[projects]) + end + + process_sql("SELECT 1 FROM projects") + end + + it 'does track exception if query cannot be parsed' do + expect(analyzer).to receive(:enabled?).and_return(true) + expect(analyzer).not_to receive(:analyze) + expect(Gitlab::ErrorTracking).to receive(:track_exception) + + expect { process_sql("invalid query") }.not_to raise_error + end + + it 'does track exception if analyzer raises exception on enabled?' do + expect(analyzer).to receive(:enabled?).and_raise('exception') + expect(analyzer).not_to receive(:analyze) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + + expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error + end + + it 'does track exception if analyzer raises exception on analyze' do + expect(analyzer).to receive(:enabled?).and_return(true) + expect(analyzer).to receive(:analyze).and_raise('exception') + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + + expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error + end + + it 'does call analyze only on enabled initializers' do + expect(analyzer).to receive(:analyze) + expect(disabled_analyzer).not_to receive(:analyze) + + expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error + end + + it 'does not call analyze on suppressed analyzers' do + expect(analyzer).to receive(:suppressed?).and_return(true) + expect(analyzer).not_to receive(:analyze) + + expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error + end + + def process_sql(sql) + described_class.instance.within do + ApplicationRecord.load_balancer.read_write do |connection| + described_class.instance.process_sql(sql, connection) + end + end + end + end +end diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb new file mode 100644 index 00000000000..ab5f05e3ec4 --- /dev/null +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_metrics_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasMetrics, query_analyzers: false do + let(:analyzer) { described_class } + + before do + allow(Gitlab::Database::QueryAnalyzer.instance).to receive(:all_analyzers).and_return([analyzer]) + end + + it 'does not increment metrics if feature flag is disabled' do + stub_feature_flags(query_analyzer_gitlab_schema_metrics: false) + + expect(analyzer).not_to receive(:analyze) + + process_sql(ActiveRecord::Base, "SELECT 1 FROM projects") + end + + context 'properly observes all queries', :mocked_ci_connection do + using RSpec::Parameterized::TableSyntax + + where do + { + "for simple query observes schema correctly" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM projects", + expectations: { + gitlab_schemas: "gitlab_main", + db_config_name: "main" + } + }, + "for query accessing gitlab_ci and gitlab_main" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM projects LEFT JOIN ci_builds ON ci_builds.project_id=projects.id", + expectations: { + gitlab_schemas: "gitlab_ci,gitlab_main", + db_config_name: "main" + } + }, + "for query accessing gitlab_ci and gitlab_main the gitlab_schemas is always ordered" => { + model: ApplicationRecord, + sql: "SELECT 1 FROM ci_builds LEFT JOIN projects ON ci_builds.project_id=projects.id", + expectations: { + gitlab_schemas: "gitlab_ci,gitlab_main", + db_config_name: "main" + } + }, + "for query accessing CI database" => { + model: Ci::ApplicationRecord, + sql: "SELECT 1 FROM ci_builds", + expectations: { + gitlab_schemas: "gitlab_ci", + db_config_name: "ci" + } + } + } + end + + with_them do + around do |example| + Gitlab::Database::QueryAnalyzer.instance.within { example.run } + end + + it do + expect(described_class.schemas_metrics).to receive(:increment) + .with(expectations).and_call_original + + process_sql(model, sql) + end + end + end + + def process_sql(model, sql) + Gitlab::Database::QueryAnalyzer.instance.within do + # Skip load balancer and retrieve connection assigned to model + Gitlab::Database::QueryAnalyzer.instance.process_sql(sql, model.retrieve_connection) + end + end +end diff --git a/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb b/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb new file mode 100644 index 00000000000..eb8ccb0bd89 --- /dev/null +++ b/spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification, query_analyzers: false do + let_it_be(:pipeline, refind: true) { create(:ci_pipeline) } + let_it_be(:project, refind: true) { create(:project) } + + before do + allow(Gitlab::Database::QueryAnalyzer.instance).to receive(:all_analyzers).and_return([described_class]) + end + + around do |example| + Gitlab::Database::QueryAnalyzer.instance.within { example.run } + end + + shared_examples 'successful examples' do + context 'outside transaction' do + it { expect { run_queries }.not_to raise_error } + end + + context 'within transaction' do + it do + Project.transaction do + expect { run_queries }.not_to raise_error + end + end + end + + context 'within nested transaction' do + it do + Project.transaction(requires_new: true) do + Project.transaction(requires_new: true) do + expect { run_queries }.not_to raise_error + end + end + end + end + end + + context 'when CI and other tables are read in a transaction' do + def run_queries + pipeline.reload + project.reload + end + + include_examples 'successful examples' + end + + context 'when only CI data is modified' do + def run_queries + pipeline.touch + project.reload + end + + include_examples 'successful examples' + end + + context 'when other data is modified' do + def run_queries + pipeline.reload + project.touch + end + + include_examples 'successful examples' + end + + context 'when both CI and other data is modified' do + def run_queries + project.touch + pipeline.touch + end + + context 'outside transaction' do + it { expect { run_queries }.not_to raise_error } + end + + context 'when data modification happens in a transaction' do + it 'raises error' do + Project.transaction do + expect { run_queries }.to raise_error /Cross-database data modification/ + end + end + + context 'when data modification happens in nested transactions' do + it 'raises error' do + Project.transaction(requires_new: true) do + project.touch + Project.transaction(requires_new: true) do + expect { pipeline.touch }.to raise_error /Cross-database data modification/ + end + end + end + end + end + + context 'when executing a SELECT FOR UPDATE query' do + def run_queries + project.touch + pipeline.lock! + end + + context 'outside transaction' do + it { expect { run_queries }.not_to raise_error } + end + + context 'when data modification happens in a transaction' do + it 'raises error' do + Project.transaction do + expect { run_queries }.to raise_error /Cross-database data modification/ + end + end + + context 'when the modification is inside a factory save! call' do + let(:runner) { create(:ci_runner, :project, projects: [build(:project)]) } + + it 'does not raise an error' do + runner + end + end + end + end + + context 'when CI association is modified through project' do + def run_queries + project.variables.build(key: 'a', value: 'v') + project.save! + end + + include_examples 'successful examples' + end + + describe '.allow_cross_database_modification_within_transaction' do + it 'skips raising error' do + expect do + described_class.allow_cross_database_modification_within_transaction(url: 'gitlab-issue') do + Project.transaction do + pipeline.touch + project.touch + end + end + end.not_to raise_error + end + + it 'skips raising error on factory creation' do + expect do + described_class.allow_cross_database_modification_within_transaction(url: 'gitlab-issue') do + ApplicationRecord.transaction do + create(:ci_pipeline) + end + end + end.not_to raise_error + end + end + end + + context 'when some table with a defined schema and another table with undefined gitlab_schema is modified' do + it 'raises an error including including message about undefined schema' do + expect do + Project.transaction do + project.touch + project.connection.execute('UPDATE foo_bars_undefined_table SET a=1 WHERE id = -1') + end + end.to raise_error /Cross-database data modification.*The gitlab_schema was undefined/ + end + end +end diff --git a/spec/lib/gitlab/database/reflection_spec.rb b/spec/lib/gitlab/database/reflection_spec.rb new file mode 100644 index 00000000000..7c3d797817d --- /dev/null +++ b/spec/lib/gitlab/database/reflection_spec.rb @@ -0,0 +1,280 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Reflection do + let(:database) { described_class.new(ApplicationRecord) } + + describe '#username' do + context 'when a username is set' do + it 'returns the username' do + allow(database).to receive(:config).and_return(username: 'bob') + + expect(database.username).to eq('bob') + end + end + + context 'when a username is not set' do + it 'returns the value of the USER environment variable' do + allow(database).to receive(:config).and_return(username: nil) + allow(ENV).to receive(:[]).with('USER').and_return('bob') + + expect(database.username).to eq('bob') + end + end + end + + describe '#database_name' do + it 'returns the name of the database' do + allow(database).to receive(:config).and_return(database: 'test') + + expect(database.database_name).to eq('test') + end + end + + describe '#adapter_name' do + it 'returns the database adapter name' do + allow(database).to receive(:config).and_return(adapter: 'test') + + expect(database.adapter_name).to eq('test') + end + end + + describe '#human_adapter_name' do + context 'when the adapter is PostgreSQL' do + it 'returns PostgreSQL' do + allow(database).to receive(:config).and_return(adapter: 'postgresql') + + expect(database.human_adapter_name).to eq('PostgreSQL') + end + end + + context 'when the adapter is not PostgreSQL' do + it 'returns Unknown' do + allow(database).to receive(:config).and_return(adapter: 'kittens') + + expect(database.human_adapter_name).to eq('Unknown') + end + end + end + + describe '#postgresql?' do + context 'when using PostgreSQL' do + it 'returns true' do + allow(database).to receive(:adapter_name).and_return('PostgreSQL') + + expect(database.postgresql?).to eq(true) + end + end + + context 'when not using PostgreSQL' do + it 'returns false' do + allow(database).to receive(:adapter_name).and_return('MySQL') + + expect(database.postgresql?).to eq(false) + end + end + end + + describe '#db_read_only?' do + it 'detects a read-only database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => "t" }]) + + expect(database.db_read_only?).to be_truthy + end + + it 'detects a read-only database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => true }]) + + expect(database.db_read_only?).to be_truthy + end + + it 'detects a read-write database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => "f" }]) + + expect(database.db_read_only?).to be_falsey + end + + it 'detects a read-write database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => false }]) + + expect(database.db_read_only?).to be_falsey + end + end + + describe '#db_read_write?' do + it 'detects a read-only database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => "t" }]) + + expect(database.db_read_write?).to eq(false) + end + + it 'detects a read-only database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => true }]) + + expect(database.db_read_write?).to eq(false) + end + + it 'detects a read-write database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => "f" }]) + + expect(database.db_read_write?).to eq(true) + end + + it 'detects a read-write database' do + allow(database.model.connection) + .to receive(:execute) + .with('SELECT pg_is_in_recovery()') + .and_return([{ "pg_is_in_recovery" => false }]) + + expect(database.db_read_write?).to eq(true) + end + end + + describe '#version' do + around do |example| + database.instance_variable_set(:@version, nil) + example.run + database.instance_variable_set(:@version, nil) + end + + context "on postgresql" do + it "extracts the version number" do + allow(database) + .to receive(:database_version) + .and_return("PostgreSQL 9.4.4 on x86_64-apple-darwin14.3.0") + + expect(database.version).to eq '9.4.4' + end + end + + it 'memoizes the result' do + count = ActiveRecord::QueryRecorder + .new { 2.times { database.version } } + .count + + expect(count).to eq(1) + end + end + + describe '#postgresql_minimum_supported_version?' do + it 'returns false when using PostgreSQL 10' do + allow(database).to receive(:version).and_return('10') + + expect(database.postgresql_minimum_supported_version?).to eq(false) + end + + it 'returns false when using PostgreSQL 11' do + allow(database).to receive(:version).and_return('11') + + expect(database.postgresql_minimum_supported_version?).to eq(false) + end + + it 'returns true when using PostgreSQL 12' do + allow(database).to receive(:version).and_return('12') + + expect(database.postgresql_minimum_supported_version?).to eq(true) + end + end + + describe '#cached_column_exists?' do + it 'only retrieves the data from the schema cache' do + database = described_class.new(Project) + queries = ActiveRecord::QueryRecorder.new do + 2.times do + expect(database.cached_column_exists?(:id)).to be_truthy + expect(database.cached_column_exists?(:bogus_column)).to be_falsey + end + end + + expect(queries.count).to eq(0) + end + end + + describe '#cached_table_exists?' do + it 'only retrieves the data from the schema cache' do + dummy = Class.new(ActiveRecord::Base) do + self.table_name = 'bogus_table_name' + end + + queries = ActiveRecord::QueryRecorder.new do + 2.times do + expect(described_class.new(Project).cached_table_exists?).to be_truthy + expect(described_class.new(dummy).cached_table_exists?).to be_falsey + end + end + + expect(queries.count).to eq(0) + end + + it 'returns false when database does not exist' do + database = described_class.new(Project) + + expect(database.model).to receive(:connection) do + raise ActiveRecord::NoDatabaseError, 'broken' + end + + expect(database.cached_table_exists?).to be(false) + end + end + + describe '#exists?' do + it 'returns true if the database exists' do + expect(database.exists?).to be(true) + end + + it "returns false if the database doesn't exist" do + expect(database.model.connection.schema_cache) + .to receive(:database_version) + .and_raise(ActiveRecord::NoDatabaseError) + + expect(database.exists?).to be(false) + end + end + + describe '#system_id' do + it 'returns the PostgreSQL system identifier' do + expect(database.system_id).to be_an_instance_of(Integer) + end + end + + describe '#config' do + it 'returns a HashWithIndifferentAccess' do + expect(database.config) + .to be_an_instance_of(HashWithIndifferentAccess) + end + + it 'returns a default pool size' do + expect(database.config) + .to include(pool: Gitlab::Database.default_pool_size) + end + + it 'does not cache its results' do + a = database.config + b = database.config + + expect(a).not_to equal(b) + end + end +end diff --git a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb index ee3f2b1b415..2ae9037959d 100644 --- a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb +++ b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb @@ -46,14 +46,14 @@ RSpec.describe Gitlab::Database::Reindexing::IndexSelection do expect(subject).not_to include(excluded.index) end - it 'excludes indexes larger than 100 GB ondisk size' do - excluded = create( + it 'includes indexes larger than 100 GB ondisk size' do + included = create( :postgres_index_bloat_estimate, index: create(:postgres_index, ondisk_size_bytes: 101.gigabytes), bloat_size_bytes: 25.gigabyte ) - expect(subject).not_to include(excluded.index) + expect(subject).to include(included.index) end context 'with time frozen' do diff --git a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb index a8f196d8f0e..1b409924acc 100644 --- a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb +++ b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb @@ -11,6 +11,8 @@ RSpec.describe Gitlab::Database::Reindexing::ReindexAction do swapout_view_for_table(:postgres_indexes) end + it { is_expected.to be_a Gitlab::Database::SharedModel } + describe '.create_for' do subject { described_class.create_for(index) } diff --git a/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb index 6f87475fc94..db267ff4f14 100644 --- a/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb +++ b/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb @@ -62,7 +62,7 @@ RSpec.describe Gitlab::Database::Reindexing::ReindexConcurrently, '#perform' do it 'recreates the index using REINDEX with a long statement timeout' do expect_to_execute_in_order( - "SET statement_timeout TO '32400s'", + "SET statement_timeout TO '86400s'", "REINDEX INDEX CONCURRENTLY \"public\".\"#{index.name}\"", "RESET statement_timeout" ) @@ -84,7 +84,7 @@ RSpec.describe Gitlab::Database::Reindexing::ReindexConcurrently, '#perform' do it 'drops the dangling indexes while controlling lock_timeout' do expect_to_execute_in_order( # Regular index rebuild - "SET statement_timeout TO '32400s'", + "SET statement_timeout TO '86400s'", "REINDEX INDEX CONCURRENTLY \"public\".\"#{index_name}\"", "RESET statement_timeout", # Drop _ccnew index diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index 550f9db2b5b..13aff343432 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -4,10 +4,63 @@ require 'spec_helper' RSpec.describe Gitlab::Database::Reindexing do include ExclusiveLeaseHelpers + include Database::DatabaseHelpers - describe '.perform' do - subject { described_class.perform(candidate_indexes) } + describe '.automatic_reindexing' do + subject { described_class.automatic_reindexing(maximum_records: limit) } + let(:limit) { 5 } + + before_all do + swapout_view_for_table(:postgres_indexes) + end + + before do + allow(Gitlab::Database::Reindexing).to receive(:cleanup_leftovers!) + allow(Gitlab::Database::Reindexing).to receive(:perform_from_queue).and_return(0) + allow(Gitlab::Database::Reindexing).to receive(:perform_with_heuristic).and_return(0) + end + + it 'cleans up leftovers, before consuming the queue' do + expect(Gitlab::Database::Reindexing).to receive(:cleanup_leftovers!).ordered + expect(Gitlab::Database::Reindexing).to receive(:perform_from_queue).ordered + + subject + end + + context 'with records in the queue' do + before do + create(:reindexing_queued_action) + end + + context 'with enough records in the queue to reach limit' do + let(:limit) { 1 } + + it 'does not perform reindexing with heuristic' do + expect(Gitlab::Database::Reindexing).to receive(:perform_from_queue).and_return(limit) + expect(Gitlab::Database::Reindexing).not_to receive(:perform_with_heuristic) + + subject + end + end + + context 'without enough records in the queue to reach limit' do + let(:limit) { 2 } + + it 'continues if the queue did not have enough records' do + expect(Gitlab::Database::Reindexing).to receive(:perform_from_queue).ordered.and_return(1) + expect(Gitlab::Database::Reindexing).to receive(:perform_with_heuristic).with(maximum_records: 1).ordered + + subject + end + end + end + end + + describe '.perform_with_heuristic' do + subject { described_class.perform_with_heuristic(candidate_indexes, maximum_records: limit) } + + let(:limit) { 2 } let(:coordinator) { instance_double(Gitlab::Database::Reindexing::Coordinator) } let(:index_selection) { instance_double(Gitlab::Database::Reindexing::IndexSelection) } let(:candidate_indexes) { double } @@ -15,7 +68,7 @@ RSpec.describe Gitlab::Database::Reindexing do it 'delegates to Coordinator' do expect(Gitlab::Database::Reindexing::IndexSelection).to receive(:new).with(candidate_indexes).and_return(index_selection) - expect(index_selection).to receive(:take).with(2).and_return(indexes) + expect(index_selection).to receive(:take).with(limit).and_return(indexes) indexes.each do |index| expect(Gitlab::Database::Reindexing::Coordinator).to receive(:new).with(index).and_return(coordinator) @@ -26,6 +79,59 @@ RSpec.describe Gitlab::Database::Reindexing do end end + describe '.perform_from_queue' do + subject { described_class.perform_from_queue(maximum_records: limit) } + + before_all do + swapout_view_for_table(:postgres_indexes) + end + + let(:limit) { 2 } + let(:queued_actions) { create_list(:reindexing_queued_action, 3) } + let(:coordinator) { instance_double(Gitlab::Database::Reindexing::Coordinator) } + + before do + queued_actions.take(limit).each do |action| + allow(Gitlab::Database::Reindexing::Coordinator).to receive(:new).with(action.index).and_return(coordinator) + allow(coordinator).to receive(:perform) + end + end + + it 'consumes the queue in order of created_at and applies the limit' do + queued_actions.take(limit).each do |action| + expect(Gitlab::Database::Reindexing::Coordinator).to receive(:new).ordered.with(action.index).and_return(coordinator) + expect(coordinator).to receive(:perform) + end + + subject + end + + it 'updates queued action and sets state to done' do + subject + + queue = queued_actions + + queue.shift(limit).each do |action| + expect(action.reload.state).to eq('done') + end + + queue.each do |action| + expect(action.reload.state).to eq('queued') + end + end + + it 'updates queued action upon error and sets state to failed' do + expect(Gitlab::Database::Reindexing::Coordinator).to receive(:new).ordered.with(queued_actions.first.index).and_return(coordinator) + expect(coordinator).to receive(:perform).and_raise('something went wrong') + + subject + + states = queued_actions.map(&:reload).map(&:state) + + expect(states).to eq(%w(failed done queued)) + end + end + describe '.cleanup_leftovers!' do subject { described_class.cleanup_leftovers! } diff --git a/spec/lib/gitlab/database/schema_cache_with_renamed_table_spec.rb b/spec/lib/gitlab/database/schema_cache_with_renamed_table_spec.rb index 8c0c4155ccc..7caee414719 100644 --- a/spec/lib/gitlab/database/schema_cache_with_renamed_table_spec.rb +++ b/spec/lib/gitlab/database/schema_cache_with_renamed_table_spec.rb @@ -11,12 +11,12 @@ RSpec.describe Gitlab::Database::SchemaCacheWithRenamedTable do let(:new_model) do Class.new(ActiveRecord::Base) do - self.table_name = 'projects_new' + self.table_name = '_test_projects_new' end end before do - stub_const('Gitlab::Database::TABLES_TO_BE_RENAMED', { 'projects' => 'projects_new' }) + stub_const('Gitlab::Database::TABLES_TO_BE_RENAMED', { 'projects' => '_test_projects_new' }) end context 'when table is not renamed yet' do @@ -32,8 +32,8 @@ RSpec.describe Gitlab::Database::SchemaCacheWithRenamedTable do context 'when table is renamed' do before do - ActiveRecord::Base.connection.execute("ALTER TABLE projects RENAME TO projects_new") - ActiveRecord::Base.connection.execute("CREATE VIEW projects AS SELECT * FROM projects_new") + ActiveRecord::Base.connection.execute("ALTER TABLE projects RENAME TO _test_projects_new") + ActiveRecord::Base.connection.execute("CREATE VIEW projects AS SELECT * FROM _test_projects_new") old_model.reset_column_information ActiveRecord::Base.connection.schema_cache.clear! @@ -54,14 +54,14 @@ RSpec.describe Gitlab::Database::SchemaCacheWithRenamedTable do it 'has the same indexes' do indexes_for_old_table = ActiveRecord::Base.connection.schema_cache.indexes('projects') - indexes_for_new_table = ActiveRecord::Base.connection.schema_cache.indexes('projects_new') + indexes_for_new_table = ActiveRecord::Base.connection.schema_cache.indexes('_test_projects_new') expect(indexes_for_old_table).to eq(indexes_for_new_table) end it 'has the same column_hash' do columns_hash_for_old_table = ActiveRecord::Base.connection.schema_cache.columns_hash('projects') - columns_hash_for_new_table = ActiveRecord::Base.connection.schema_cache.columns_hash('projects_new') + columns_hash_for_new_table = ActiveRecord::Base.connection.schema_cache.columns_hash('_test_projects_new') expect(columns_hash_for_old_table).to eq(columns_hash_for_new_table) end diff --git a/spec/lib/gitlab/database/schema_migrations/context_spec.rb b/spec/lib/gitlab/database/schema_migrations/context_spec.rb index 0323fa22b78..07c97ea0ec3 100644 --- a/spec/lib/gitlab/database/schema_migrations/context_spec.rb +++ b/spec/lib/gitlab/database/schema_migrations/context_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Gitlab::Database::SchemaMigrations::Context do end context 'CI database' do - let(:connection_class) { Ci::CiDatabaseRecord } + let(:connection_class) { Ci::ApplicationRecord } it 'returns a directory path that is database specific' do skip_if_multiple_databases_not_setup diff --git a/spec/lib/gitlab/database/shared_model_spec.rb b/spec/lib/gitlab/database/shared_model_spec.rb index 5d616aeb05f..94f2b5a3434 100644 --- a/spec/lib/gitlab/database/shared_model_spec.rb +++ b/spec/lib/gitlab/database/shared_model_spec.rb @@ -27,6 +27,38 @@ RSpec.describe Gitlab::Database::SharedModel do end end + context 'when multiple connection overrides are nested', :aggregate_failures do + let(:second_connection) { double('connection') } + + it 'allows the nesting with the same connection object' do + expect_original_connection_around do + described_class.using_connection(new_connection) do + expect(described_class.connection).to be(new_connection) + + described_class.using_connection(new_connection) do + expect(described_class.connection).to be(new_connection) + end + + expect(described_class.connection).to be(new_connection) + end + end + end + + it 'raises an error if the connection is changed' do + expect_original_connection_around do + described_class.using_connection(new_connection) do + expect(described_class.connection).to be(new_connection) + + expect do + described_class.using_connection(second_connection) {} + end.to raise_error(/cannot nest connection overrides/) + + expect(described_class.connection).to be(new_connection) + end + end + end + end + context 'when the block raises an error', :aggregate_failures do it 're-raises the error, removing the overridden connection' do expect_original_connection_around do diff --git a/spec/lib/gitlab/database/unidirectional_copy_trigger_spec.rb b/spec/lib/gitlab/database/unidirectional_copy_trigger_spec.rb index 2955c208f16..bbddb5f1af5 100644 --- a/spec/lib/gitlab/database/unidirectional_copy_trigger_spec.rb +++ b/spec/lib/gitlab/database/unidirectional_copy_trigger_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Gitlab::Database::UnidirectionalCopyTrigger do let(:table_name) { '_test_table' } let(:connection) { ActiveRecord::Base.connection } - let(:copy_trigger) { described_class.on_table(table_name) } + let(:copy_trigger) { described_class.on_table(table_name, connection: connection) } describe '#name' do context 'when a single column name is given' do |