From 56c4b780f9e201a89a19c4f7e5811633e0a51142 Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Tue, 21 Oct 2025 15:07:25 -0400 Subject: [PATCH 1/8] Update to handle error routing for child tables --- .../batch_cleaner_service.rb | 56 ++++++++++++++----- .../loose_foreign_keys/cleanup_worker.rb | 1 - 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/app/services/loose_foreign_keys/batch_cleaner_service.rb b/app/services/loose_foreign_keys/batch_cleaner_service.rb index 833b78430c2d4a..154a1f765b9e1d 100644 --- a/app/services/loose_foreign_keys/batch_cleaner_service.rb +++ b/app/services/loose_foreign_keys/batch_cleaner_service.rb @@ -34,36 +34,64 @@ def initialize( end def execute + # Capture feature category from the child table to ensure consistent error routing + # for both cleanup operations and final record processing + feature_category = nil + loose_foreign_key_definitions.each do |loose_foreign_key_definition| - run_cleaner_service(loose_foreign_key_definition, with_skip_locked: true) + feature_category ||= find_child_table_feature_category(loose_foreign_key_definition.from_table) - if modification_tracker.over_limit? - handle_over_limit - break - end + with_captured_feature_category_context(feature_category) do + run_cleaner_service(loose_foreign_key_definition, with_skip_locked: true) - run_cleaner_service(loose_foreign_key_definition, with_skip_locked: false) + if modification_tracker.over_limit? + handle_over_limit + break + end - if modification_tracker.over_limit? - handle_over_limit - break + run_cleaner_service(loose_foreign_key_definition, with_skip_locked: false) + + if modification_tracker.over_limit? + handle_over_limit + break + end end end return if modification_tracker.over_limit? - # At this point, all associations are cleaned up, we can update the status of the parent records - update_count = Gitlab::Database::SharedModel.using_connection(connection) do - LooseForeignKeys::DeletedRecord.mark_records_processed(deleted_parent_records) - end + with_captured_feature_category_context(feature_category) do + # At this point, all associations are cleaned up, we can update the status of the parent records + update_count = Gitlab::Database::SharedModel.using_connection(connection) do + LooseForeignKeys::DeletedRecord.mark_records_processed(deleted_parent_records) + end - deleted_records_counter.increment({ table: parent_table, db_config_name: db_config_name }, update_count) + deleted_records_counter.increment({ table: parent_table, db_config_name: db_config_name }, update_count) + end end private attr_reader :parent_table, :loose_foreign_key_definitions, :deleted_parent_records, :modification_tracker, :deleted_records_counter, :deleted_records_rescheduled_count, :deleted_records_incremented_count, :connection, :logger + def find_child_table_feature_category(table) + Gitlab::Database::Dictionary.entries.find_by_table_name(table)&.feature_categories&.first || :database + end + + def with_captured_feature_category_context(feature_category) + if feature_category_additions_enabled? + Gitlab::ApplicationContext.with_context(feature_category: feature_category) do + yield + end + else + yield + end + end + + def feature_category_additions_enabled? + Feature.enabled?(:loose_foreign_key_worker_improvements, Feature.current_pod) + end + def handle_over_limit records_to_reschedule = [] records_to_increment = [] diff --git a/app/workers/loose_foreign_keys/cleanup_worker.rb b/app/workers/loose_foreign_keys/cleanup_worker.rb index f5b7016bb790ef..e88a11d9ebe6f8 100644 --- a/app/workers/loose_foreign_keys/cleanup_worker.rb +++ b/app/workers/loose_foreign_keys/cleanup_worker.rb @@ -7,7 +7,6 @@ class CleanupWorker include CronjobQueue # rubocop: disable Scalability/CronWorkerContext sidekiq_options retry: false - feature_category :database data_consistency :always idempotent! -- GitLab From 50b58836374d25722281d4da9426cd2a37176c17 Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Tue, 21 Oct 2025 15:30:15 -0400 Subject: [PATCH 2/8] Update the queues yml --- app/workers/all_queues.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 88cb09d9fa6c3c..0ab03d28b2224e 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -839,7 +839,7 @@ :queue_namespace: :cronjob - :name: cronjob:loose_foreign_keys_cleanup :worker_name: LooseForeignKeys::CleanupWorker - :feature_category: :database + :feature_category: :has_external_dependencies: false :urgency: :low :resource_boundary: :unknown -- GitLab From fe6a6e7c277592d0d74ab765e96c077778bb07d1 Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Tue, 21 Oct 2025 15:47:33 -0400 Subject: [PATCH 3/8] Add Feature flag info --- .../loose_foreign_key_worker_improvements.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml diff --git a/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml b/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml new file mode 100644 index 00000000000000..2930a083faaa75 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml @@ -0,0 +1,10 @@ +--- +name: loose_foreign_key_worker_improvements +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/573367 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209695 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/578006 +milestone: '18.6' +group: group::database frameworks +type: gitlab_com_derisk +default_enabled: false \ No newline at end of file -- GitLab From 2e7a98898daa90de97c7444e5c7f70a279531087 Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Tue, 21 Oct 2025 15:50:38 -0400 Subject: [PATCH 4/8] Add newline at the end of FF yml --- .../gitlab_com_derisk/loose_foreign_key_worker_improvements.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml b/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml index 2930a083faaa75..20df92ff33c29b 100644 --- a/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml +++ b/config/feature_flags/gitlab_com_derisk/loose_foreign_key_worker_improvements.yml @@ -7,4 +7,4 @@ rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/578006 milestone: '18.6' group: group::database frameworks type: gitlab_com_derisk -default_enabled: false \ No newline at end of file +default_enabled: false -- GitLab From 4eacfe8be938bffcd768fbc41d75f5bbfbad0a14 Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Wed, 22 Oct 2025 13:53:31 -0400 Subject: [PATCH 5/8] Update LFK logic and tests --- .../batch_cleaner_service.rb | 90 +++++++++++-------- .../loose_foreign_keys/cleanup_worker.rb | 1 + .../batch_cleaner_service_spec.rb | 66 +++++++++++++- 3 files changed, 121 insertions(+), 36 deletions(-) diff --git a/app/services/loose_foreign_keys/batch_cleaner_service.rb b/app/services/loose_foreign_keys/batch_cleaner_service.rb index 154a1f765b9e1d..2af187c5acebe5 100644 --- a/app/services/loose_foreign_keys/batch_cleaner_service.rb +++ b/app/services/loose_foreign_keys/batch_cleaner_service.rb @@ -34,58 +34,78 @@ def initialize( end def execute - # Capture feature category from the child table to ensure consistent error routing - # for both cleanup operations and final record processing - feature_category = nil + if feature_category_additions_enabled? + new_cleaner_service + else + original_cleaner_service + end - loose_foreign_key_definitions.each do |loose_foreign_key_definition| - feature_category ||= find_child_table_feature_category(loose_foreign_key_definition.from_table) + return if modification_tracker.over_limit? - with_captured_feature_category_context(feature_category) do - run_cleaner_service(loose_foreign_key_definition, with_skip_locked: true) + # At this point, all associations are cleaned up, we can update the status of the parent records + update_count = Gitlab::Database::SharedModel.using_connection(connection) do + LooseForeignKeys::DeletedRecord.mark_records_processed(deleted_parent_records) + end - if modification_tracker.over_limit? - handle_over_limit - break - end + deleted_records_counter.increment({ table: parent_table, db_config_name: db_config_name }, update_count) + end - run_cleaner_service(loose_foreign_key_definition, with_skip_locked: false) + private - if modification_tracker.over_limit? - handle_over_limit - break - end - end - end + attr_reader :parent_table, :loose_foreign_key_definitions, :deleted_parent_records, :modification_tracker, :deleted_records_counter, :deleted_records_rescheduled_count, :deleted_records_incremented_count, :connection, :logger - return if modification_tracker.over_limit? + def original_cleaner_service + loose_foreign_key_definitions.each do |loose_foreign_key_definition| + run_cleaner_service(loose_foreign_key_definition, with_skip_locked: true) - with_captured_feature_category_context(feature_category) do - # At this point, all associations are cleaned up, we can update the status of the parent records - update_count = Gitlab::Database::SharedModel.using_connection(connection) do - LooseForeignKeys::DeletedRecord.mark_records_processed(deleted_parent_records) + if modification_tracker.over_limit? + handle_over_limit + break end - deleted_records_counter.increment({ table: parent_table, db_config_name: db_config_name }, update_count) + run_cleaner_service(loose_foreign_key_definition, with_skip_locked: false) + + if modification_tracker.over_limit? + handle_over_limit + break + end end end - private + def new_cleaner_service + loose_foreign_key_definitions.each do |loose_foreign_key_definition| + break if process_definition_with_feature_category(loose_foreign_key_definition) + end + end - attr_reader :parent_table, :loose_foreign_key_definitions, :deleted_parent_records, :modification_tracker, :deleted_records_counter, :deleted_records_rescheduled_count, :deleted_records_incremented_count, :connection, :logger + def process_definition_with_feature_category(loose_foreign_key_definition) + feature_category = find_child_table_feature_category(loose_foreign_key_definition.from_table) - def find_child_table_feature_category(table) - Gitlab::Database::Dictionary.entries.find_by_table_name(table)&.feature_categories&.first || :database + Gitlab::ApplicationContext.with_context(feature_category: feature_category) do + process_cleaner_services(loose_foreign_key_definition) + end end - def with_captured_feature_category_context(feature_category) - if feature_category_additions_enabled? - Gitlab::ApplicationContext.with_context(feature_category: feature_category) do - yield - end - else - yield + def process_cleaner_services(loose_foreign_key_definition) + run_cleaner_service(loose_foreign_key_definition, with_skip_locked: true) + + if modification_tracker.over_limit? + handle_over_limit + return true end + + run_cleaner_service(loose_foreign_key_definition, with_skip_locked: false) + + if modification_tracker.over_limit? + handle_over_limit + return true + end + + false + end + + def find_child_table_feature_category(table) + Gitlab::Database::Dictionary.entries.find_by_table_name(table)&.feature_categories&.first || :database end def feature_category_additions_enabled? diff --git a/app/workers/loose_foreign_keys/cleanup_worker.rb b/app/workers/loose_foreign_keys/cleanup_worker.rb index e88a11d9ebe6f8..f5b7016bb790ef 100644 --- a/app/workers/loose_foreign_keys/cleanup_worker.rb +++ b/app/workers/loose_foreign_keys/cleanup_worker.rb @@ -7,6 +7,7 @@ class CleanupWorker include CronjobQueue # rubocop: disable Scalability/CronWorkerContext sidekiq_options retry: false + feature_category :database data_consistency :always idempotent! diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb index fa3afb65f8aa8c..b0ebbd5e6e2db9 100644 --- a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -1,4 +1,4 @@ -# frozen_string_literal: true +# frozen_string_literal: true require 'spec_helper' @@ -365,4 +365,68 @@ def create_table_structure expect(loose_fk_child_table_2.where(parent_id_with_different_column: parent_record_1.id).count).to eq(2) end end + + describe 'feature flag: loose_foreign_key_worker_improvements' do + before do + parent_record_1.delete + end + + context 'when feature flag is enabled' do + before do + stub_feature_flags(loose_foreign_key_worker_improvements: true) + end + + it 'uses new cleaner service with feature category context' do + expect(Gitlab::ApplicationContext).to receive(:with_context).exactly(4).times.and_call_original + + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute + end + + it 'still cleans up records correctly' do + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute + + expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty + expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(loose_foreign_key_worker_improvements: false) + end + + it 'uses original cleaner service without feature category context' do + expect(Gitlab::ApplicationContext).not_to receive(:with_context) + + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute + end + + it 'still cleans up records correctly' do + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute + + expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty + expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) + end + end + end end -- GitLab From 3a712fa1e20875515ba32a30b6b47f828f2573ac Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Wed, 22 Oct 2025 13:55:29 -0400 Subject: [PATCH 6/8] Add this feature category back into the worker --- app/workers/all_queues.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 0ab03d28b2224e..88cb09d9fa6c3c 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -839,7 +839,7 @@ :queue_namespace: :cronjob - :name: cronjob:loose_foreign_keys_cleanup :worker_name: LooseForeignKeys::CleanupWorker - :feature_category: + :feature_category: :database :has_external_dependencies: false :urgency: :low :resource_boundary: :unknown -- GitLab From 728b85130c53a73db3e6e819cda095e1807a640e Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Wed, 22 Oct 2025 13:56:40 -0400 Subject: [PATCH 7/8] Fix spacing in spec --- spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb index b0ebbd5e6e2db9..1009c7ae2ad25b 100644 --- a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -1,4 +1,4 @@ -# frozen_string_literal: true +# frozen_string_literal: true require 'spec_helper' -- GitLab From f13c0586d155cd52796139f9498a48ff7fa925d1 Mon Sep 17 00:00:00 2001 From: Irina Bronipolsky Date: Wed, 22 Oct 2025 14:12:46 -0400 Subject: [PATCH 8/8] Update test specs --- .../batch_cleaner_service_spec.rb | 436 +++++++++--------- 1 file changed, 208 insertions(+), 228 deletions(-) diff --git a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb index 1009c7ae2ad25b..0708cd8a4fc6db 100644 --- a/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb +++ b/spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb @@ -137,106 +137,16 @@ def create_table_structure migration.drop_table :_test_loose_fk_child_table_4 end - context 'when parent records are deleted' do - let(:deleted_records_counter) { Gitlab::Metrics.client.get(:loose_foreign_key_processed_deleted_records) } - - before do - parent_record_1.delete - - expect(loose_fk_child_table_1.count).to eq(4) - expect(loose_fk_child_table_2.count).to eq(4) - expect(loose_fk_child_table_4.count).to eq(4) - - described_class.new( - parent_table: '_test_loose_fk_parent_table', - loose_foreign_key_definitions: loose_foreign_key_definitions, - deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), - connection: ::ApplicationRecord.connection - ).execute - end - - it 'cleans up the child records' do - expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty - expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) - expect(loose_fk_child_table_4.where(parent_id: parent_record_1.id, association_type: 'association_type_x')).to be_empty - end - - it 'updates the child records' do - expect(loose_fk_child_table_3.where(parent_id: parent_record_1.id, status: 4).count).to eq(2) - end - - it 'cleans up the pending parent DeletedRecord' do - expect(LooseForeignKeys::DeletedRecord.status_pending.count).to eq(0) - expect(LooseForeignKeys::DeletedRecord.status_processed.count).to eq(1) - end - - it 'records the DeletedRecord status updates', :prometheus do - counter = Gitlab::Metrics.client.get(:loose_foreign_key_processed_deleted_records) - - expect(counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) - end - - it 'does not delete unrelated records' do - expect(loose_fk_child_table_1.where(parent_id: other_parent_record.id).count).to eq(2) - expect(loose_fk_child_table_2.where(parent_id_with_different_column: other_parent_record.id).count).to eq(2) - expect(loose_fk_child_table_4.where(parent_id: parent_record_1.id, association_type: 'association_type_y').count).to eq(2) - end - - it 'does not update unrelated records' do - expect(loose_fk_child_table_3.where(parent_id: other_parent_record.id, status: 1).count).to eq(2) - end - end - - context 'when the child table is partitioned' do - let(:parent_child_table) { table(:_test_p_loose_fk_parent_table) } - let(:partitioned_child_table1) { table("gitlab_partitions_dynamic._test_p_loose_fk_parent_table_100") } - let(:partitioned_child_table2) { table("gitlab_partitions_dynamic._test_p_loose_fk_parent_table_101") } - - let(:loose_foreign_key_definitions) do - [ - ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( - '_test_p_loose_fk_parent_table', - '_test_loose_fk_parent_table', - { - column: 'parent_id', - on_delete: :async_delete, - gitlab_schema: :gitlab_main - } - ) - ] - end - - before do - ApplicationRecord.connection.execute(<<~SQL) - CREATE TABLE _test_p_loose_fk_parent_table ( - parent_id bigint NOT NULL, - created_at timestamptz NOT NULL, - PRIMARY KEY (created_at) - ) PARTITION BY RANGE(created_at); - - CREATE TABLE gitlab_partitions_dynamic._test_p_loose_fk_parent_table_100 PARTITION OF _test_p_loose_fk_parent_table - FOR VALUES FROM ('2020-01-01') TO ('2020-02-01'); - - CREATE TABLE gitlab_partitions_dynamic._test_p_loose_fk_parent_table_101 PARTITION OF _test_p_loose_fk_parent_table - FOR VALUES FROM ('2020-02-01') TO ('2020-03-01'); - SQL - - partitioned_child_table1.create!(parent_id: parent_record_1.id, created_at: '2020-01-02 02:00') - partitioned_child_table2.create!(parent_id: parent_record_1.id, created_at: '2020-02-02 02:00') - partitioned_child_table2.create!(parent_id: other_parent_record.id, created_at: '2020-02-02 03:00') - end - + shared_examples 'cleans up loose foreign key records' do context 'when parent records are deleted' do - it 'cleans up the child partitioned records' do - expect(parent_child_table.count).to eq(3) - expect(partitioned_child_table1.count).to eq(1) - expect(partitioned_child_table2.count).to eq(2) + let(:deleted_records_counter) { Gitlab::Metrics.client.get(:loose_foreign_key_processed_deleted_records) } + before do parent_record_1.delete - expect_next_instance_of(LooseForeignKeys::PartitionCleanerService) do |service| - expect(service).to receive(:execute).at_least(:once).and_call_original - end.at_least(:once) + expect(loose_fk_child_table_1.count).to eq(4) + expect(loose_fk_child_table_2.count).to eq(4) + expect(loose_fk_child_table_4.count).to eq(4) described_class.new( parent_table: '_test_loose_fk_parent_table', @@ -244,150 +154,207 @@ def create_table_structure deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), connection: ::ApplicationRecord.connection ).execute + end + + it 'cleans up the child records' do + expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty + expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) + expect(loose_fk_child_table_4.where(parent_id: parent_record_1.id, association_type: 'association_type_x')).to be_empty + end - expect(parent_child_table.count).to eq(1) - expect(partitioned_child_table1.count).to eq(0) - expect(partitioned_child_table2.count).to eq(1) + it 'updates the child records' do + expect(loose_fk_child_table_3.where(parent_id: parent_record_1.id, status: 4).count).to eq(2) + end + + it 'cleans up the pending parent DeletedRecord' do + expect(LooseForeignKeys::DeletedRecord.status_pending.count).to eq(0) + expect(LooseForeignKeys::DeletedRecord.status_processed.count).to eq(1) + end + + it 'records the DeletedRecord status updates', :prometheus do + counter = Gitlab::Metrics.client.get(:loose_foreign_key_processed_deleted_records) + + expect(counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) + end + + it 'does not delete unrelated records' do + expect(loose_fk_child_table_1.where(parent_id: other_parent_record.id).count).to eq(2) + expect(loose_fk_child_table_2.where(parent_id_with_different_column: other_parent_record.id).count).to eq(2) + expect(loose_fk_child_table_4.where(parent_id: parent_record_1.id, association_type: 'association_type_y').count).to eq(2) + end + + it 'does not update unrelated records' do + expect(loose_fk_child_table_3.where(parent_id: other_parent_record.id, status: 1).count).to eq(2) end end - end - describe 'fair queueing' do - context 'when the execution is over the limit' do - let(:modification_tracker) { instance_double(LooseForeignKeys::ModificationTracker) } - let(:over_limit_return_values) { [true] } - let(:deleted_record) { LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 1).first } - let(:deleted_records_rescheduled_counter) { Gitlab::Metrics.client.get(:loose_foreign_key_rescheduled_deleted_records) } - let(:deleted_records_incremented_counter) { Gitlab::Metrics.client.get(:loose_foreign_key_incremented_deleted_records) } + context 'when the child table is partitioned' do + let(:parent_child_table) { table(:_test_p_loose_fk_parent_table) } + let(:partitioned_child_table1) { table("gitlab_partitions_dynamic._test_p_loose_fk_parent_table_100") } + let(:partitioned_child_table2) { table("gitlab_partitions_dynamic._test_p_loose_fk_parent_table_101") } - let(:cleaner) do - described_class.new( - parent_table: '_test_loose_fk_parent_table', - loose_foreign_key_definitions: loose_foreign_key_definitions, - deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), - connection: ::ApplicationRecord.connection, - modification_tracker: modification_tracker - ) + let(:loose_foreign_key_definitions) do + [ + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + '_test_p_loose_fk_parent_table', + '_test_loose_fk_parent_table', + { + column: 'parent_id', + on_delete: :async_delete, + gitlab_schema: :gitlab_main + } + ) + ] end before do - parent_record_1.delete - allow(modification_tracker).to receive(:over_limit?).and_return(*over_limit_return_values) - allow(modification_tracker).to receive(:add_deletions) + ApplicationRecord.connection.execute(<<~SQL) + CREATE TABLE _test_p_loose_fk_parent_table ( + parent_id bigint NOT NULL, + created_at timestamptz NOT NULL, + PRIMARY KEY (created_at) + ) PARTITION BY RANGE(created_at); + + CREATE TABLE gitlab_partitions_dynamic._test_p_loose_fk_parent_table_100 PARTITION OF _test_p_loose_fk_parent_table + FOR VALUES FROM ('2020-01-01') TO ('2020-02-01'); + + CREATE TABLE gitlab_partitions_dynamic._test_p_loose_fk_parent_table_101 PARTITION OF _test_p_loose_fk_parent_table + FOR VALUES FROM ('2020-02-01') TO ('2020-03-01'); + SQL + + partitioned_child_table1.create!(parent_id: parent_record_1.id, created_at: '2020-01-02 02:00') + partitioned_child_table2.create!(parent_id: parent_record_1.id, created_at: '2020-02-02 02:00') + partitioned_child_table2.create!(parent_id: other_parent_record.id, created_at: '2020-02-02 03:00') end - context 'when the deleted record is under the maximum allowed cleanup attempts' do - it 'updates the cleanup_attempts column', :aggregate_failures do - deleted_record.update!(cleanup_attempts: 1) + context 'when parent records are deleted' do + it 'cleans up the child partitioned records' do + expect(parent_child_table.count).to eq(3) + expect(partitioned_child_table1.count).to eq(1) + expect(partitioned_child_table2.count).to eq(2) - cleaner.execute - - expect(deleted_record.reload.cleanup_attempts).to eq(2) - expect(deleted_records_incremented_counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) - end + parent_record_1.delete - context 'when the deleted record is above the maximum allowed cleanup attempts' do - it 'reschedules the record', :aggregate_failures do - deleted_record.update!(cleanup_attempts: LooseForeignKeys::BatchCleanerService::CLEANUP_ATTEMPTS_BEFORE_RESCHEDULE + 1) + expect_next_instance_of(LooseForeignKeys::PartitionCleanerService) do |service| + expect(service).to receive(:execute).at_least(:once).and_call_original + end.at_least(:once) - freeze_time do - cleaner.execute + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute - expect(deleted_record.reload).to have_attributes( - cleanup_attempts: 0, - consume_after: 5.minutes.from_now - ) - expect(deleted_records_rescheduled_counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) - end - end + expect(parent_child_table.count).to eq(1) + expect(partitioned_child_table1.count).to eq(0) + expect(partitioned_child_table2.count).to eq(1) end + end + end - describe 'when over limit happens on the second cleanup call without skip locked' do - # over_limit? is called twice, we test here the 2nd call - # - When invoking cleanup with SKIP LOCKED - # - When invoking cleanup (no SKIP LOCKED) - let(:over_limit_return_values) { [false, true] } + describe 'fair queueing' do + context 'when the execution is over the limit' do + let(:modification_tracker) { instance_double(LooseForeignKeys::ModificationTracker) } + let(:over_limit_return_values) { [true] } + let(:deleted_record) { LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 1).first } + let(:deleted_records_rescheduled_counter) { Gitlab::Metrics.client.get(:loose_foreign_key_rescheduled_deleted_records) } + let(:deleted_records_incremented_counter) { Gitlab::Metrics.client.get(:loose_foreign_key_incremented_deleted_records) } + + let(:cleaner) do + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection, + modification_tracker: modification_tracker + ) + end - it 'updates the cleanup_attempts column' do - expect(cleaner).to receive(:run_cleaner_service).twice + before do + parent_record_1.delete + allow(modification_tracker).to receive(:over_limit?).and_return(*over_limit_return_values) + allow(modification_tracker).to receive(:add_deletions) + end + context 'when the deleted record is under the maximum allowed cleanup attempts' do + it 'updates the cleanup_attempts column', :aggregate_failures do deleted_record.update!(cleanup_attempts: 1) cleaner.execute expect(deleted_record.reload.cleanup_attempts).to eq(2) + expect(deleted_records_incremented_counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) end - end - end - end - end - describe 'when the definition is invalid' do - let(:loose_foreign_key_definitions) do - [ - ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( - '_test_loose_fk_child_table_1', - '_test_loose_fk_parent_table', - { - column: 'parent_id', - on_delete: :async_delete, - gitlab_schema: :gitlab_main - } - ), - ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( - '_test_loose_fk_child_table_2', - '_test_loose_fk_parent_table', - { - column: 'parent_id_with_different_column', - on_delete: :not_valid, - gitlab_schema: :gitlab_main - } - ) - ] - end + context 'when the deleted record is above the maximum allowed cleanup attempts' do + it 'reschedules the record', :aggregate_failures do + deleted_record.update!(cleanup_attempts: LooseForeignKeys::BatchCleanerService::CLEANUP_ATTEMPTS_BEFORE_RESCHEDULE + 1) - before do - parent_record_1.delete - end + freeze_time do + cleaner.execute - it 'logs error and skips the definition' do - expect(Sidekiq.logger).to receive(:error).with("Invalid on_delete argument: not_valid").twice - expect(Sidekiq.logger).to receive(:error).with("Invalid on_delete argument for definition: _test_loose_fk_child_table_2").twice + expect(deleted_record.reload).to have_attributes( + cleanup_attempts: 0, + consume_after: 5.minutes.from_now + ) + expect(deleted_records_rescheduled_counter.get(table: loose_fk_parent_table.table_name, db_config_name: 'main')).to eq(1) + end + end + end - described_class.new( - parent_table: '_test_loose_fk_parent_table', - loose_foreign_key_definitions: loose_foreign_key_definitions, - deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), - connection: ::ApplicationRecord.connection - ).execute + describe 'when over limit happens on the second cleanup call without skip locked' do + # over_limit? is called twice, we test here the 2nd call + # - When invoking cleanup with SKIP LOCKED + # - When invoking cleanup (no SKIP LOCKED) + let(:over_limit_return_values) { [false, true] } - expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty - expect(loose_fk_child_table_2.where(parent_id_with_different_column: parent_record_1.id).count).to eq(2) - end - end + it 'updates the cleanup_attempts column' do + expect(cleaner).to receive(:run_cleaner_service).twice - describe 'feature flag: loose_foreign_key_worker_improvements' do - before do - parent_record_1.delete - end + deleted_record.update!(cleanup_attempts: 1) - context 'when feature flag is enabled' do - before do - stub_feature_flags(loose_foreign_key_worker_improvements: true) + cleaner.execute + + expect(deleted_record.reload.cleanup_attempts).to eq(2) + end + end + end end + end - it 'uses new cleaner service with feature category context' do - expect(Gitlab::ApplicationContext).to receive(:with_context).exactly(4).times.and_call_original + describe 'when the definition is invalid' do + let(:loose_foreign_key_definitions) do + [ + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + '_test_loose_fk_child_table_1', + '_test_loose_fk_parent_table', + { + column: 'parent_id', + on_delete: :async_delete, + gitlab_schema: :gitlab_main + } + ), + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + '_test_loose_fk_child_table_2', + '_test_loose_fk_parent_table', + { + column: 'parent_id_with_different_column', + on_delete: :not_valid, + gitlab_schema: :gitlab_main + } + ) + ] + end - described_class.new( - parent_table: '_test_loose_fk_parent_table', - loose_foreign_key_definitions: loose_foreign_key_definitions, - deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), - connection: ::ApplicationRecord.connection - ).execute + before do + parent_record_1.delete end - it 'still cleans up records correctly' do + it 'logs error and skips the definition' do + expect(Sidekiq.logger).to receive(:error).with("Invalid on_delete argument: not_valid").twice + expect(Sidekiq.logger).to receive(:error).with("Invalid on_delete argument for definition: _test_loose_fk_child_table_2").twice + described_class.new( parent_table: '_test_loose_fk_parent_table', loose_foreign_key_definitions: loose_foreign_key_definitions, @@ -396,37 +363,50 @@ def create_table_structure ).execute expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty - expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) + expect(loose_fk_child_table_2.where(parent_id_with_different_column: parent_record_1.id).count).to eq(2) end end + end - context 'when feature flag is disabled' do - before do - stub_feature_flags(loose_foreign_key_worker_improvements: false) - end + context 'with feature flag enabled (default)' do + before do + stub_feature_flags(loose_foreign_key_worker_improvements: true) + end - it 'uses original cleaner service without feature category context' do - expect(Gitlab::ApplicationContext).not_to receive(:with_context) + it_behaves_like 'cleans up loose foreign key records' - described_class.new( - parent_table: '_test_loose_fk_parent_table', - loose_foreign_key_definitions: loose_foreign_key_definitions, - deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), - connection: ::ApplicationRecord.connection - ).execute - end + it 'uses new cleaner service with feature category context' do + parent_record_1.delete - it 'still cleans up records correctly' do - described_class.new( - parent_table: '_test_loose_fk_parent_table', - loose_foreign_key_definitions: loose_foreign_key_definitions, - deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), - connection: ::ApplicationRecord.connection - ).execute + expect(Gitlab::ApplicationContext).to receive(:with_context).exactly(4).times.and_call_original - expect(loose_fk_child_table_1.where(parent_id: parent_record_1.id)).to be_empty - expect(loose_fk_child_table_2.where(parent_id_with_different_column: nil).count).to eq(2) - end + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute + end + end + + context 'with feature flag disabled' do + before do + stub_feature_flags(loose_foreign_key_worker_improvements: false) + end + + it_behaves_like 'cleans up loose foreign key records' + + it 'uses original cleaner service without feature category context' do + parent_record_1.delete + + expect(Gitlab::ApplicationContext).not_to receive(:with_context) + + described_class.new( + parent_table: '_test_loose_fk_parent_table', + loose_foreign_key_definitions: loose_foreign_key_definitions, + deleted_parent_records: LooseForeignKeys::DeletedRecord.load_batch_for_table('public._test_loose_fk_parent_table', 100), + connection: ::ApplicationRecord.connection + ).execute end end end -- GitLab