Projects: improve deletion (#12456)

For some related fields, Django first does a select, then it probably
loads each object into memory? But after that it does the deletion using
a `IN` statement with the ID of the objects...

Using raw_delete avoids that, but it can also cause integrity problems,
as Django does its own implementation of on_delete, so I chose models
that don't have additional relations and also shouldn't have problems as
they are always deleted when the project is deleted. Another source of
problems is when models have foreign fields with on_delete=SET_NULL, as
django will do an UPDATE over the model with all the related fields...
for example for latest_build, django will run an update over all
projects with an `IN (... all builds IDs..)`, to set the latest_build ID
to null..., which is silly since the whole project model is being
deleted, but django doesn't know that lates_build can only contain a
build from the project. A raw delete could help, but builds and versions
have a lot of related fields in other models, so I'm a little hesitant.

Closes #12410
Ref #10040

Author: stsewd

readthedocs/projects/models.py

@@ -691,6 +691,15 @@ def save(self, *args, **kwargs):
     def delete(self, *args, **kwargs):
         from readthedocs.projects.tasks.utils import clean_project_resources
 
+        # NOTE: We use _raw_delete to avoid Django fetching all objects
+        # before the deletion. Be careful when using _raw_delete, signals
+        # won't be sent, and can cause integrity problems if the model
+        # has relations with other models.
+        qs = self.page_views.all()
+        qs._raw_delete(qs.db)
+        qs = self.search_queries.all()
+        qs._raw_delete(qs.db)
+
         # Remove extra resources
         clean_project_resources(self)
 

readthedocs/projects/tasks/utils.py

@@ -87,6 +87,13 @@ def clean_project_resources(project, version=None, version_slug=None):
      sometimes to clean old resources.
 
     .. note::
+
+       This function shouldn't delete objects that can't be recreated
+       by re-activating the version (e.g. page views, search queries),
+       as it's used when a version is deactivated.
+
+    .. note::
+
        This function is usually called just before deleting project.
        Make sure to not depend on the project object inside the tasks.
     """
@@ -108,11 +115,16 @@ def clean_project_resources(project, version=None, version_slug=None):
         version_slug=version_slug,
     )
 
-    # Remove imported files
+    # NOTE: We use _raw_delete to avoid Django fetching all objects
+    # before the deletion. Be careful when using _raw_delete, signals
+    # won't be sent, and can cause integrity problems if the model
+    # has relations with other models.
     if version:
-        version.imported_files.all().delete()
+        qs = version.imported_files.all()
+        qs._raw_delete(qs.db)
     else:
-        project.imported_files.all().delete()
+        qs = project.imported_files.all()
+        qs._raw_delete(qs.db)
 
 
 @app.task()

readthedocs/projects/tests/test_models.py

@@ -3,7 +3,10 @@
 from django.test import TestCase
 from django_dynamic_fixture import get
 
-from readthedocs.projects.models import Feature, Project
+from readthedocs.analytics.models import PageView
+from readthedocs.builds.models import Build, Version
+from readthedocs.projects.models import Feature, ImportedFile, Project
+from readthedocs.search.models import SearchQuery
 
 
 class TestURLPatternsUtils(TestCase):
@@ -137,3 +140,15 @@ def test_proxied_api_prefix(self):
         self.assertEqual(self.project.proxied_api_url, "prefix/_/")
         self.assertEqual(self.project.proxied_api_host, "/prefix/_")
         self.assertEqual(self.project.proxied_api_prefix, "/prefix/")
+
+    def test_number_of_queries_on_project_deletion(self):
+        for i in range(5):
+            version = get(Version, project=self.project, slug=f"subproject-{i}", active=True, built=True)
+            for _ in range(50):
+                get(PageView, project=self.project, version=version)
+                get(ImportedFile, project=self.project, version=version)
+                get(SearchQuery, project=self.project, version=version)
+                get(Build, project=self.project, version=version)
+
+        with self.assertNumQueries(48):
+            self.project.delete()