diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 0d2ad188483..5ecf28e18f9 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -b84ba4f096da54ebb6a85c14ab736474c72f1a2a +d12fb69a841d91d843f392a124865f6d47d3bc22 diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 65a45373204..8039fac02ec 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -27,7 +27,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController feature_category :source_code_management, [:repository, :clear_repository_check_states] feature_category :continuous_integration, [:ci_cd, :reset_registration_token] - feature_category :usage_ping, [:usage_data] + feature_category :service_ping, [:usage_data] feature_category :integrations, [:integrations] feature_category :pages, [:lets_encrypt_terms_of_service] diff --git a/app/controllers/projects/service_ping_controller.rb b/app/controllers/projects/service_ping_controller.rb index e211ab3de0b..00530c09be8 100644 --- a/app/controllers/projects/service_ping_controller.rb +++ b/app/controllers/projects/service_ping_controller.rb @@ -3,7 +3,7 @@ class Projects::ServicePingController < Projects::ApplicationController before_action :authenticate_user! - feature_category :usage_ping + feature_category :service_ping def web_ide_clientside_preview return render_404 unless Gitlab::CurrentSettings.web_ide_clientside_preview_enabled? diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index d8ba530f3f6..c40feb42eea 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -425,15 +425,6 @@ module IssuablesHelper } end - def sidebar_status_data(issuable_sidebar, project) - { - iid: issuable_sidebar[:iid], - issuable_type: issuable_sidebar[:type], - full_path: project.full_path, - can_edit: issuable_sidebar.dig(:current_user, :can_edit).to_s - } - end - def parent @project || @group end diff --git a/app/models/integrations/jenkins.rb b/app/models/integrations/jenkins.rb index 815e86bcaa1..264bbd0b6ed 100644 --- a/app/models/integrations/jenkins.rb +++ b/app/models/integrations/jenkins.rb @@ -97,7 +97,6 @@ module Integrations { type: 'text', name: 'username', - required: true, help: s_('The username for the Jenkins server.') }, { diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 177ffd90b10..2395daf276d 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -81,7 +81,7 @@ #js-severity - if issuable_sidebar.dig(:features_available, :health_status) - .js-sidebar-status-entry-point{ data: sidebar_status_data(issuable_sidebar, @project) } + .js-sidebar-status-entry-point - if issuable_sidebar.has_key?(:confidential) %script#js-confidential-issue-data{ type: "application/json" }= { is_confidential: issuable_sidebar[:confidential], is_editable: can_edit_issuable }.to_json.html_safe diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index c2026d01df7..391548c5243 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -267,7 +267,7 @@ :tags: [] - :name: cronjob:gitlab_usage_ping :worker_name: GitlabUsagePingWorker - :feature_category: :usage_ping + :feature_category: :service_ping :has_external_dependencies: :urgency: :low :resource_boundary: :unknown diff --git a/app/workers/gitlab_usage_ping_worker.rb b/app/workers/gitlab_usage_ping_worker.rb index 5da644a9960..108e5b55c29 100644 --- a/app/workers/gitlab_usage_ping_worker.rb +++ b/app/workers/gitlab_usage_ping_worker.rb @@ -8,7 +8,7 @@ class GitlabUsagePingWorker # rubocop:disable Scalability/IdempotentWorker include CronjobQueue # rubocop:disable Scalability/CronWorkerContext include Gitlab::ExclusiveLeaseHelpers - feature_category :usage_ping + feature_category :service_ping sidekiq_options retry: 3, dead: false sidekiq_retry_in { |count| (count + 1) * 8.hours.to_i } diff --git a/config/feature_flags/development/database_reindexing_pg12.yml b/config/feature_flags/development/database_reindexing_pg12.yml new file mode 100644 index 00000000000..cf0545bb2f9 --- /dev/null +++ b/config/feature_flags/development/database_reindexing_pg12.yml @@ -0,0 +1,8 @@ +--- +name: database_reindexing_pg12 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64695 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/334372 +milestone: '14.1' +type: development +group: group::database +default_enabled: false diff --git a/doc/development/internal_api.md b/doc/development/internal_api.md index 982d6c0854d..4614db96263 100644 --- a/doc/development/internal_api.md +++ b/doc/development/internal_api.md @@ -529,7 +529,7 @@ POST /namespaces/:id/gitlab_subscription Example request: ```shell -curl --request POST --header "TOKEN: " "https://gitlab.com/api/v4/namespaces/1234/gitlab_subscription?start_date="2020-07-15"&plan="silver"&seats=10" +curl --request POST --header "TOKEN: " "https://gitlab.com/api/v4/namespaces/1234/gitlab_subscription?start_date="2020-07-15"&plan="premium"&seats=10" ``` Example response: @@ -537,8 +537,8 @@ Example response: ```json { "plan": { - "code":"silver", - "name":"Silver", + "code":"premium", + "name":"premium", "trial":false, "auto_renew":null, "upgradable":false @@ -588,8 +588,8 @@ Example response: ```json { "plan": { - "code":"silver", - "name":"Silver", + "code":"premium", + "name":"premium", "trial":false, "auto_renew":null, "upgradable":false @@ -627,8 +627,8 @@ Example response: ```json { "plan": { - "code":"silver", - "name":"Silver", + "code":"premium", + "name":"premium", "trial":false, "auto_renew":null, "upgradable":false diff --git a/doc/user/clusters/agent/ci_cd_tunnel.md b/doc/user/clusters/agent/ci_cd_tunnel.md index 29023acdd65..3cafdabf909 100644 --- a/doc/user/clusters/agent/ci_cd_tunnel.md +++ b/doc/user/clusters/agent/ci_cd_tunnel.md @@ -30,30 +30,29 @@ To create the Tunnel: .kubectl_config: &kubectl_config - | - cat << EOF > "$HOME/agent_config.yaml" + cat << EOF > "$CI_PROJECT_DIR/.kubeconfig.agent.yaml" apiVersion: v1 kind: Config clusters: - cluster: - server: https://kas.gitlab.com/k8s-proxy + server: https://kas.gitlab.com/k8s-proxy name: agent users: - name: agent user: - token: "ci:$AGENT_ID:$CI_JOB_TOKEN" + token: "ci:$AGENT_ID:$CI_JOB_TOKEN" contexts: - context: cluster: agent - user: agent - name: agent + user: agent + name: agent current-context: agent EOF - - export KUBECONFIG="$KUBECONFIG:$HOME/agent_config.yaml" deploy: script: - *kubectl_config - - kubectl get pods + - kubectl --kubeconfig="$CI_PROJECT_DIR/.kubeconfig.agent.yaml" get pods ``` 1. Execute `kubectl` commands directly against your cluster with this CI/CD job you just created. diff --git a/doc/user/project/repository/branches/default.md b/doc/user/project/repository/branches/default.md index 6c2469ac377..ebc9d9aefde 100644 --- a/doc/user/project/repository/branches/default.md +++ b/doc/user/project/repository/branches/default.md @@ -57,9 +57,7 @@ GitLab administrators can configure a new default branch name at the ### Instance-level custom initial branch name **(FREE SELF)** > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/221013) in GitLab 13.2. -> - It's deployed behind a feature flag, enabled by default. -> - It cannot be enabled or disabled per-project. -> - It's recommended for production use. +> - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/325163) in GitLab 13.12. GitLab [administrators](../../../permissions.md) of self-managed instances can customize the initial branch for projects hosted on that instance. Individual diff --git a/doc/user/project/repository/repository_mirroring.md b/doc/user/project/repository/repository_mirroring.md index d4f563085a4..369e054aed1 100644 --- a/doc/user/project/repository/repository_mirroring.md +++ b/doc/user/project/repository/repository_mirroring.md @@ -320,8 +320,9 @@ For more information, see [Start the pull mirroring process for a Project](../.. > - Moved to GitLab Premium in 13.9. Based on the mirror direction that you choose, you can opt to mirror only the -[protected branches](../protected_branches.md) from/to your remote repository. -For pull mirroring, non-protected branches are not mirrored and can diverge. +[protected branches](../protected_branches.md) in the mirroring project, +either from or to your remote repository. For pull mirroring, non-protected branches in +the mirroring project are not mirrored and can diverge. To use this option, check the **Only mirror protected branches** box when creating a repository mirror. **(PREMIUM)** diff --git a/lib/api/usage_data.rb b/lib/api/usage_data.rb index 7deec15dcac..43c75206b88 100644 --- a/lib/api/usage_data.rb +++ b/lib/api/usage_data.rb @@ -4,7 +4,7 @@ module API class UsageData < ::API::Base before { authenticate_non_get! } - feature_category :usage_ping + feature_category :service_ping namespace 'usage_data' do before do diff --git a/lib/api/usage_data_non_sql_metrics.rb b/lib/api/usage_data_non_sql_metrics.rb index 63a14a223f5..d9e0d153e58 100644 --- a/lib/api/usage_data_non_sql_metrics.rb +++ b/lib/api/usage_data_non_sql_metrics.rb @@ -4,7 +4,7 @@ module API class UsageDataNonSqlMetrics < ::API::Base before { authenticated_as_admin! } - feature_category :usage_ping + feature_category :service_ping namespace 'usage_data' do before do diff --git a/lib/api/usage_data_queries.rb b/lib/api/usage_data_queries.rb index 0ad9ad7650c..22e83fe0294 100644 --- a/lib/api/usage_data_queries.rb +++ b/lib/api/usage_data_queries.rb @@ -4,7 +4,7 @@ module API class UsageDataQueries < ::API::Base before { authenticated_as_admin! } - feature_category :usage_ping + feature_category :service_ping namespace 'usage_data' do before do diff --git a/lib/gitlab/database/postgres_index.rb b/lib/gitlab/database/postgres_index.rb index 6e734834841..4c6a04293a6 100644 --- a/lib/gitlab/database/postgres_index.rb +++ b/lib/gitlab/database/postgres_index.rb @@ -20,16 +20,25 @@ module Gitlab # A 'regular' index is a non-unique index, # that does not serve an exclusion constraint and # is defined on a table that is not partitioned. - scope :regular, -> { where(unique: false, partitioned: false, exclusion: false)} + # + # Deprecated: Switch to scope .reindexing_support + scope :regular, -> { where(unique: false, partitioned: false, exclusion: false, expression: false)} + + # Indexes for reindexing with PG12 + scope :reindexing_support, -> { where(partitioned: false, exclusion: false, expression: false) } scope :not_match, ->(regex) { where("name !~ ?", regex)} + scope :match, ->(regex) { where("name ~* ?", regex)} + scope :not_recently_reindexed, -> do recent_actions = Reindexing::ReindexAction.recent.where('index_identifier = identifier') where('NOT EXISTS (?)', recent_actions) end + alias_method :reset, :reload + def bloat_size strong_memoize(:bloat_size) { bloat_estimate&.bloat_size || 0 } end diff --git a/lib/gitlab/database/reindexing.rb b/lib/gitlab/database/reindexing.rb index 0cfad690283..1e9cb68879d 100644 --- a/lib/gitlab/database/reindexing.rb +++ b/lib/gitlab/database/reindexing.rb @@ -14,11 +14,16 @@ module Gitlab end def self.candidate_indexes - Gitlab::Database::PostgresIndex - .regular - .where('NOT expression') + indexes = Gitlab::Database::PostgresIndex .not_match("^#{ConcurrentReindex::TEMPORARY_INDEX_PREFIX}") .not_match("^#{ConcurrentReindex::REPLACED_INDEX_PREFIX}") + .not_match("#{ReindexConcurrently::TEMPORARY_INDEX_PATTERN}$") + + if Feature.enabled?(:database_reindexing_pg12, type: :development) + indexes.reindexing_support + else + indexes.regular + end end end end diff --git a/lib/gitlab/database/reindexing/coordinator.rb b/lib/gitlab/database/reindexing/coordinator.rb index d68f47b5b6c..e8a92c44836 100644 --- a/lib/gitlab/database/reindexing/coordinator.rb +++ b/lib/gitlab/database/reindexing/coordinator.rb @@ -41,7 +41,13 @@ module Gitlab end def perform_for(index, action) - ConcurrentReindex.new(index).perform + strategy = if Feature.enabled?(:database_reindexing_pg12, type: :development) + ReindexConcurrently + else + ConcurrentReindex + end + + strategy.new(index).perform rescue StandardError action.state = :failed diff --git a/lib/gitlab/database/reindexing/reindex_concurrently.rb b/lib/gitlab/database/reindexing/reindex_concurrently.rb new file mode 100644 index 00000000000..506db1b6f17 --- /dev/null +++ b/lib/gitlab/database/reindexing/reindex_concurrently.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +module Gitlab + module Database + module Reindexing + # This is a >= PG12 reindexing strategy based on `REINDEX CONCURRENTLY` + class ReindexConcurrently + ReindexError = Class.new(StandardError) + + TEMPORARY_INDEX_PATTERN = '\_ccnew[0-9]*' + STATEMENT_TIMEOUT = 9.hours + PG_MAX_INDEX_NAME_LENGTH = 63 + + # When dropping an index, we acquire a SHARE UPDATE EXCLUSIVE lock, + # which only conflicts with DDL and vacuum. We therefore execute this with a rather + # high lock timeout and a long pause in between retries. This is an alternative to + # setting a high statement timeout, which would lead to a long running query with effects + # on e.g. vacuum. + REMOVE_INDEX_RETRY_CONFIG = [[1.minute, 9.minutes]] * 30 + + attr_reader :index, :logger + + def initialize(index, logger: Gitlab::AppLogger) + @index = index + @logger = logger + end + + def perform + raise ReindexError, 'indexes serving an exclusion constraint are currently not supported' if index.exclusion? + raise ReindexError, 'index is a left-over temporary index from a previous reindexing run' if index.name =~ /#{TEMPORARY_INDEX_PATTERN}/ + + # Expression indexes require additional statistics in `pg_statistic`: + # select * from pg_statistic where starelid = (select oid from pg_class where relname = 'some_index'); + # + # In PG12, this has been fixed in https://gitlab.com/postgres/postgres/-/commit/b17ff07aa3eb142d2cde2ea00e4a4e8f63686f96. + # Discussion happened in https://www.postgresql.org/message-id/flat/CAFcNs%2BqpFPmiHd1oTXvcPdvAHicJDA9qBUSujgAhUMJyUMb%2BSA%40mail.gmail.com + # following a GitLab.com incident that surfaced this (https://gitlab.com/gitlab-com/gl-infra/production/-/issues/2885). + # + # While this has been backpatched, we continue to disable expression indexes until further review. + raise ReindexError, 'expression indexes are currently not supported' if index.expression? + + begin + with_logging do + set_statement_timeout do + execute("REINDEX INDEX CONCURRENTLY #{quote_table_name(index.schema)}.#{quote_table_name(index.name)}") + end + end + ensure + cleanup_dangling_indexes + end + end + + private + + def with_logging + bloat_size = index.bloat_size + ondisk_size_before = index.ondisk_size_bytes + + logger.info( + message: "Starting reindex of #{index}", + index: index.identifier, + table: index.tablename, + estimated_bloat_bytes: bloat_size, + index_size_before_bytes: ondisk_size_before + ) + + duration = Benchmark.realtime do + yield + end + + index.reset + + logger.info( + message: "Finished reindex of #{index}", + index: index.identifier, + table: index.tablename, + estimated_bloat_bytes: bloat_size, + index_size_before_bytes: ondisk_size_before, + index_size_after_bytes: index.ondisk_size_bytes, + duration_s: duration.round(2) + ) + end + + def cleanup_dangling_indexes + Gitlab::Database::PostgresIndex.match("#{TEMPORARY_INDEX_PATTERN}$").each do |lingering_index| + # Example lingering index name: some_index_ccnew1 + + # Example prefix: 'some_index' + prefix = lingering_index.name.gsub(/#{TEMPORARY_INDEX_PATTERN}/, '') + + # Example suffix: '_ccnew1' + suffix = lingering_index.name.match(/#{TEMPORARY_INDEX_PATTERN}/)[0] + + # Only remove if the lingering index name could have been chosen + # as a result of a REINDEX operation (considering that PostgreSQL + # truncates index names to 63 chars and adds a suffix). + if index.name[0...PG_MAX_INDEX_NAME_LENGTH - suffix.length] == prefix + remove_index(lingering_index) + end + end + end + + def remove_index(index) + logger.info("Removing dangling index #{index.identifier}") + + retries = Gitlab::Database::WithLockRetriesOutsideTransaction.new( + timing_configuration: REMOVE_INDEX_RETRY_CONFIG, + klass: self.class, + logger: logger + ) + + retries.run(raise_on_exhaustion: false) do + execute("DROP INDEX CONCURRENTLY IF EXISTS #{quote_table_name(index.schema)}.#{quote_table_name(index.name)}") + end + end + + def with_lock_retries(&block) + arguments = { klass: self.class, logger: logger } + Gitlab::Database::WithLockRetries.new(**arguments).run(raise_on_exhaustion: true, &block) + end + + def set_statement_timeout + execute("SET statement_timeout TO '%ds'" % STATEMENT_TIMEOUT) + yield + ensure + execute('RESET statement_timeout') + end + + delegate :execute, :quote_table_name, to: :connection + def connection + @connection ||= ActiveRecord::Base.connection + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index cd59ac3cf24..71a62f55aba 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12832,9 +12832,6 @@ msgstr "" msgid "Error occurred when saving reviewers" msgstr "" -msgid "Error occurred while updating the %{issuableType} status" -msgstr "" - msgid "Error occurred while updating the issue status" msgstr "" @@ -30474,9 +30471,6 @@ msgstr "" msgid "Something went wrong while setting %{issuableType} confidentiality." msgstr "" -msgid "Something went wrong while setting %{issuableType} health status." -msgstr "" - msgid "Something went wrong while setting %{issuableType} notifications." msgstr "" diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index f3514360a1f..c6f26809385 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -264,6 +264,18 @@ RSpec.describe 'Database schema' do end end + context 'index names' do + it 'disallows index names with a _ccnew[0-9]* suffix' do + # During REINDEX operations, Postgres generates a temporary index with a _ccnew[0-9]* suffix + # Since indexes are being considered temporary and subject to removal if they stick around for longer. See Gitlab::Database::Reindexing. + # + # Hence we disallow adding permanent indexes with this suffix. + problematic_indexes = Gitlab::Database::PostgresIndex.match("#{Gitlab::Database::Reindexing::ReindexConcurrently::TEMPORARY_INDEX_PATTERN}$").all + + expect(problematic_indexes).to be_empty + end + end + private def retrieve_columns_name_with_jsonb diff --git a/spec/lib/gitlab/database/postgres_index_spec.rb b/spec/lib/gitlab/database/postgres_index_spec.rb index 2fda9b85c5a..7ac3cd6197f 100644 --- a/spec/lib/gitlab/database/postgres_index_spec.rb +++ b/spec/lib/gitlab/database/postgres_index_spec.rb @@ -34,6 +34,24 @@ RSpec.describe Gitlab::Database::PostgresIndex do it 'only indexes that dont serve an exclusion constraint' do expect(described_class.regular).to all(have_attributes(exclusion: false)) end + + it 'only non-expression indexes' do + expect(described_class.regular).to all(have_attributes(expression: false)) + end + end + + describe '.reindexing_support' do + it 'only non partitioned indexes' do + expect(described_class.reindexing_support).to all(have_attributes(partitioned: false)) + end + + it 'only indexes that dont serve an exclusion constraint' do + expect(described_class.reindexing_support).to all(have_attributes(exclusion: false)) + end + + it 'only non-expression indexes' do + expect(described_class.reindexing_support).to all(have_attributes(expression: false)) + end end describe '.not_match' do diff --git a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb index ae6362ba812..fd8b177c809 100644 --- a/spec/lib/gitlab/database/reindexing/coordinator_spec.rb +++ b/spec/lib/gitlab/database/reindexing/coordinator_spec.rb @@ -9,13 +9,6 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do describe '.perform' do subject { described_class.new(index, notifier).perform } - before do - swapout_view_for_table(:postgres_indexes) - - allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer) - allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) - end - let(:index) { create(:postgres_index) } let(:notifier) { instance_double(Gitlab::Database::Reindexing::GrafanaNotifier, notify_start: nil, notify_end: nil) } let(:reindexer) { instance_double(Gitlab::Database::Reindexing::ConcurrentReindex, perform: nil) } @@ -26,57 +19,83 @@ RSpec.describe Gitlab::Database::Reindexing::Coordinator do let(:lease_timeout) { 1.day } let(:uuid) { 'uuid' } - context 'locking' do - it 'acquires a lock while reindexing' do - expect(lease).to receive(:try_obtain).ordered.and_return(uuid) + shared_examples_for 'reindexing coordination' do + context 'locking' do + it 'acquires a lock while reindexing' do + expect(lease).to receive(:try_obtain).ordered.and_return(uuid) - expect(reindexer).to receive(:perform).ordered + expect(reindexer).to receive(:perform).ordered - expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) + expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid) - subject + subject + end + + it 'does not perform reindexing actions if lease is not granted' do + expect(lease).to receive(:try_obtain).ordered.and_return(false) + expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new) + + subject + end end - it 'does not perform reindexing actions if lease is not granted' do - expect(lease).to receive(:try_obtain).ordered.and_return(false) - expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new) + context 'notifications' do + it 'sends #notify_start before reindexing' do + expect(notifier).to receive(:notify_start).with(action).ordered + expect(reindexer).to receive(:perform).ordered - subject + subject + end + + it 'sends #notify_end after reindexing and updating the action is done' do + expect(action).to receive(:finish).ordered + expect(notifier).to receive(:notify_end).with(action).ordered + + subject + end + end + + context 'action tracking' do + it 'calls #finish on the action' do + expect(reindexer).to receive(:perform).ordered + expect(action).to receive(:finish).ordered + + subject + end + + it 'upon error, it still calls finish and raises the error' do + expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong') + expect(action).to receive(:finish).ordered + + expect { subject }.to raise_error(/something went wrong/) + + expect(action).to be_failed + end end end - context 'notifications' do - it 'sends #notify_start before reindexing' do - expect(notifier).to receive(:notify_start).with(action).ordered - expect(reindexer).to receive(:perform).ordered + context 'legacy reindexing method (< PG12) - to be removed' do + before do + stub_feature_flags(database_reindexing_pg12: false) + swapout_view_for_table(:postgres_indexes) - subject + allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer) + allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) end - it 'sends #notify_end after reindexing and updating the action is done' do - expect(action).to receive(:finish).ordered - expect(notifier).to receive(:notify_end).with(action).ordered - - subject - end + it_behaves_like 'reindexing coordination' end - context 'action tracking' do - it 'calls #finish on the action' do - expect(reindexer).to receive(:perform).ordered - expect(action).to receive(:finish).ordered + context 'PG12 reindexing method' do + before do + stub_feature_flags(database_reindexing_pg12: true) + swapout_view_for_table(:postgres_indexes) - subject + allow(Gitlab::Database::Reindexing::ReindexConcurrently).to receive(:new).with(index).and_return(reindexer) + allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:create_for).with(index).and_return(action) end - it 'upon error, it still calls finish and raises the error' do - expect(reindexer).to receive(:perform).ordered.and_raise('something went wrong') - expect(action).to receive(:finish).ordered - - expect { subject }.to raise_error(/something went wrong/) - - expect(action).to be_failed - end + it_behaves_like 'reindexing coordination' end end end diff --git a/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb b/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb new file mode 100644 index 00000000000..6f87475fc94 --- /dev/null +++ b/spec/lib/gitlab/database/reindexing/reindex_concurrently_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Reindexing::ReindexConcurrently, '#perform' do + subject { described_class.new(index, logger: logger).perform } + + let(:table_name) { '_test_reindex_table' } + let(:column_name) { '_test_column' } + let(:index_name) { '_test_reindex_index' } + let(:index) { Gitlab::Database::PostgresIndex.by_identifier("public.#{iname(index_name)}") } + let(:logger) { double('logger', debug: nil, info: nil, error: nil ) } + let(:connection) { ActiveRecord::Base.connection } + + before do + connection.execute(<<~SQL) + CREATE TABLE #{table_name} ( + id serial NOT NULL PRIMARY KEY, + #{column_name} integer NOT NULL); + + CREATE INDEX #{index_name} ON #{table_name} (#{column_name}); + SQL + end + + context 'when the index serves an exclusion constraint' do + before do + allow(index).to receive(:exclusion?).and_return(true) + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /indexes serving an exclusion constraint are currently not supported/) + end + end + + context 'when attempting to reindex an expression index' do + before do + allow(index).to receive(:expression?).and_return(true) + end + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /expression indexes are currently not supported/) + end + end + + context 'when the index is a dangling temporary index from a previous reindexing run' do + context 'with the temporary index prefix' do + let(:index_name) { '_test_reindex_index_ccnew' } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /left-over temporary index/) + end + end + + context 'with the temporary index prefix with a counter' do + let(:index_name) { '_test_reindex_index_ccnew1' } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::ReindexError, /left-over temporary index/) + end + end + end + + it 'recreates the index using REINDEX with a long statement timeout' do + expect_to_execute_in_order( + "SET statement_timeout TO '32400s'", + "REINDEX INDEX CONCURRENTLY \"public\".\"#{index.name}\"", + "RESET statement_timeout" + ) + + subject + end + + context 'with dangling indexes matching TEMPORARY_INDEX_PATTERN, i.e. /some\_index\_ccnew(\d)*/' do + before do + # dangling indexes + connection.execute("CREATE INDEX #{iname(index_name, '_ccnew')} ON #{table_name} (#{column_name})") + connection.execute("CREATE INDEX #{iname(index_name, '_ccnew2')} ON #{table_name} (#{column_name})") + + # Unrelated index - don't drop + connection.execute("CREATE INDEX some_other_index_ccnew ON #{table_name} (#{column_name})") + end + + shared_examples_for 'dropping the dangling index' do + it 'drops the dangling indexes while controlling lock_timeout' do + expect_to_execute_in_order( + # Regular index rebuild + "SET statement_timeout TO '32400s'", + "REINDEX INDEX CONCURRENTLY \"public\".\"#{index_name}\"", + "RESET statement_timeout", + # Drop _ccnew index + "SET lock_timeout TO '60000ms'", + "DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{iname(index_name, '_ccnew')}\"", + "RESET idle_in_transaction_session_timeout; RESET lock_timeout", + # Drop _ccnew2 index + "SET lock_timeout TO '60000ms'", + "DROP INDEX CONCURRENTLY IF EXISTS \"public\".\"#{iname(index_name, '_ccnew2')}\"", + "RESET idle_in_transaction_session_timeout; RESET lock_timeout" + ) + + subject + end + end + + context 'with normal index names' do + it_behaves_like 'dropping the dangling index' + end + + context 'with index name at 63 character limit' do + let(:index_name) { 'a' * 63 } + + before do + # Another unrelated index - don't drop + extra_index = index_name[0...55] + connection.execute("CREATE INDEX #{extra_index}_ccnew ON #{table_name} (#{column_name})") + end + + it_behaves_like 'dropping the dangling index' + end + end + + def iname(name, suffix = '') + "#{name[0...63 - suffix.size]}#{suffix}" + end + + def expect_to_execute_in_order(*queries) + # Indexes cannot be created CONCURRENTLY in a transaction. Since the tests are wrapped in transactions, + # verify the original call but pass through the non-concurrent form. + queries.each do |query| + expect(connection).to receive(:execute).with(query).ordered.and_wrap_original do |method, sql| + method.call(sql.sub(/CONCURRENTLY/, '')) + end + end + end +end diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index b2f038e8b62..0a3cbcef46a 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -29,11 +29,30 @@ RSpec.describe Gitlab::Database::Reindexing do describe '.candidate_indexes' do subject { described_class.candidate_indexes } - it 'retrieves regular indexes that are no left-overs from previous runs' do - result = double - expect(Gitlab::Database::PostgresIndex).to receive_message_chain('regular.where.not_match.not_match').with(no_args).with('NOT expression').with('^tmp_reindex_').with('^old_reindex_').and_return(result) + context 'with deprecated method for < PG12' do + before do + stub_feature_flags(database_reindexing_pg12: false) + end - expect(subject).to eq(result) + it 'retrieves regular indexes that are no left-overs from previous runs' do + result = double + expect(Gitlab::Database::PostgresIndex).to receive_message_chain('not_match.not_match.not_match.regular').with('^tmp_reindex_').with('^old_reindex_').with('\_ccnew[0-9]*$').with(no_args).and_return(result) + + expect(subject).to eq(result) + end + end + + context 'with deprecated method for >= PG12' do + before do + stub_feature_flags(database_reindexing_pg12: true) + end + + it 'retrieves regular indexes that are no left-overs from previous runs' do + result = double + expect(Gitlab::Database::PostgresIndex).to receive_message_chain('not_match.not_match.not_match.reindexing_support').with('^tmp_reindex_').with('^old_reindex_').with('\_ccnew[0-9]*$').with(no_args).and_return(result) + + expect(subject).to eq(result) + end end end end