Skip to content

Fix remaining cross-joins in old migrations that have already run

Dylan Griffith requested to merge 340017-remove-unnecessary-cross-joins-for-old-background-migrations into master

What does this MR do and why?

TL;DR this MR is just deleting code that is in old migrations that we don't need anymore and is causing problems with some tests

Cross-joins are described at https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-cross-database-transactions . Basically these are single queries that span the Main and the CI database (now that we have 2 separate databases). They are blocked by a middleware we run in our tests which had to have a special allowlist for old migrations that we won't actually need to run ever again. This MR removes the code and the allowlist entries.

As part of #340017 (closed) we want to empty out spec/support/database/cross-join-allowlist.yml and remove all remaining uses of allow_cross_joins_across_databases. The only things leftover were old migrations so this MR handles all of those.

Details about the migrations and why we're allowed to remove them

All of these migrations were added or last referenced across the following MRs which were all 14.x and therefore it is safe to now remove them. Per https://docs.gitlab.com/ee/update/#upgrading-to-a-new-major-version it is enforced that all clients upgrade to the latest minor release before upgrading major releases (in this case 15.x) and they must also ensure all background migrations have completed before they upgrade which means that making these a no-op now in 15.x is fine.

We also have some documentation for making migrations a no-op in https://docs.gitlab.com/ee/development/database/deleting_migrations.html which is OK so long as there are no schema changes.

The one migration in this MR that has schema changes is db/post_migrate/20210907211557_finalize_ci_builds_bigint_conversion.rb . In this case since it also has already been run I just removed the offending code that created cross-joins which was only the LOCK statements. These lock statements were only used as a performance improvement to reduce the likelihood of lock timeouts. Since, again, we can expect this will have already before this point we don't need to worry about this peformance optimization.

The MRs that added these migrations originally are:

  1. !65201 (merged) => added 20210907211557_finalize_ci_builds_bigint_conversion
  2. !71944 (merged) => added 20211007155221_schedule_populate_status_column_of_security_scans.rb
  3. !79344 (merged) => finalized BackfillCiNamespaceMirrors and BackfillCiProjectMirrors

I also decided to just remove any specs for the migrations (even the no-op ones) as there is no value in testing a no-op migration. Technically the migrations still get executed in CI anyway so it doesn't really matter whether or not we have an empty no-op spec.

Screenshots or screen recordings

How to set up and validate locally

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Closes #340017 (closed)

Edited by Dylan Griffith

Merge request reports