Merge branch 'remove-external-pull-requests-deletion-tracking' into 'master'

Untrack external_pull_requests row deletions

See merge request gitlab-org/gitlab!82476

db/post_migrate/20220309084838_remove_external_pull_request_tracking.rb

+# frozen_string_literal: true
+
+class RemoveExternalPullRequestTracking < Gitlab::Database::Migration[1.0]
+  include Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers
+
+  enable_lock_retries!
+
+  def up
+    untrack_record_deletions(:external_pull_requests)
+  end
+
+  def down
+    track_record_deletions(:external_pull_requests)
+  end
+end

db/post_migrate/20220309084954_remove_leftover_external_pull_request_deletions.rb

+# frozen_string_literal: true
+
+class RemoveLeftoverExternalPullRequestDeletions < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    # Delete all pending record deletions in the public.external_pull_requests until
+    # there are no more rows left.
+    loop do
+      result = execute <<~SQL
+      DELETE FROM "loose_foreign_keys_deleted_records"
+      WHERE
+      ("loose_foreign_keys_deleted_records"."partition", "loose_foreign_keys_deleted_records"."id") IN (
+        SELECT "loose_foreign_keys_deleted_records"."partition", "loose_foreign_keys_deleted_records"."id"
+        FROM "loose_foreign_keys_deleted_records"
+        WHERE
+        "loose_foreign_keys_deleted_records"."fully_qualified_table_name" = 'public.external_pull_requests' AND
+        "loose_foreign_keys_deleted_records"."status" = 1
+        LIMIT 100
+      )
+      SQL
+
+      break if result.cmd_tuples == 0
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20220309084838

+d9d17f94f54840eace48f210e3886423a8dc04109f2ebca8d8edb7d53e0b5688
\ No newline at end of file

db/schema_migrations/20220309084954

+6d9c5454372317955c4e16b5a02dece575221f15af60c33df45fffbca169c08c
\ No newline at end of file

db/structure.sql

@@ -30858,8 +30858,6 @@ CREATE TRIGGER ci_pipelines_loose_fk_trigger AFTER DELETE ON ci_pipelines REFERE
 
 CREATE TRIGGER ci_runners_loose_fk_trigger AFTER DELETE ON ci_runners REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();
 
-CREATE TRIGGER external_pull_requests_loose_fk_trigger AFTER DELETE ON external_pull_requests REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();
-
 CREATE TRIGGER merge_requests_loose_fk_trigger AFTER DELETE ON merge_requests REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();
 
 CREATE TRIGGER namespaces_loose_fk_trigger AFTER DELETE ON namespaces REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();

doc/development/database/loose_foreign_keys.md

@@ -249,6 +249,67 @@ end
 At this point, the setup phase is concluded. The deleted `projects` records should be automatically
 picked up by the scheduled cleanup worker job.
 
+### Remove the loose foreign key
+
+When the loose foreign key definition is no longer needed (parent table is removed, or FK is restored),
+we need to remove the definition from the YAML file and ensure that we don't leave pending deleted
+records in the database.
+
+1. Remove the loose foreign key definition from the config (`config/gitlab_loose_foreign_keys.yml`).
+1. Remove the deletion tracking trigger from the parent table (if the parent table is still there).
+1. Remove leftover deleted records from the `loose_foreign_keys_deleted_records` table.
+
+Migration for removing the trigger:
+
+```ruby
+class UnTrackProjectRecordChanges < Gitlab::Database::Migration[1.0]
+  include Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers
+
+  enable_lock_retries!
+
+  def up
+    untrack_record_deletions(:projects)
+  end
+
+  def down
+    track_record_deletions(:projects)
+  end
+end
+```
+
+With the trigger removal, we prevent further records to be inserted in the `loose_foreign_keys_deleted_records`
+table however, there is still a chance for having leftover pending records in the table. These records
+must be removed with an inline data migration.
+
+```ruby
+class RemoveLeftoverProjectDeletions < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    loop do
+      result = execute <<~SQL
+      DELETE FROM "loose_foreign_keys_deleted_records"
+      WHERE
+      ("loose_foreign_keys_deleted_records"."partition", "loose_foreign_keys_deleted_records"."id") IN (
+        SELECT "loose_foreign_keys_deleted_records"."partition", "loose_foreign_keys_deleted_records"."id"
+        FROM "loose_foreign_keys_deleted_records"
+        WHERE
+        "loose_foreign_keys_deleted_records"."fully_qualified_table_name" = 'public.projects' AND
+        "loose_foreign_keys_deleted_records"."status" = 1
+        LIMIT 100
+      )
+      SQL
+
+      break if result.cmd_tuples == 0
+    end
+  end
+
+  def down
+    # no-op
+  end
+end
+```
+
 ## Testing
 
 The "`it has loose foreign keys`" shared example can be used to test the presence of the `ON DELETE` trigger and the

spec/migrations/20220309084954_remove_leftover_external_pull_request_deletions_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe RemoveLeftoverExternalPullRequestDeletions do
+  let(:deleted_records) { table(:loose_foreign_keys_deleted_records) }
+
+  let(:pending_record1) { deleted_records.create!(id: 1, fully_qualified_table_name: 'public.external_pull_requests', primary_key_value: 1, status: 1) }
+  let(:pending_record2) { deleted_records.create!(id: 2, fully_qualified_table_name: 'public.external_pull_requests', primary_key_value: 2, status: 1) }
+  let(:other_pending_record1) { deleted_records.create!(id: 3, fully_qualified_table_name: 'public.projects', primary_key_value: 1, status: 1) }
+  let(:other_pending_record2) { deleted_records.create!(id: 4, fully_qualified_table_name: 'public.ci_builds', primary_key_value: 1, status: 1) }
+  let(:processed_record1) { deleted_records.create!(id: 5, fully_qualified_table_name: 'public.external_pull_requests', primary_key_value: 3, status: 2) }
+  let(:other_processed_record1) { deleted_records.create!(id: 6, fully_qualified_table_name: 'public.ci_builds', primary_key_value: 2, status: 2) }
+
+  let!(:persisted_ids_before) do
+    [
+      pending_record1,
+      pending_record2,
+      other_pending_record1,
+      other_pending_record2,
+      processed_record1,
+      other_processed_record1
+    ].map(&:id).sort
+  end
+
+  let!(:persisted_ids_after) do
+    [
+      other_pending_record1,
+      other_pending_record2,
+      processed_record1,
+      other_processed_record1
+    ].map(&:id).sort
+  end
+
+  def all_ids
+    deleted_records.all.map(&:id).sort
+  end
+
+  it 'deletes pending external_pull_requests records' do
+    expect { migrate! }.to change { all_ids }.from(persisted_ids_before).to(persisted_ids_after)
+  end
+end