Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform batch deletion of invalid records before applying new foreign keys #2588

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

zackgalbreath
Copy link
Contributor

Perform batch deletion of invalid records before applying new foreign keys

Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why doing this is preferable to simply leaving the migrations as-is?

Two concerns here:

  1. Broadly speaking, modifying old migrations adds additional risk, even if the changes are small. If there isn't a meaningful reason why editing a given migration is needed (e.g., a critical failure or performance issue), I don't like the idea of introducing additional risks.
  2. One of the biggest issues with containerized deployments of CDash is the fact that deploying an older version does not automatically roll back newer migrations, or give the option to do so manually. One solution to this problem used by some migration systems is to store the text of the migration in the migrations table itself, so older versions of CDash can roll back newer migrations. If we ever want to do that, compatibility with code from outside the migration will be a major concern. As such, I think it's better to keep all of the relevant code (excluding a handful of library functions from Laravel) contained within each migration file whenever possible. Even if we don't end up storing migration logic in the database, there is still the issue of not wanting to edit old migrations if the functions being referenced are ever refactored.

@zackgalbreath
Copy link
Contributor Author

Is there a reason why doing this is preferable to simply leaving the migrations as-is?

Broadly speaking, the motivation behind this proposed change is to improve the performance of these recent migrations. In particular, the buildfailure2argument table is quite large on some of the production instances we support (> 100 million rows).

@williamjallen
Copy link
Collaborator

What do you think about copying and pasting the code into each of the migrations so they are independent of the rest of the codebase? It feels wrong to do in principle, but I think it's acceptable given that migrations are essentially one-time use code snippets. I could be convinced otherwise if you have strong opinions on this though.

@zackgalbreath zackgalbreath force-pushed the migrations_batch_delete branch from 65a14fb to 0b058ae Compare December 2, 2024 19:38
@zackgalbreath zackgalbreath changed the title Use DatabaseCleanupUtils::deleteUnusedRows() in recent migrations Perform batch deletion of invalid records before applying new foreign keys Dec 2, 2024
@zackgalbreath zackgalbreath added this pull request to the merge queue Dec 2, 2024
Merged via the queue into master with commit bfd5bbc Dec 2, 2024
6 checks passed
@zackgalbreath zackgalbreath deleted the migrations_batch_delete branch December 2, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants