diff --git a/db/database_connections/ci.yaml b/db/database_connections/ci.yaml index daa155dcb00283df3a66c52838e80bbf28763731..5331765214e10449a0888e3630a589ae8f96ece7 100644 --- a/db/database_connections/ci.yaml +++ b/db/database_connections/ci.yaml @@ -4,6 +4,11 @@ gitlab_schemas: - gitlab_internal - gitlab_shared - gitlab_ci +lock_gitlab_schemas: + - gitlab_main + - gitlab_main_clusterwide + - gitlab_main_cell + - gitlab_pm klass: Ci::ApplicationRecord # if CI database is not configured, use this database fallback_database: main diff --git a/db/database_connections/main.yaml b/db/database_connections/main.yaml index dfdd50eb08595e4743f583c84bc3a24dfd74f19c..9eadd26ec26f67ff5ada5d23cd8bc66a1a335f21 100644 --- a/db/database_connections/main.yaml +++ b/db/database_connections/main.yaml @@ -6,6 +6,8 @@ gitlab_schemas: - gitlab_main - gitlab_main_cell - gitlab_pm +lock_gitlab_schemas: + - gitlab_ci # Note that we use ActiveRecord::Base here and not ApplicationRecord. # This is deliberate, as: # - the load balancer must be enabled for _all_ models diff --git a/lib/gitlab/database/database_connection_info.rb b/lib/gitlab/database/database_connection_info.rb index 57ecbcd64aea81c9eb5df2b099d9eb710520822f..f0cafcf041b41ea6528b2654268b1d6454d207f4 100644 --- a/lib/gitlab/database/database_connection_info.rb +++ b/lib/gitlab/database/database_connection_info.rb @@ -6,6 +6,7 @@ module Database :name, :description, :gitlab_schemas, + :lock_gitlab_schemas, :klass, :fallback_database, :db_dir, @@ -20,6 +21,7 @@ def initialize(*) self.name = name.to_sym self.gitlab_schemas = gitlab_schemas.map(&:to_sym) self.klass = klass.constantize + self.lock_gitlab_schemas = (lock_gitlab_schemas || []).map(&:to_sym) self.fallback_database = fallback_database&.to_sym self.db_dir = Rails.root.join(db_dir || 'db') end diff --git a/lib/gitlab/database/gitlab_schema.rb b/lib/gitlab/database/gitlab_schema.rb index 9b58284b389066235d752b33ad8e3f105c4b8e51..0bd357b7730476b4df70a4e68e056a25396d68af 100644 --- a/lib/gitlab/database/gitlab_schema.rb +++ b/lib/gitlab/database/gitlab_schema.rb @@ -23,6 +23,21 @@ def self.table_schemas!(tables) tables.map { |table| table_schema!(table) }.to_set end + # Mainly used for test tables + # It maps table names prefixes to gitlab_schemas. + # The order of keys matter. Prefixes that contain other prefixes should come first. + IMPLICIT_GITLAB_SCHEMAS = { + '_test_gitlab_main_clusterwide_' => :gitlab_main_clusterwide, + '_test_gitlab_main_cell_' => :gitlab_main_cell, + '_test_gitlab_main_' => :gitlab_main, + '_test_gitlab_ci_' => :gitlab_ci, + '_test_gitlab_embedding_' => :gitlab_embedding, + '_test_gitlab_geo_' => :gitlab_geo, + '_test_gitlab_pm_' => :gitlab_pm, + '_test_' => :gitlab_shared, + 'pg_' => :gitlab_internal + }.freeze + # rubocop:disable Metrics/CyclomaticComplexity def self.table_schema(name) schema_name, table_name = name.split('.', 2) # Strip schema name like: `public.` @@ -54,19 +69,11 @@ def self.table_schema(name) # All tables from `information_schema.` are marked as `internal` return :gitlab_internal if schema_name == 'information_schema' - return :gitlab_main if table_name.start_with?('_test_gitlab_main_') - - return :gitlab_ci if table_name.start_with?('_test_gitlab_ci_') - - return :gitlab_embedding if table_name.start_with?('_test_gitlab_embedding_') - - return :gitlab_geo if table_name.start_with?('_test_gitlab_geo_') - - # All tables that start with `_test_` without a following schema are shared and ignored - return :gitlab_shared if table_name.start_with?('_test_') + IMPLICIT_GITLAB_SCHEMAS.each do |prefix, gitlab_schema| + return gitlab_schema if table_name.start_with?(prefix) + end - # All `pg_` tables are marked as `internal` - return :gitlab_internal if table_name.start_with?('pg_') + nil end # rubocop:enable Metrics/CyclomaticComplexity diff --git a/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables.rb b/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables.rb index 55c4fd6a7afb313219718758f21b1f14950f6cf0..fe456fab50586237ecf8bb86ab49f31294611715 100644 --- a/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables.rb +++ b/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables.rb @@ -11,7 +11,9 @@ module AutomaticLockWritesOnTables end def exec_migration(connection, direction) - return super if %w[main ci].exclude?(Gitlab::Database.db_config_name(connection)) + db_config_name = Gitlab::Database.db_config_name(connection) + db_info = Gitlab::Database.all_database_connections.fetch(db_config_name) + return super if db_info.lock_gitlab_schemas.empty? return super if automatic_lock_on_writes_disabled? # This compares the tables only on the `public` schema. Partitions are not affected @@ -20,7 +22,7 @@ def exec_migration(connection, direction) new_tables = connection.tables - tables new_tables.each do |table_name| - lock_writes_on_table(connection, table_name) if should_lock_writes_on_table?(table_name) + lock_writes_on_table(connection, table_name) if should_lock_writes_on_table?(db_info, table_name) end end @@ -39,16 +41,17 @@ def automatic_lock_on_writes_disabled? end end - def should_lock_writes_on_table?(table_name) - # currently gitlab_schema represents only present existing tables, this is workaround for deleted tables - # that should be skipped as they will be removed in a future migration. + def should_lock_writes_on_table?(db_info, table_name) + # We skip locking writes on tables that are scheduled for deletion in a future migration return false if Gitlab::Database::GitlabSchema.deleted_tables_to_schema[table_name] table_schema = Gitlab::Database::GitlabSchema.table_schema!(table_name.to_s) - return false unless %i[gitlab_main gitlab_ci].include?(table_schema) - - Gitlab::Database.gitlab_schemas_for_connection(connection).exclude?(table_schema) + # This takes into consideration which database mode is used. + # In single-db and single-db-ci-connection the main database includes gitlab_ci tables, + # so we don't lock them there. + Gitlab::Database.gitlab_schemas_for_connection(connection).exclude?(table_schema) && + db_info.lock_gitlab_schemas.include?(table_schema) end # with_retries creates new a transaction. So we set it to false if the connection is diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb index 48f5cdb995b3122c0ef6bc6389c6e4fbb21998d0..1c864239ae68f9c63a762b1c683f81e4e312dd82 100644 --- a/spec/lib/gitlab/database/gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -29,6 +29,9 @@ 'audit_events_part_5fc467ac26' | :gitlab_main '_test_gitlab_main_table' | :gitlab_main '_test_gitlab_ci_table' | :gitlab_ci + '_test_gitlab_main_clusterwide_table' | :gitlab_main_clusterwide + '_test_gitlab_main_cell_table' | :gitlab_main_cell + '_test_gitlab_pm_table' | :gitlab_pm '_test_my_table' | :gitlab_shared 'pg_attribute' | :gitlab_internal end diff --git a/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb b/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb index e4241348b5431c35219d5557693b0eef85aed27b..577bf00ba2f6ca9377f3ac1a8791f73741787df2 100644 --- a/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/automatic_lock_writes_on_tables_spec.rb @@ -9,7 +9,10 @@ let(:schema_class) { Class.new(Gitlab::Database::Migration[2.1]) } let(:skip_automatic_lock_on_writes) { false } let(:gitlab_main_table_name) { :_test_gitlab_main_table } + let(:gitlab_main_clusterwide_table_name) { :_test_gitlab_main_clusterwide_table } + let(:gitlab_main_cell_table_name) { :_test_gitlab_main_cell_table } let(:gitlab_ci_table_name) { :_test_gitlab_ci_table } + let(:gitlab_pm_table_name) { :_test_gitlab_pm_table } let(:gitlab_geo_table_name) { :_test_gitlab_geo_table } let(:gitlab_shared_table_name) { :_test_table } @@ -24,8 +27,11 @@ # Drop the created test tables, because we use non-transactional tests after do drop_table_if_exists(gitlab_main_table_name) + drop_table_if_exists(gitlab_main_clusterwide_table_name) + drop_table_if_exists(gitlab_main_cell_table_name) drop_table_if_exists(gitlab_ci_table_name) drop_table_if_exists(gitlab_geo_table_name) + drop_table_if_exists(gitlab_pm_table_name) drop_table_if_exists(gitlab_shared_table_name) drop_table_if_exists(renamed_gitlab_main_table_name) drop_table_if_exists(renamed_gitlab_ci_table_name) @@ -82,8 +88,12 @@ context 'when single database' do let(:config_model) { Gitlab::Database.database_base_models[:main] } let(:create_gitlab_main_table_migration_class) { create_table_migration(gitlab_main_table_name) } + let(:create_gitlab_main_cell_table_migration_class) { create_table_migration(gitlab_main_cell_table_name) } let(:create_gitlab_ci_table_migration_class) { create_table_migration(gitlab_ci_table_name) } let(:create_gitlab_shared_table_migration_class) { create_table_migration(gitlab_shared_table_name) } + let(:create_gitlab_main_clusterwide_table_migration_class) do + create_table_migration(gitlab_main_clusterwide_table_name) + end before do skip_if_database_exists(:ci) @@ -93,13 +103,19 @@ expect(Gitlab::Database::LockWritesManager).not_to receive(:new) create_gitlab_main_table_migration_class.migrate(:up) + create_gitlab_main_cell_table_migration_class.migrate(:up) + create_gitlab_main_clusterwide_table_migration_class.migrate(:up) create_gitlab_ci_table_migration_class.migrate(:up) create_gitlab_shared_table_migration_class.migrate(:up) expect do create_gitlab_main_table_migration_class.connection.execute("DELETE FROM #{gitlab_main_table_name}") + create_gitlab_main_cell_table_migration_class.connection.execute("DELETE FROM #{gitlab_main_cell_table_name}") create_gitlab_ci_table_migration_class.connection.execute("DELETE FROM #{gitlab_ci_table_name}") create_gitlab_shared_table_migration_class.connection.execute("DELETE FROM #{gitlab_shared_table_name}") + create_gitlab_main_clusterwide_table_migration_class.connection.execute( + "DELETE FROM #{gitlab_main_clusterwide_table_name}" + ) end.not_to raise_error end end @@ -163,6 +179,27 @@ end end + context 'for creating a gitlab_main_clusterwide table' do + let(:table_name) { gitlab_main_clusterwide_table_name } + + it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:main] + it_behaves_like 'locks writes on table', Gitlab::Database.database_base_models[:ci] + end + + context 'for creating a gitlab_main_cell table' do + let(:table_name) { gitlab_main_cell_table_name } + + it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:main] + it_behaves_like 'locks writes on table', Gitlab::Database.database_base_models[:ci] + end + + context 'for creating a gitlab_pm table' do + let(:table_name) { gitlab_pm_table_name } + + it_behaves_like 'does not lock writes on table', Gitlab::Database.database_base_models[:main] + it_behaves_like 'locks writes on table', Gitlab::Database.database_base_models[:ci] + end + context 'for creating a gitlab_ci table' do let(:table_name) { gitlab_ci_table_name }