feat: raw delete expired instead of `Queryset.delete` #235

daadu · 2021-09-27T09:20:51Z

This is avoid unncessary loading of objects in-memory in certain conditions.

From Django docs:

The delete() method does a bulk delete and does not call any delete() methods on your models. It does, however, emit the pre_delete and post_delete signals for all deleted objects (including cascaded deletions).

Django needs to fetch objects into memory to send signals and handle cascades. However, if there are no cascades and no signals, then Django may take a fast-path and delete objects without fetching into memory. For large deletes this can result in significantly reduced memory usage. The amount of executed queries can be reduced, too.

Closes #232

This is avoid unncessary loading of objects in-memory in certain conditions

daadu · 2021-09-27T09:24:04Z

There is already a test that checks if the delete_expired method is functioning properly - so in case of breaking change in "protected Django method" - we will be able to catch it (Hopefully!)

daadu · 2021-09-27T09:25:18Z

@auvipy - Requesting review

auvipy · 2021-09-27T09:43:01Z

thanks for the pr

auvipy

I still want to see some new unit tests

daadu · 2021-09-27T10:47:55Z

@auvipy unit test against raw_delete added as suggested.

auvipy · 2021-09-27T10:49:11Z

django_celery_results/managers.py

@@ -88,7 +88,7 @@ def get_all_expired(self, expires):
    def delete_expired(self, expires):
        """Delete all expired results."""
        with transaction.atomic():
-            self.get_all_expired(expires).delete()
+            raw_delete(queryset=self.get_all_expired(expires))


another thing i want to be assured that running makemigrations detect any change in migrations

I added similar test for django-silk two days back, where i simply called management command makemigrations silk --check --dry-run - if there are pending migrations then it returns with exit code 1.

Should we implement the same?

that would be great!

There is already this test which does that.

django-celery-results/t/unit/test_migrations.py

Lines 26 to 32 in 0a00044

def test_models_match_migrations(self):

"""Make sure that no model changes exist.

This logic is taken from django's makemigrations.py file.

Here just detect if model changes exist that require

a migration, and if so we fail.

"""

should we replace this with call_command thing i mentioned above? One advantage is that it is a higher level Django API - changes in Django internal will not affect the test - until only the command behaviour is changed.

which part do you want to replace with what? can you elaborate please?

Running makemigrations django_celery_results --check --dry-run command inside the test as mentioned above.

made those changes. Please review.

commit - 113ac2e

auvipy · 2021-09-27T10:50:02Z

django_celery_results/managers.py

@@ -88,7 +88,7 @@ def get_all_expired(self, expires):
    def delete_expired(self, expires):


tests for this method needs to be updated as well

what kind of update? the current test looks sufficient to me.

…calling internal logic of `makemigrations` to check if no pending migrations

auvipy · 2021-09-28T09:55:06Z

can you check why CI is failing please

daadu · 2021-09-28T11:47:27Z

@auvipy Looks like the CI is stuck.

auvipy · 2021-10-11T18:25:32Z

CI is running green :D

auvipy · 2021-10-11T18:31:23Z

going to merge this for now. future improvements are always welcome

ForthTurn · 2023-01-13T06:58:00Z

I have a django model with a ForeignKey to TaskResult, and we set the on_delete to models.CASCADE. Maybe there is a problem after this change because the _raw_delete will not cause django's CASCADE delete?

auvipy · 2023-01-13T08:36:26Z

can you please share more detail?

ForthTurn · 2023-01-16T04:32:14Z

I have a django model named MyTaskResult, here is the code:

class MyTaskResult(models.Model):
    result = models.OneToOneField(
        TaskResult,
        on_delete=models.CASCADE,
        verbose_name='TaskResult',
    )
    # other fields

and it works well when django-celery-results at 2.2.0.
But if update to 2.3.0 which the version include this mr, when I triggered TaskResult.objects.delete_expired():

from datetime import timedelta
from django_celery_results.models import TaskResult

t = timedelta(minutes=1)
TaskResult.objects.delete_expired(t)

I got this error:

django.db.utils.IntegrityError: (1451, 'Cannot delete or update a parent row: a foreign key constraint fails (..., CONSTRAINT `..._result_id_bbca4d3d_fk_django_ce` FOREIGN KEY (`result_id`) REFERENCES `django_celery_results_taskresult` (`id`))')"

I think the builtin task celery.backend_cleanup will also trigger this method and raise this exception.

In 2.2.0, delete_expired()calls self.get_all_expired(expires).delete(), and it will make django to delete the MyTaskResult before delete TaskResult.

auvipy · 2023-01-16T08:13:08Z

can you help to partially revert this manually? or provide a bettter mechanism??

ForthTurn · 2023-01-16T11:35:07Z

can you help to partially revert this manually? or provide a bettter mechanism??

Maybe can add a config key to switch? I saw it in here, but not in this mr

ForthTurn · 2023-01-17T03:11:16Z

And I think the default behavior should be to use queryset's delete instead of raw_delete. Queryset.delete can be guaranteed to be correct. If the users can confirm they don't have a cascade ForeignKey or listen to the delete signal, they can switch the config to reduce the usage of memory.

auvipy · 2023-01-17T08:44:03Z

And I think the default behavior should be to use queryset's delete instead of raw_delete. Queryset.delete can be guaranteed to be correct. If the users can confirm they don't have a cascade ForeignKey or listen to the delete signal, they can switch the config to reduce the usage of memory.

I will be happy to accept a PR with partial revert of the PR[raw-delete part]

ForthTurn · 2023-01-18T12:03:47Z

I will be happy to accept a PR with partial revert of the PR[raw-delete part]

I will submit this at a later date :)

…#235)" This partially reverts commit 7c7cb67.

…#384) * Revert "feat: raw delete expired instead of `Queryset.delete` (#235)" This partially reverts commit 7c7cb67. * Fix tests for Django 4.2 --------- Co-authored-by: Asif Saif Uddin <auvipy@gmail.com>

daadu added 2 commits September 27, 2021 14:50

feat: raw delete expired instead of Queryset.delete

0b63256

This is avoid unncessary loading of objects in-memory in certain conditions

minor: verbose fix

c256d82

daadu added 3 commits September 27, 2021 15:15

minor: verbose fix

350aeb1

minor: verbose fix

df10206

minor: verbose simplified

ed859b5

auvipy requested changes Sep 27, 2021

View reviewed changes

daadu added 3 commits September 27, 2021 16:02

test: unit test for utils.raw_delete added

6559dd9

minor: fix some lint issues

e15a3de

minor: fix some lint issues

d3099e3

auvipy requested changes Sep 27, 2021

View reviewed changes

daadu added 2 commits September 27, 2021 17:22

minor: fix method names

9c65797

[test] MigrationTests: calling makemigrations --check instead of …

113ac2e

…calling internal logic of `makemigrations` to check if no pending migrations

daadu requested a review from auvipy September 28, 2021 07:06

[test] import optimized

b8aaad2

[test] fix - wrong function name

ba0efdc

auvipy approved these changes Oct 11, 2021

View reviewed changes

auvipy merged commit 7c7cb67 into celery:master Oct 11, 2021

daadu deleted the raw-delete-cleanup branch October 12, 2021 06:12

hansegucker mentioned this pull request Apr 22, 2023

backend_cleanup fails when TaskResult is referenced in a model #383

Closed

hansegucker mentioned this pull request Apr 22, 2023

Revert "feat: raw delete expired instead of Queryset.delete (#235)" #384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: raw delete expired instead of `Queryset.delete` #235

feat: raw delete expired instead of `Queryset.delete` #235

daadu commented Sep 27, 2021 •

edited

Loading

daadu commented Sep 27, 2021

daadu commented Sep 27, 2021

auvipy commented Sep 27, 2021

auvipy left a comment

daadu commented Sep 27, 2021

auvipy Sep 27, 2021

daadu Sep 27, 2021

auvipy Sep 27, 2021

daadu Sep 27, 2021

daadu Sep 27, 2021

auvipy Sep 27, 2021

daadu Sep 27, 2021

auvipy Sep 27, 2021

daadu Sep 28, 2021

daadu Sep 28, 2021

auvipy Sep 27, 2021

daadu Sep 27, 2021

auvipy commented Sep 28, 2021

daadu commented Sep 28, 2021

auvipy commented Oct 11, 2021

auvipy commented Oct 11, 2021

ForthTurn commented Jan 13, 2023

auvipy commented Jan 13, 2023

ForthTurn commented Jan 16, 2023

auvipy commented Jan 16, 2023

ForthTurn commented Jan 16, 2023

ForthTurn commented Jan 17, 2023

auvipy commented Jan 17, 2023

ForthTurn commented Jan 18, 2023

	def test_models_match_migrations(self):
	"""Make sure that no model changes exist.

	This logic is taken from django's makemigrations.py file.
	Here just detect if model changes exist that require
	a migration, and if so we fail.
	"""

		@@ -88,7 +88,7 @@ def get_all_expired(self, expires):
		def delete_expired(self, expires):

feat: raw delete expired instead of Queryset.delete #235

feat: raw delete expired instead of Queryset.delete #235

Conversation

daadu commented Sep 27, 2021 • edited Loading

daadu commented Sep 27, 2021

daadu commented Sep 27, 2021

auvipy commented Sep 27, 2021

auvipy left a comment

Choose a reason for hiding this comment

daadu commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvipy commented Sep 28, 2021

daadu commented Sep 28, 2021

auvipy commented Oct 11, 2021

auvipy commented Oct 11, 2021

ForthTurn commented Jan 13, 2023

auvipy commented Jan 13, 2023

ForthTurn commented Jan 16, 2023

auvipy commented Jan 16, 2023

ForthTurn commented Jan 16, 2023

ForthTurn commented Jan 17, 2023

auvipy commented Jan 17, 2023

ForthTurn commented Jan 18, 2023

feat: raw delete expired instead of `Queryset.delete` #235

feat: raw delete expired instead of `Queryset.delete` #235

daadu commented Sep 27, 2021 •

edited

Loading