From 9f5c75418832f855fa2f037f2341885d2aaeab4a Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Wed, 12 Mar 2025 09:03:34 +0000
Subject: [PATCH] Cleanup migrations adding merge_request_diff_files.project FK
This foreign key caused the incident
https://gitlab.com/gitlab-com/gl-infra/production/-/issues/19474 . It
was added in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177799 . The
problem is that we added a foreign key before adding an index. This
means that all statements like `DELETE FROM projects ...` will need to
scan the entire `merge_request_diff_files` table to check if the foreign
key is valid. This was causing timeouts.
In order to resolve the incident we've already manually deleted this
migration on GitLab.com . This MR additionally adds another migration to
remove the foreign key to clean up any environments where this migration
may have already run.
---
..._project_id_to_merge_request_diff_files.rb | 8 +++++++-
...ect_id_fk_from_merge_request_diff_files.rb | 20 +++++++++++++++++++
db/schema_migrations/20250312061803 | 1 +
db/structure.sql | 3 ---
spec/db/schema_spec.rb | 2 +-
5 files changed, 29 insertions(+), 5 deletions(-)
create mode 100644 db/migrate/20250312061803_remove_project_id_fk_from_merge_request_diff_files.rb
create mode 100644 db/schema_migrations/20250312061803
diff --git a/db/migrate/20250224231330_add_project_id_to_merge_request_diff_files.rb b/db/migrate/20250224231330_add_project_id_to_merge_request_diff_files.rb
index c3efa0dabe8ea..d4a02faf30c0c 100644
--- a/db/migrate/20250224231330_add_project_id_to_merge_request_diff_files.rb
+++ b/db/migrate/20250224231330_add_project_id_to_merge_request_diff_files.rb
@@ -16,7 +16,13 @@ def up
add_column SOURCE_TABLE, :project_id, :bigint, if_not_exists: true
end
- add_concurrent_foreign_key SOURCE_TABLE, :projects, column: :project_id, on_delete: :cascade
+ # This caused the incident https://gitlab.com/gitlab-com/gl-infra/production/-/issues/19474
+ # We must first add the index before adding a foreign key. We also explicitly removed it in
+ # db/post_migrate/20250312061803_remove_project_id_fk_from_merge_request_diff_files.rb to clean up any installations
+ # that ran this.
+ #
+ # no-op:
+ # add_concurrent_foreign_key SOURCE_TABLE, :projects, column: :project_id, on_delete: :cascade
end
def down
diff --git a/db/migrate/20250312061803_remove_project_id_fk_from_merge_request_diff_files.rb b/db/migrate/20250312061803_remove_project_id_fk_from_merge_request_diff_files.rb
new file mode 100644
index 0000000000000..705cbf8e2f5a6
--- /dev/null
+++ b/db/migrate/20250312061803_remove_project_id_fk_from_merge_request_diff_files.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class RemoveProjectIdFkFromMergeRequestDiffFiles < Gitlab::Database::Migration[2.2]
+ milestone '17.10'
+
+ disable_ddl_transaction!
+
+ FK_NAME = 'fk_0e3ba01603'
+
+ def up
+ with_lock_retries do
+ remove_foreign_key_if_exists(:merge_request_diff_files, :projects, name: FK_NAME, reverse_lock_order: true)
+ end
+ end
+
+ def down
+ # no-op
+ # we won't add this again, as it may create problems without an index
+ end
+end
diff --git a/db/schema_migrations/20250312061803 b/db/schema_migrations/20250312061803
new file mode 100644
index 0000000000000..c8518ffaa1147
--- /dev/null
+++ b/db/schema_migrations/20250312061803
@@ -0,0 +1 @@
+c643bd5a8781d57cf943170020f44972ad0e06faa1f602b80409eea7704310ee
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 893f765bddbc4..e320d2edfdfe8 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -39401,9 +39401,6 @@ ALTER TABLE ONLY subscription_user_add_on_assignments
ALTER TABLE ONLY approval_project_rules_users
ADD CONSTRAINT fk_0dfcd9e339 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
-ALTER TABLE ONLY merge_request_diff_files
- ADD CONSTRAINT fk_0e3ba01603 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
-
ALTER TABLE ONLY ci_runner_projects
ADD CONSTRAINT fk_0e743433ff FOREIGN KEY (runner_id) REFERENCES ci_runners_archived(id) ON DELETE CASCADE;
diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb
index 7e5ee95a09b77..71e1bcaa45a32 100644
--- a/spec/db/schema_spec.rb
+++ b/spec/db/schema_spec.rb
@@ -31,7 +31,6 @@
ci_sources_pipelines: [%w[source_partition_id source_pipeline_id], %w[partition_id pipeline_id]],
ci_sources_projects: [%w[partition_id pipeline_id]], # index on pipeline_id is sufficient
ci_stages: [%w[partition_id pipeline_id]], # the index on pipeline_id is sufficient
- merge_request_diff_files: [%w[project_id]], # async index to be created - https://gitlab.com/gitlab-org/gitlab/-/issues/523103
notes: %w[namespace_id], # this index is added in an async manner, hence it needs to be ignored in the first phase.
p_ci_build_trace_metadata: [%w[partition_id build_id], %w[partition_id trace_artifact_id]], # the index on build_id is enough
p_ci_builds: [%w[partition_id stage_id], %w[partition_id execution_config_id], %w[auto_canceled_by_partition_id auto_canceled_by_id], %w[upstream_pipeline_partition_id upstream_pipeline_id], %w[partition_id commit_id]], # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142804#note_1745483081
@@ -172,6 +171,7 @@
merge_request_cleanup_schedules: %w[project_id],
merge_requests_compliance_violations: %w[target_project_id],
merge_request_diffs: %w[project_id],
+ merge_request_diff_files: %w[project_id],
merge_request_diff_commits: %w[commit_author_id committer_id],
# merge_request_diff_commits_b5377a7a34 is the temporary table for the merge_request_diff_commits partitioning
# backfill. It will get foreign keys after the partitioning is finished.
--
GitLab