diff --git a/.rubocop.yml b/.rubocop.yml index b2c1d3ac947fc275b7c58775cb2fe790827d3fcf..895afd67c315b3fc8c32369ca2d3765f578b3a49 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -409,6 +409,7 @@ Database/MultipleDatabases: - 'ee/lib/ee/gitlab/background_migration/**/*.rb' - 'spec/lib/gitlab/background_migration/**/*.rb' - 'spec/lib/gitlab/database/**/*.rb' + - 'spec/tasks/gitlab/db_rake_spec.rb' Migration/BatchMigrationsPostOnly: Enabled: true diff --git a/.rubocop_todo/database/multiple_databases.yml b/.rubocop_todo/database/multiple_databases.yml index 43da6f8a5b4d67e69e6665478c63c5c0cf131780..be91da8aad4fcf0a3b84917600d3609e154e6650 100644 --- a/.rubocop_todo/database/multiple_databases.yml +++ b/.rubocop_todo/database/multiple_databases.yml @@ -5,10 +5,3 @@ Database/MultipleDatabases: - 'db/post_migrate/20210811122206_update_external_project_bots.rb' - 'db/post_migrate/20210812013042_remove_duplicate_project_authorizations.rb' - 'ee/spec/services/ee/merge_requests/update_service_spec.rb' - - 'spec/support/caching.rb' - - 'spec/support/helpers/database/database_helpers.rb' - - 'spec/support/helpers/database/table_schema_helpers.rb' - - 'spec/support/helpers/migrations_helpers.rb' - - 'spec/support/helpers/query_recorder.rb' - - 'spec/support/helpers/usage_data_helpers.rb' - - 'spec/tasks/gitlab/db_rake_spec.rb' diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml index 07e7b374db2af18e080067aa6226bb8512cb016b..0910e63f819ff4e1f4ae72127b991927e1a92c82 100644 --- a/.rubocop_todo/layout/line_length.yml +++ b/.rubocop_todo/layout/line_length.yml @@ -3156,7 +3156,6 @@ Layout/LineLength: - 'lib/gitlab/database/reflection.rb' - 'lib/gitlab/database/reindexing.rb' - 'lib/gitlab/database/reindexing/coordinator.rb' - - 'lib/gitlab/database/reindexing/grafana_notifier.rb' - 'lib/gitlab/database/reindexing/reindex_concurrently.rb' - 'lib/gitlab/database/schema_migrations/context.rb' - 'lib/gitlab/database/similarity_score.rb' @@ -4458,7 +4457,6 @@ Layout/LineLength: - 'spec/lib/gitlab/database/query_analyzer_spec.rb' - 'spec/lib/gitlab/database/query_analyzers/prevent_cross_database_modification_spec.rb' - 'spec/lib/gitlab/database/query_analyzers/restrict_allowed_schemas_spec.rb' - - 'spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb' - 'spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb' - 'spec/lib/gitlab/database/reindexing_spec.rb' - 'spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_base_spec.rb' diff --git a/lib/gitlab/database/reindexing/grafana_notifier.rb b/lib/gitlab/database/reindexing/grafana_notifier.rb index ece9327b658059b9efe8d65f67390954660ed24f..e43eddbefc08f51dcd898dafc1b1f64a1cc6881d 100644 --- a/lib/gitlab/database/reindexing/grafana_notifier.rb +++ b/lib/gitlab/database/reindexing/grafana_notifier.rb @@ -60,7 +60,9 @@ def annotate(payload) "Authorization": "Bearer #{@api_key}" } - success = Gitlab::HTTP.post("#{@api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true).success? + success = Gitlab::HTTP.post( + "#{@api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true + ).success? log_error("Response code #{response.code}") unless success diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb b/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb index db4383a79d48fed1b8b20e9a8ac25a7e2d003a1b..1c0f5a0c42028da9d862d6d8fc36f7d71ecedcae 100644 --- a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb +++ b/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable do +RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::AutovacuumActiveOnTable, + feature_category: :database do include Database::DatabaseHelpers let(:connection) { Gitlab::Database.database_base_models[:main].connection } @@ -17,7 +18,7 @@ subject { described_class.new(context).evaluate } before do - swapout_view_for_table(:postgres_autovacuum_activity) + swapout_view_for_table(:postgres_autovacuum_activity, connection: connection) end let(:tables) { [table] } diff --git a/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb b/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb index c1ac8f0c9cdd3961271089c2b58a3e7ec8942cfb..f24c4559349359646bea0a65f094890e4a4e2162 100644 --- a/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb +++ b/spec/lib/gitlab/database/postgres_autovacuum_activity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model do +RSpec.describe Gitlab::Database::PostgresAutovacuumActivity, type: :model, feature_category: :database do include Database::DatabaseHelpers it { is_expected.to be_a Gitlab::Database::SharedModel } @@ -13,7 +13,7 @@ let(:tables) { %w[foo test] } before do - swapout_view_for_table(:postgres_autovacuum_activity) + swapout_view_for_table(:postgres_autovacuum_activity, connection: ApplicationRecord.connection) # unrelated create(:postgres_autovacuum_activity, table: 'bar') diff --git a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb index bb91617714a4fc7b48468aca58f289a9f786ef7c..d6a9edc76eb7241ec68cf4615c271e2eaabb57cc 100644 --- a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb +++ b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb @@ -2,13 +2,15 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::Coordinator do +RSpec.describe Gitlab::Database::Reindexing::Coordinator, feature_category: :database do include Database::DatabaseHelpers include ExclusiveLeaseHelpers - let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) } let(:index) { create(:postgres_index) } let(:connection) { index.connection } + let(:notifier) do + instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) + end let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) } let(:lease_key) { "gitlab/database/reindexing/coordinator/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } @@ -19,14 +21,11 @@ model = Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME] Gitlab::Database::SharedModel.using_connection(model.connection) do + swapout_view_for_table(:postgres_indexes, connection: model.connection) example.run end end - before do - swapout_view_for_table(:postgres_indexes) - end - describe '#perform' do subject { described_class.new(index, notifier).perform } diff --git a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb index 1bccdda3be1c3fa68949a020cdd06843c340a484..e67c97cbf9c749db099680c028a49eb7640b4183 100644 --- a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb +++ b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb @@ -12,7 +12,7 @@ let(:action) { create(:reindex_action) } before do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end let(:headers) do @@ -25,7 +25,9 @@ let(:response) { double('response', success?: true) } def expect_api_call(payload) - expect(Gitlab::HTTP).to receive(:post).with("#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true).and_return(response) + expect(Gitlab::HTTP).to receive(:post).with( + "#{api_url}/api/annotations", body: payload.to_json, headers: headers, allow_local_requests: true + ).and_return(response) end shared_examples_for 'interacting with Grafana annotations API' do @@ -109,7 +111,9 @@ def expect_api_call(payload) end context 'additional tag is provided' do - subject { described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_start(action) } + subject do + described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_start(action) + end let(:payload) do { @@ -163,7 +167,9 @@ def expect_api_call(payload) end context 'additional tag is provided' do - subject { described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_end(action) } + subject do + described_class.new(api_key: api_key, api_url: api_url, additional_tag: additional_tag).notify_end(action) + end let(:payload) do { diff --git a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb index 9f31716ab9485ebd709acd8723602ad1ff018578..bb745970e3848838ac936c280f294bd523b50533 100644 --- a/spec/lib/gitlab/database/reindexing/index_selection_spec.rb +++ b/spec/lib/gitlab/database/reindexing/index_selection_spec.rb @@ -2,14 +2,16 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::IndexSelection do +RSpec.describe Gitlab::Database::Reindexing::IndexSelection, feature_category: :database do include Database::DatabaseHelpers subject { described_class.new(Gitlab::Database::PostgresIndex.all).to_a } + let(:connection) { ApplicationRecord.connection } + before do - swapout_view_for_table(:postgres_index_bloat_estimates) - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_index_bloat_estimates, connection: connection) + swapout_view_for_table(:postgres_indexes, connection: connection) create_list(:postgres_index, 10, ondisk_size_bytes: 10.gigabytes).each_with_index do |index, i| create(:postgres_index_bloat_estimate, index: index, bloat_size_bytes: 2.gigabyte * (i + 1)) @@ -17,7 +19,7 @@ end def execute(sql) - ActiveRecord::Base.connection.execute(sql) + connection.execute(sql) end it 'orders by highest relative bloat first' do diff --git a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb index 1b409924acc2409b513cc0f57a54f19554e23d0a..06b89e087371714d364a3d732b8bbc7cbb40021f 100644 --- a/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb +++ b/spec/lib/gitlab/database/reindexing/reindex_action_spec.rb @@ -2,13 +2,13 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Reindexing::ReindexAction do +RSpec.describe Gitlab::Database::Reindexing::ReindexAction, feature_category: :database do include Database::DatabaseHelpers let(:index) { create(:postgres_index) } before_all do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end it { is_expected.to be_a Gitlab::Database::SharedModel } diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index fa26aa593299befc3e657923367afd6f002e6ee1..05e7420ab34435053d37742b7f4ef605c6f90d4f 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -76,7 +76,7 @@ let(:limit) { 5 } before_all do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end before do @@ -147,7 +147,7 @@ subject { described_class.perform_from_queue(maximum_records: limit) } before_all do - swapout_view_for_table(:postgres_indexes) + swapout_view_for_table(:postgres_indexes, connection: ApplicationRecord.connection) end let(:limit) { 2 } diff --git a/spec/lib/gitlab/utils/usage_data_spec.rb b/spec/lib/gitlab/utils/usage_data_spec.rb index 13d046b0816c84e6e0462c3c05d367527bb9f897..5be1787fe7b4da3ca920b494ebdacdff03c8398c 100644 --- a/spec/lib/gitlab/utils/usage_data_spec.rb +++ b/spec/lib/gitlab/utils/usage_data_spec.rb @@ -374,7 +374,7 @@ def expect_error(exception, message, &block) context 'when query timeout' do subject do - with_statement_timeout(0.001) do + with_statement_timeout(0.001, connection: ApplicationRecord.connection) do relation = AlertManagement::HttpIntegration.select('pg_sleep(0.002)') described_class.histogram(relation, column, buckets: 1..100) end diff --git a/spec/models/factories_spec.rb b/spec/models/factories_spec.rb index 4915c0bd87013be4bdeaf0fb56c5a9b35e7d056f..981ca3dfa2ce46a2604f86a8646e27947b3d24f1 100644 --- a/spec/models/factories_spec.rb +++ b/spec/models/factories_spec.rb @@ -188,7 +188,13 @@ before do factories_based_on_view.each do |factory| view = build(factory).class.table_name - swapout_view_for_table(view) + view_gitlab_schema = Gitlab::Database::GitlabSchema.table_schema(view) + Gitlab::Database.database_base_models.each_value.select do |base_model| + connection = base_model.connection + next unless Gitlab::Database.gitlab_schemas_for_connection(connection).include?(view_gitlab_schema) + + swapout_view_for_table(view, connection: connection) + end end end diff --git a/spec/support/caching.rb b/spec/support/caching.rb index 11e4f5349718edc048a0c62fb6eee6786e1216e1..b18223523db555f2b26d648d660782ee230f24b5 100644 --- a/spec/support/caching.rb +++ b/spec/support/caching.rb @@ -37,8 +37,8 @@ end config.around(:each, :use_sql_query_cache) do |example| - ActiveRecord::Base.cache do - example.run - end + base_models = Gitlab::Database.database_base_models_with_gitlab_shared.values + inner_proc = proc { example.run } + base_models.inject(inner_proc) { |proc, base_model| proc { base_model.cache { proc.call } } }.call end end diff --git a/spec/support/helpers/database/database_helpers.rb b/spec/support/helpers/database/database_helpers.rb index f3b2a2a614737a485918104bef71fc3881248167..ecc42041e93a4873fb5b3473707fe03c4b3edc63 100644 --- a/spec/support/helpers/database/database_helpers.rb +++ b/spec/support/helpers/database/database_helpers.rb @@ -4,9 +4,7 @@ module Database module DatabaseHelpers # In order to directly work with views using factories, # we can swapout the view for a table of identical structure. - def swapout_view_for_table(view, connection: nil) - connection ||= ActiveRecord::Base.connection - + def swapout_view_for_table(view, connection:) connection.execute(<<~SQL.squish) CREATE TABLE #{view}_copy (LIKE #{view}); DROP VIEW #{view}; @@ -28,21 +26,20 @@ def swapout_view_for_table(view, connection: nil) # with_statement_timeout(0.1) do # model.select('pg_sleep(0.11)') # end - def with_statement_timeout(timeout) + def with_statement_timeout(timeout, connection:) # Force a positive value and a minimum of 1ms for very small values. timeout = (timeout * 1000).abs.ceil raise ArgumentError, 'Using a timeout of `0` means to disable statement timeout.' if timeout == 0 - previous_timeout = ActiveRecord::Base.connection - .exec_query('SHOW statement_timeout')[0].fetch('statement_timeout') + previous_timeout = connection.select_value('SHOW statement_timeout') - set_statement_timeout("#{timeout}ms") + connection.execute(format(%(SET LOCAL statement_timeout = '%s'), timeout)) yield ensure begin - set_statement_timeout(previous_timeout) + connection.execute(format(%(SET LOCAL statement_timeout = '%s'), previous_timeout)) rescue ActiveRecord::StatementInvalid # After a transaction was canceled/aborted due to e.g. a statement # timeout commands are ignored and will raise in PG::InFailedSqlTransaction. @@ -50,22 +47,5 @@ def with_statement_timeout(timeout) # for the currrent transaction which will be closed anyway. end end - - # Set statement timeout for the current transaction. - # - # Note, that it does not restore the previous statement timeout. - # Use `with_statement_timeout` instead. - # - # @param timeout - Statement timeout in seconds - # - # Example: - # - # set_statement_timeout(0.1) - # model.select('pg_sleep(0.11)') - def set_statement_timeout(timeout) - ActiveRecord::Base.connection.execute( - format(%(SET LOCAL statement_timeout = '%s'), timeout) - ) - end end end diff --git a/spec/support/helpers/database/table_schema_helpers.rb b/spec/support/helpers/database/table_schema_helpers.rb index 472eaa45b4b8bd414f590185b52ac9b4c3b20266..815c37e00e53b89c11ac93c3c1eec87b0e094228 100644 --- a/spec/support/helpers/database/table_schema_helpers.rb +++ b/spec/support/helpers/database/table_schema_helpers.rb @@ -3,7 +3,9 @@ module Database module TableSchemaHelpers def connection - ActiveRecord::Base.connection + # We use ActiveRecord::Base.connection here because this is mainly used for database migrations + # where we override the connection on ActiveRecord::Base.connection + ActiveRecord::Base.connection # rubocop:disable Database/MultipleDatabases end def expect_table_to_be_replaced(original_table:, replacement_table:, archived_table:) diff --git a/spec/support/helpers/migrations_helpers.rb b/spec/support/helpers/migrations_helpers.rb index e1d28a807e36974b82c6e122722fb57df180ea07..6fc5904fc83ed120f29f9f0a94c60e10a04a481a 100644 --- a/spec/support/helpers/migrations_helpers.rb +++ b/spec/support/helpers/migrations_helpers.rb @@ -104,7 +104,7 @@ def use_fake_application_settings # We stub this way because we can't stub on # `current_application_settings` due to `method_missing` is # depending on current_application_settings... - allow(ActiveRecord::Base.connection) + allow(Gitlab::Database::Migration::V1_0::MigrationRecord.connection) .to receive(:active?) .and_return(false) allow(Gitlab::Runtime) @@ -158,10 +158,10 @@ def disable_migrations_output end def migrate! - open_transactions = ActiveRecord::Base.connection.open_transactions + open_transactions = Gitlab::Database::Migration::V1_0::MigrationRecord.connection.open_transactions allow_next_instance_of(described_class) do |migration| allow(migration).to receive(:transaction_open?) do - ActiveRecord::Base.connection.open_transactions > open_transactions + Gitlab::Database::Migration::V1_0::MigrationRecord.connection.open_transactions > open_transactions end end diff --git a/spec/support/helpers/query_recorder.rb b/spec/support/helpers/query_recorder.rb index dd124ed9c7fd0a97f1419977e39c7340ca221b38..5be9ba9ae1e5b22a8dde01bcfa133cb7389e3baf 100644 --- a/spec/support/helpers/query_recorder.rb +++ b/spec/support/helpers/query_recorder.rb @@ -19,9 +19,7 @@ def initialize(skip_cached: true, skip_schema_queries: true, log_file: nil, quer def record(&block) # force replacement of bind parameters to give tests the ability to check for ids - ActiveRecord::Base.connection.unprepared_statement do - ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block) - end + ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block) end def show_backtrace(values, duration) diff --git a/spec/support/helpers/usage_data_helpers.rb b/spec/support/helpers/usage_data_helpers.rb index 78ceaf297a8beeb4830ef369bf0c4c354f5e5458..438f0d129b9b7370d6b656e4697024a9c6de9c7b 100644 --- a/spec/support/helpers/usage_data_helpers.rb +++ b/spec/support/helpers/usage_data_helpers.rb @@ -116,8 +116,9 @@ module UsageDataHelpers ).freeze def stub_usage_data_connections - allow(ActiveRecord::Base.connection).to receive(:transaction_open?).and_return(false) - allow(::Ci::ApplicationRecord.connection).to receive(:transaction_open?).and_return(false) if ::Ci::ApplicationRecord.connection_class? + Gitlab::Database.database_base_models_with_gitlab_shared.each_value do |base_model| + allow(base_model.connection).to receive(:transaction_open?).and_return(false) + end allow(Gitlab::Prometheus::Internal).to receive(:prometheus_enabled?).and_return(false) end diff --git a/spec/tasks/gitlab/db_rake_spec.rb b/spec/tasks/gitlab/db_rake_spec.rb index 22abfc33d1bb228925b58b62bb07dbefc3cf250c..7671c65d22cbdd677b07b6bd57b2db656da46a26 100644 --- a/spec/tasks/gitlab/db_rake_spec.rb +++ b/spec/tasks/gitlab/db_rake_spec.rb @@ -22,7 +22,7 @@ describe 'mark_migration_complete' do context 'with a single database' do - let(:main_model) { ActiveRecord::Base } + let(:main_model) { ApplicationRecord } before do skip_if_multiple_databases_are_setup